Lucene - Core
  1. Lucene - Core
  2. LUCENE-2642

merge LuceneTestCase and LuceneTestCaseJ4

    Details

    • Type: Test Test
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.1, 4.0-ALPHA
    • Component/s: general/test
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      We added Junit4 support, but as a separate test class.

      So unfortunately, we have two separate base classes to maintain: LuceneTestCase and LuceneTestCaseJ4.
      This creates a mess and is difficult to manage.

      Instead, I propose a single base test class that works both junit3 and junit4 style.

      I modified our LuceneTestCaseJ4 in the following way:

      • the methods to run are not limited to the ones annotated with @Test, but also any void no-arg methods that start with "test", like junit3. this means you dont have to sprinkle @Test everywhere.
      • of course, @Ignore works as expected everywhere.
      • LuceneTestCaseJ4 extends TestCase so you dont have to import static Assert.* to get all the asserts.

      for most tests, no changes are required. but a few very minor things had to be changed:

      • setUp() and tearDown() must be public, not protected.
      • useless ctors must be removed, such as TestFoo(String name) { super(name); }
      • LocalizedTestCase is gone, instead of
        public class TestQueryParser extends LocalizedTestCase {
        

        it is now

        @RunWith(LuceneTestCase.LocalizedTestCaseRunner.class)
        public class TestQueryParser extends LuceneTestCase {
        
      • Same with MultiCodecTestCase: (LuceneTestCase.MultiCodecTestCaseRunner.class}

      I only did the core tests in the patch as a start, and i just made an empty LuceneTestCase extends LuceneTestCaseJ4.

      I'd like to do contrib and solr and rename this LuceneTestCaseJ4 to only a single class: LuceneTestCase.

      1. LUCENE-2642.patch
        89 kB
        Robert Muir
      2. LUCENE-2642-extendAssert.patch
        7 kB
        Uwe Schindler
      3. LUCENE-2642.patch
        150 kB
        Robert Muir
      4. LUCENE-2642-fixes.patch
        1 kB
        Uwe Schindler

        Activity

        Hide
        Robert Muir added a comment -

        patch for core tests only, all tests pass.

        Show
        Robert Muir added a comment - patch for core tests only, all tests pass.
        Hide
        Uwe Schindler added a comment - - edited

        Why not simply extend the org.junit.Assert class? This would remove use of deprecated old JUnit3 Framework completely?

        Show
        Uwe Schindler added a comment - - edited Why not simply extend the org.junit.Assert class? This would remove use of deprecated old JUnit3 Framework completely?
        Hide
        Robert Muir added a comment -

        Why not simply extend the Assert abstract class? This would remove use of deprecated old JUnit3 Framework completely?

        I would like to do this under a different issue.

        We cannot do this, until all assertEquals(float, float) are changed to use epsilons, for example.

        By extending Assert, we can catch all the places we don't use epsilons and fix them, which
        is a great improvement, but out of scope of this issue.

        Show
        Robert Muir added a comment - Why not simply extend the Assert abstract class? This would remove use of deprecated old JUnit3 Framework completely? I would like to do this under a different issue. We cannot do this, until all assertEquals(float, float) are changed to use epsilons, for example. By extending Assert, we can catch all the places we don't use epsilons and fix them, which is a great improvement, but out of scope of this issue.
        Hide
        Uwe Schindler added a comment -

        I am just afraid of extending form the old JUnit Testcase. We can simply add @Deprecated methods to asser floats without epsilon, that we can then remove.

        So extend Assert and the add missing static methods for compatibility.

        Show
        Uwe Schindler added a comment - I am just afraid of extending form the old JUnit Testcase. We can simply add @Deprecated methods to asser floats without epsilon, that we can then remove. So extend Assert and the add missing static methods for compatibility.
        Hide
        Robert Muir added a comment -

        I am just afraid of extending form the old JUnit Testcase.

        we already extend this! Have you looked at LuceneTestCase lately?

        So extend Assert and the add missing static methods for compatibility.

        Please, i would like to keep the epsilon stuff out of this issue. All tests pass the way it is now, there is no
        problem.

        We can fix epsilons in a followup issue, and then use no junit3 code at all... as I said its a great improvement, but not necessary to mix in with this change.

        By doing both at once, if somethign goes wrong, it will be more difficult to debug. Lets keep the scope under control.

        Show
        Robert Muir added a comment - I am just afraid of extending form the old JUnit Testcase. we already extend this! Have you looked at LuceneTestCase lately? So extend Assert and the add missing static methods for compatibility. Please, i would like to keep the epsilon stuff out of this issue. All tests pass the way it is now, there is no problem. We can fix epsilons in a followup issue, and then use no junit3 code at all... as I said its a great improvement, but not necessary to mix in with this change. By doing both at once, if somethign goes wrong, it will be more difficult to debug. Lets keep the scope under control.
        Hide
        Uwe Schindler added a comment -

        Here the patch, so LuceneTestCaseJ4 only extends Assert without importing extra crap. The double/float API of old Junit3 is emulated using static overrides. After applying patch do a ant clean, else you will get lining errors.

        Show
        Uwe Schindler added a comment - Here the patch, so LuceneTestCaseJ4 only extends Assert without importing extra crap. The double/float API of old Junit3 is emulated using static overrides. After applying patch do a ant clean, else you will get lining errors.
        Hide
        Robert Muir added a comment -

        updated patch, with all of lucene/solr and including uwe's stuff.

        all tests pass.

        Show
        Robert Muir added a comment - updated patch, with all of lucene/solr and including uwe's stuff. all tests pass.
        Hide
        Michael McCandless added a comment -

        This is great Robert! Patch works for me (except for bizarre hang in Solr's TestSolrCoreProperties, apparently only on my machine, that's pre-existing).

        Show
        Michael McCandless added a comment - This is great Robert! Patch works for me (except for bizarre hang in Solr's TestSolrCoreProperties, apparently only on my machine, that's pre-existing).
        Hide
        Uwe Schindler added a comment -

        Looks good, this is a really good step forwards. We can write old-style tests, butuse JUnit4 and can optionally add the @BeforeClass and so on

        Show
        Uwe Schindler added a comment - Looks good, this is a really good step forwards. We can write old-style tests, butuse JUnit4 and can optionally add the @BeforeClass and so on
        Hide
        Robert Muir added a comment -

        We can write old-style tests, butuse JUnit4 and can optionally add the @BeforeClass and so on

        Yeah i've never understood why Junit4 requires all these static imports and annotations... i just care about @BeforeClass!

        Show
        Robert Muir added a comment - We can write old-style tests, butuse JUnit4 and can optionally add the @BeforeClass and so on Yeah i've never understood why Junit4 requires all these static imports and annotations... i just care about @BeforeClass!
        Hide
        Uwe Schindler added a comment -

        Some small fixes in reflection inspection:

        • exclude static and abstract methods
        • check native return type
        Show
        Uwe Schindler added a comment - Some small fixes in reflection inspection: exclude static and abstract methods check native return type
        Hide
        Robert Muir added a comment -

        thanks Uwe, i can merge this into the 3x backport too.

        Show
        Robert Muir added a comment - thanks Uwe, i can merge this into the 3x backport too.
        Hide
        Robert Muir added a comment -

        OK, i didnt merge the reflection fixes yet, but i backported the patch to 3x.

        Committed revision 996611, 996630 (3x).

        Will mark the issue resolved when Uwe is out of reflection hell

        Show
        Robert Muir added a comment - OK, i didnt merge the reflection fixes yet, but i backported the patch to 3x. Committed revision 996611, 996630 (3x). Will mark the issue resolved when Uwe is out of reflection hell
        Hide
        Robert Muir added a comment -

        OK i merged back all of Uwe's improvements. Thanks for the help Uwe.

        I think now in future issues we can clean up and improve this test case a lot.
        I felt discouraged from doing so with the previous duplication...

        Show
        Robert Muir added a comment - OK i merged back all of Uwe's improvements. Thanks for the help Uwe. I think now in future issues we can clean up and improve this test case a lot. I felt discouraged from doing so with the previous duplication...
        Hide
        Grant Ingersoll added a comment -

        Bulk close for 3.1

        Show
        Grant Ingersoll added a comment - Bulk close for 3.1

          People

          • Assignee:
            Unassigned
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development