Solr
  1. Solr
  2. SOLR-6902

Use JUnit rules instead of inheritance with distributed Solr tests to allow for multiple tests without the same class

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.1, 6.0
    • Component/s: Tests
    • Labels:
      None

      Description

      Finally got annoyed enough with too many things being clubbed into one test method in all distributed Solr tests (anything inheriting from BaseDistributedSearchTestCase and currently implementing doTest)..

      This just lays the groundwork really for allowing multiple test methods within the same class, and doesn't split tests as yet or flatten the inheritance hierarchy (when abused for doing multiple tests), as this touches a lot of files by itself. For that reason, the sooner this is picked up the better..

      1. SOLR-6902.patch
        174 kB
        Erick Erickson
      2. SOLR-6902.patch
        173 kB
        Erick Erickson
      3. SOLR-6902.patch
        170 kB
        Ramkumar Aiyengar
      4. SOLR-6902.patch
        175 kB
        Erick Erickson
      5. SOLR-6902.patch
        190 kB
        Ramkumar Aiyengar

        Issue Links

          Activity

          Hide
          ASF GitHub Bot added a comment -

          GitHub user andyetitmoves opened a pull request:

          https://github.com/apache/lucene-solr/pull/121

          Use JUnit rules instead of inheritance with distributed Solr tests to allow for multiple tests without the same class

          Patch for SOLR-6902

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/bloomberg/lucene-solr trunk-shard-junit-rule

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/lucene-solr/pull/121.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #121


          commit 02d678044a9f399a6bdf63e9a977d6e760a0a1bf
          Author: Ramkumar Aiyengar <raiyengar@bloomberg.net>
          Date: 2014-12-30T14:39:23Z

          Use JUnit rules instead of inheritance with distributed tests to allow for multiple tests without the same class.


          Show
          ASF GitHub Bot added a comment - GitHub user andyetitmoves opened a pull request: https://github.com/apache/lucene-solr/pull/121 Use JUnit rules instead of inheritance with distributed Solr tests to allow for multiple tests without the same class Patch for SOLR-6902 You can merge this pull request into a Git repository by running: $ git pull https://github.com/bloomberg/lucene-solr trunk-shard-junit-rule Alternatively you can review and apply these changes as the patch at: https://github.com/apache/lucene-solr/pull/121.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #121 commit 02d678044a9f399a6bdf63e9a977d6e760a0a1bf Author: Ramkumar Aiyengar <raiyengar@bloomberg.net> Date: 2014-12-30T14:39:23Z Use JUnit rules instead of inheritance with distributed tests to allow for multiple tests without the same class.
          Hide
          Ramkumar Aiyengar added a comment -

          There's a bit of a subtlety here btw with a re-implementation of the setUp/tearDown mechanism since I couldn't figure out a way to have rules run after they have (which are tagged @Before).

          Show
          Ramkumar Aiyengar added a comment - There's a bit of a subtlety here btw with a re-implementation of the setUp / tearDown mechanism since I couldn't figure out a way to have rules run after they have (which are tagged @Before).
          Hide
          Ramkumar Aiyengar added a comment -

          Updated diff to resolve conflicts against latest trunk

          Show
          Ramkumar Aiyengar added a comment - Updated diff to resolve conflicts against latest trunk
          Hide
          Erick Erickson added a comment -

          Where's the patch file?

          Show
          Erick Erickson added a comment - Where's the patch file?
          Hide
          Ramkumar Aiyengar added a comment -

          The patch is in the pull request, but if you need it as a patch file attached, here you go, I have attached it now.

          (I know you mentioned putting in a patch file instead of a PR in a different issue, it's just that I personally find it easier/better to use git + pr as a workflow, and the PR integration to dev-list and mails like http://markmail.org/message/whjq4u2zkpjoh4b2 seem to suggest that it's an accepted method. That said, if a committer requests it as a patch file, I am happy to oblige..)

          Show
          Ramkumar Aiyengar added a comment - The patch is in the pull request, but if you need it as a patch file attached, here you go, I have attached it now. (I know you mentioned putting in a patch file instead of a PR in a different issue, it's just that I personally find it easier/better to use git + pr as a workflow, and the PR integration to dev-list and mails like http://markmail.org/message/whjq4u2zkpjoh4b2 seem to suggest that it's an accepted method. That said, if a committer requests it as a patch file, I am happy to oblige..)
          Hide
          Erick Erickson added a comment -

          Here's another version that removes a lot of the unused imports. I have to confess I've just skimmed it, I'll take a closer look at the base classes (where all the real action is) tomorrow.

          I guess my question for everyone is what the consensus is here? My reservation is mostly that this touches a lot of files, and we're trying to cut 5.0 next week. I'm reluctant to change this much code this close to setting the tag for 5.0. This seems more like style/cleanup than fixing outstanding problems, so I'm thinking that waiting until the label is set for 5.0 and putting this in 5.1 might be the right thing to do here, assuming there are no objections. Waiting will make resolving conflicts even more "interesting" since Alan is going to be in the test code for removing old-style solr.xml....

          That said, if some of the heavy cloud hitters (e.g. Mark Miller Alan Woodward and Noble Paul) think differently I'll be happy to commit this after the precommit problem is fixed (see below).

          The precommit target fails with:
          /test-framework/src/java/org/apache/solr/BaseDistributedSearchTestCase.java:87: warning - @ShardsFixed(num is an unknown tag.

          That's not a big deal though, I can ask someone who knows how to fix that where to look. LuceneTestCase? Anyway, if we go forward we can fix that I'm sure.

          BTW, I'm "Git challenged", and last time I tried to pull down a patch I somehow got the wrong stuff, thanks for providing it! Generally we like to have the patch rather than go to a repo so the JIRA is self-contained, we never know when a repo will go away.

          Show
          Erick Erickson added a comment - Here's another version that removes a lot of the unused imports. I have to confess I've just skimmed it, I'll take a closer look at the base classes (where all the real action is) tomorrow. I guess my question for everyone is what the consensus is here? My reservation is mostly that this touches a lot of files, and we're trying to cut 5.0 next week. I'm reluctant to change this much code this close to setting the tag for 5.0. This seems more like style/cleanup than fixing outstanding problems, so I'm thinking that waiting until the label is set for 5.0 and putting this in 5.1 might be the right thing to do here, assuming there are no objections. Waiting will make resolving conflicts even more "interesting" since Alan is going to be in the test code for removing old-style solr.xml.... That said, if some of the heavy cloud hitters (e.g. Mark Miller Alan Woodward and Noble Paul ) think differently I'll be happy to commit this after the precommit problem is fixed (see below). The precommit target fails with: /test-framework/src/java/org/apache/solr/BaseDistributedSearchTestCase.java:87: warning - @ShardsFixed(num is an unknown tag. That's not a big deal though, I can ask someone who knows how to fix that where to look. LuceneTestCase? Anyway, if we go forward we can fix that I'm sure. BTW, I'm "Git challenged", and last time I tried to pull down a patch I somehow got the wrong stuff, thanks for providing it! Generally we like to have the patch rather than go to a repo so the JIRA is self-contained, we never know when a repo will go away.
          Hide
          Mark Miller added a comment -

          It's a great issue, but a bad idea to fuss with it right before 5.0 I think. We should be focused on test hardening rather than something that might introduce instability. I know that is a bit inconvenient, but Anshum has said he will be cutting an rc very soon.

          I think it's also too painful to simply do it on trunk for now and backport later.

          We will have a release branch very soon though and then it can go in.

          Show
          Mark Miller added a comment - It's a great issue, but a bad idea to fuss with it right before 5.0 I think. We should be focused on test hardening rather than something that might introduce instability. I know that is a bit inconvenient, but Anshum has said he will be cutting an rc very soon. I think it's also too painful to simply do it on trunk for now and backport later. We will have a release branch very soon though and then it can go in.
          Hide
          Erick Erickson added a comment -

          bq: a bad idea to fuss with it right before 5.0 I think

          Yeah, that was my feeling too, nice to have confirmation...

          I'll try to keep my local copy up to date with trunk to make the eventual reconciliation smoother, we'll see how that works.

          Show
          Erick Erickson added a comment - bq: a bad idea to fuss with it right before 5.0 I think Yeah, that was my feeling too, nice to have confirmation... I'll try to keep my local copy up to date with trunk to make the eventual reconciliation smoother, we'll see how that works.
          Hide
          Ramkumar Aiyengar added a comment -

          No worries, putting this in after 5.0 is cut makes sense. Thanks for picking this up Erick!

          Show
          Ramkumar Aiyengar added a comment - No worries, putting this in after 5.0 is cut makes sense. Thanks for picking this up Erick!
          Hide
          Noble Paul added a comment -

          Erick Erickson This is long overdue and it is very annoying to have these tests all running from a single method. But as Mark mentioned , we are very close to 5.0 RC. So +1 for committing this after 5.0 branch

          Show
          Noble Paul added a comment - Erick Erickson This is long overdue and it is very annoying to have these tests all running from a single method. But as Mark mentioned , we are very close to 5.0 RC. So +1 for committing this after 5.0 branch
          Hide
          Ramkumar Aiyengar added a comment -

          Erick Erickson, have modified your patch to get rid of the pre-commit issue. One of the files was also failing compile due to missing imports, have added that as well..

          Show
          Ramkumar Aiyengar added a comment - Erick Erickson , have modified your patch to get rid of the pre-commit issue. One of the files was also failing compile due to missing imports, have added that as well..
          Hide
          Erick Erickson added a comment -

          Great, thanks! I was just about to tackle the annotation issue, now I don't have to.

          Sorry about the missing import, I noticed that a couple of days ago, it's a result of some over-zealous cmd-X on my part when I was removing unused imports.

          Should be committing tonight/tomorrow now that 5.0 has been labeled.

          Show
          Erick Erickson added a comment - Great, thanks! I was just about to tackle the annotation issue, now I don't have to. Sorry about the missing import, I noticed that a couple of days ago, it's a result of some over-zealous cmd-X on my part when I was removing unused imports. Should be committing tonight/tomorrow now that 5.0 has been labeled.
          Hide
          Erick Erickson added a comment - - edited

          Still doesn't pass precommit though, now the error is something about unbalanced </ul> in BaseDistributedSeachTestCase.ShardsRepeat.html. I may have some time to track this later this weekend...

          And, of course, as near as I can tell the generated file does not have any unbalanced <ul></ul> tags. Siiiigggghhhh,

          Show
          Erick Erickson added a comment - - edited Still doesn't pass precommit though, now the error is something about unbalanced </ul> in BaseDistributedSeachTestCase.ShardsRepeat.html. I may have some time to track this later this weekend... And, of course, as near as I can tell the generated file does not have any unbalanced <ul></ul> tags. Siiiigggghhhh,
          Hide
          Ramkumar Aiyengar added a comment - - edited

          Sorry, about that, I wasn't able to get that far in precommit because of an another issue on trunk currently due to a possibly broken checkJavadocLinks.py which is rejecting some external links.

          I fixed that, and now I see the issue. This is certainly a checkJavaDocs.py issue. It's munging and cutting the file up, and trying to validate this fragment which is one, munged, and two, cut up at a wrong place. It seems to be looking for the code between two h3 or h4's, and trying to match up ul's. It does try to add an extra ul sometimes, but in this case two ul's are needed.

          <ul><pre>public abstract&nbsp;int&nbsp;min</pre>
          <dl>
          <dt>Default:</dt>
          <dd>1</dd>
          </dl>
          </li>
          </ul>
          </li>
          </ul>
          <ul class="blockList">
          <li class="blockList"><a name="max--">
          <!--   -->
          </a>
          <ul class="blockListLast">
          <li class="blockList">
          </ul>
          
          Show
          Ramkumar Aiyengar added a comment - - edited Sorry, about that, I wasn't able to get that far in precommit because of an another issue on trunk currently due to a possibly broken checkJavadocLinks.py which is rejecting some external links. I fixed that, and now I see the issue. This is certainly a checkJavaDocs.py issue. It's munging and cutting the file up, and trying to validate this fragment which is one, munged, and two, cut up at a wrong place. It seems to be looking for the code between two h3 or h4's, and trying to match up ul's. It does try to add an extra ul sometimes, but in this case two ul's are needed. <ul><pre>public abstract&nbsp;int&nbsp;min</pre> <dl> <dt>Default:</dt> <dd>1</dd> </dl> </li> </ul> </li> </ul> <ul class="blockList"> <li class="blockList"><a name="max--"> <!-- --> </a> <ul class="blockListLast"> <li class="blockList"> </ul>
          Hide
          Ramkumar Aiyengar added a comment -

          I am not sure how to proceed with the checkJavaDocs.py issue though. Michael McCandless, any suggestions, I see you have some experience with the file. With the patch above, looks like that script fails on solr/build/docs/solr-test-framework/org/apache/solr/BaseDistributedSearchTestCase.ShardsRepeat.html though the html appears to be fine..

          Show
          Ramkumar Aiyengar added a comment - I am not sure how to proceed with the checkJavaDocs.py issue though. Michael McCandless , any suggestions, I see you have some experience with the file. With the patch above, looks like that script fails on solr/build/docs/solr-test-framework/org/apache/solr/BaseDistributedSearchTestCase.ShardsRepeat.html though the html appears to be fine..
          Hide
          Ramkumar Aiyengar added a comment -

          On further inspection, doesn't look like this HTML verification adds any value, so created LUCENE-6188 to just remove it instead.

          Show
          Ramkumar Aiyengar added a comment - On further inspection, doesn't look like this HTML verification adds any value, so created LUCENE-6188 to just remove it instead.
          Hide
          Anshum Gupta added a comment -

          This looks good from a first glance. Thanks for holding out and not pushing it into the 5.0 though .

          Show
          Anshum Gupta added a comment - This looks good from a first glance. Thanks for holding out and not pushing it into the 5.0 though .
          Hide
          Michael McCandless added a comment -

          I'm trying to dig into why the linter is upset by this patch ... but the patch fails to apply to current 5.x branch (at least 9 of the files match "patch" upset). Is there a newer version of the patch that applies? Or maybe we could commit this to a branch and I could run from there?

          Show
          Michael McCandless added a comment - I'm trying to dig into why the linter is upset by this patch ... but the patch fails to apply to current 5.x branch (at least 9 of the files match "patch" upset). Is there a newer version of the patch that applies? Or maybe we could commit this to a branch and I could run from there?
          Hide
          Ramkumar Aiyengar added a comment -

          The patch is highly perishable unfortunately I will try to bring it up, but meanwhile you could use the pull request in the first update, that should reproduce the issue..

          Show
          Ramkumar Aiyengar added a comment - The patch is highly perishable unfortunately I will try to bring it up, but meanwhile you could use the pull request in the first update, that should reproduce the issue..
          Hide
          Michael McCandless added a comment -

          Thanks Ramkumar Aiyengar I'm able to reproduce the linting bug from the original PR ...

          Show
          Michael McCandless added a comment - Thanks Ramkumar Aiyengar I'm able to reproduce the linting bug from the original PR ...
          Hide
          Michael McCandless added a comment -

          OK I committed a fix to the linter ... try again?

          Show
          Michael McCandless added a comment - OK I committed a fix to the linter ... try again?
          Hide
          Erick Erickson added a comment -

          Michael McCandless Thanks!

          As Ram says, the half-life of this patch is short. I'd been keeping my copy up to date pretty much daily until Alan's check-in for removing support for old-style XML broke the easy updates. That code had to hit the test framework pretty heavily and I pretty much expected to have to update this patch as a result. Wish I'd been disappointed

          It's on my list today/tonight unless interrupts occur or Ramkumar Aiyengar gets there first.

          Show
          Erick Erickson added a comment - Michael McCandless Thanks! As Ram says, the half-life of this patch is short. I'd been keeping my copy up to date pretty much daily until Alan's check-in for removing support for old-style XML broke the easy updates. That code had to hit the test framework pretty heavily and I pretty much expected to have to update this patch as a result. Wish I'd been disappointed It's on my list today/tonight unless interrupts occur or Ramkumar Aiyengar gets there first.
          Hide
          Erick Erickson added a comment -

          This version of the patch resolves the patching problems, compiles and precommit now passes! Many thanks to Michael McCandless.

          Ramkumar Aiyengar any comments you'd like to make on what I had to do to reconcile this feel free. I had to reconcile three files by hand:
          ExternalCollectionsTest
          AliasIntegrationTest
          BaseDistributedSearchTestCase

          In addition, a new file had to stop using doTest: RecoveryAfterSoftCommitTest

          Finally ShardRoutingTest just had some weird foo when the patch was applied, but the changes were minimal.

          Running tests now, but probably won't see the results until tomorrow morning. If they all pass and there are no objections, I hope to commit this to trunk and 5x tomorrow or over the weekend.

          Whew!

          Show
          Erick Erickson added a comment - This version of the patch resolves the patching problems, compiles and precommit now passes! Many thanks to Michael McCandless . Ramkumar Aiyengar any comments you'd like to make on what I had to do to reconcile this feel free. I had to reconcile three files by hand: ExternalCollectionsTest AliasIntegrationTest BaseDistributedSearchTestCase In addition, a new file had to stop using doTest: RecoveryAfterSoftCommitTest Finally ShardRoutingTest just had some weird foo when the patch was applied, but the changes were minimal. Running tests now, but probably won't see the results until tomorrow morning. If they all pass and there are no objections, I hope to commit this to trunk and 5x tomorrow or over the weekend. Whew!
          Hide
          Ramkumar Aiyengar added a comment -

          ExternalCollectionsTest: I think you would need to resurrect doTest now and rename it `test` with the @Test and @ShardsFixed tags – it now has two tests, and removing it will run them separately (and the second one won't run with shards setup)

          Show
          Ramkumar Aiyengar added a comment - ExternalCollectionsTest: I think you would need to resurrect doTest now and rename it `test` with the @Test and @ShardsFixed tags – it now has two tests, and removing it will run them separately (and the second one won't run with shards setup)
          Hide
          Erick Erickson added a comment -

          Ramkumar Aiyengar Thanks, I must have been on drugs when I "fixed" that file.

          I changed it and the tests run, attaching a patch and committing shortly.

          Absent this foo the tests ran successfully a couple of times so we'll see what we'll see.

          Show
          Erick Erickson added a comment - Ramkumar Aiyengar Thanks, I must have been on drugs when I "fixed" that file. I changed it and the tests run, attaching a patch and committing shortly. Absent this foo the tests ran successfully a couple of times so we'll see what we'll see.
          Hide
          Erick Erickson added a comment -

          Final patch with Ram's comments incorporated and CHANGES.txt

          Show
          Erick Erickson added a comment - Final patch with Ram's comments incorporated and CHANGES.txt
          Hide
          ASF subversion and git services added a comment -

          Commit 1654256 from Erick Erickson in branch 'dev/trunk'
          [ https://svn.apache.org/r1654256 ]

          SOLR-6902: Use JUnit rules instead of inheritance with distributed Solr tests to allow for multiple tests without the same class

          Show
          ASF subversion and git services added a comment - Commit 1654256 from Erick Erickson in branch 'dev/trunk' [ https://svn.apache.org/r1654256 ] SOLR-6902 : Use JUnit rules instead of inheritance with distributed Solr tests to allow for multiple tests without the same class
          Hide
          ASF subversion and git services added a comment -

          Commit 1654300 from Erick Erickson in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1654300 ]

          SOLR-6902: Use JUnit rules instead of inheritance with distributed Solr tests to allow for multiple tests without the same clas

          Show
          ASF subversion and git services added a comment - Commit 1654300 from Erick Erickson in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1654300 ] SOLR-6902 : Use JUnit rules instead of inheritance with distributed Solr tests to allow for multiple tests without the same clas
          Hide
          Erick Erickson added a comment -

          Thanks Ramkumar and Mike!

          Show
          Erick Erickson added a comment - Thanks Ramkumar and Mike!
          Hide
          ASF GitHub Bot added a comment -

          Github user andyetitmoves closed the pull request at:

          https://github.com/apache/lucene-solr/pull/121

          Show
          ASF GitHub Bot added a comment - Github user andyetitmoves closed the pull request at: https://github.com/apache/lucene-solr/pull/121
          Hide
          Shalin Shekhar Mangar added a comment -

          This issue is incorrectly mentioned under 6.0 in the CHANGES.txt on trunk and under the "Introduction" section on branch_5x.

          Show
          Shalin Shekhar Mangar added a comment - This issue is incorrectly mentioned under 6.0 in the CHANGES.txt on trunk and under the "Introduction" section on branch_5x.
          Hide
          ASF subversion and git services added a comment -

          Commit 1659852 from shalin@apache.org in branch 'dev/trunk'
          [ https://svn.apache.org/r1659852 ]

          SOLR-6902: Move change log entry to 5.1 section

          Show
          ASF subversion and git services added a comment - Commit 1659852 from shalin@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1659852 ] SOLR-6902 : Move change log entry to 5.1 section
          Hide
          ASF subversion and git services added a comment -

          Commit 1659853 from shalin@apache.org in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1659853 ]

          SOLR-6902: Move change log entry to 5.1 section

          Show
          ASF subversion and git services added a comment - Commit 1659853 from shalin@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1659853 ] SOLR-6902 : Move change log entry to 5.1 section
          Hide
          Timothy Potter added a comment -

          Bulk close after 5.1 release

          Show
          Timothy Potter added a comment - Bulk close after 5.1 release

            People

            • Assignee:
              Erick Erickson
              Reporter:
              Ramkumar Aiyengar
            • Votes:
              0 Vote for this issue
              Watchers:
              10 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development