Lucene - Core
  1. Lucene - Core
  2. LUCENE-3762

Upgrade JUnit to 4.10, refactor state-machine of detecting setUp/tearDown call chaining.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Trivial Trivial
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.6, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      Both Lucene and Solr use JUnit 4.7. I suggest we move forward and upgrade to JUnit 4.10 which provides several infrastructural changes (serializable Description objects, class-level rules, various tweaks). JUnit 4.10 also changes (or fixes, depends how you look at it) the order in which @Before/@After hooks and @Rules are applied. This makes the old state-machine in LuceneTestCase fail (because the order is changed).

      I rewrote the state machine and used a different, I think simpler, although Uwe may disagree , mechanism in which the hook methods setUp/ tearDown are still there, but they are empty at the top level and serve only to detect whether subclasses chain super.setUp/tearDown properly (if they override anything).

      In the long term, I would love to just get rid of public setup/teardown methods and make them private (so that they cannot be overriden or even seen by subclasses) but this will require changes to the runner itself.

      1. LUCENE-3762-backport.patch
        49 kB
        Dawid Weiss
      2. LUCENE-3762.patch
        1.08 MB
        Dawid Weiss
      3. LUCENE-3762.patch
        35 kB
        Dawid Weiss
      4. LUCENE-3762.patch
        42 kB
        Dawid Weiss

        Activity

        Hide
        Dawid Weiss added a comment -

        Reopening – method names in reproduction report.

        Show
        Dawid Weiss added a comment - Reopening – method names in reproduction report.
        Hide
        Dawid Weiss added a comment -

        Good catch, Steve – yes, I might have introduced it as part of the refactoring. JUnit has deprecated MethodRule in favor of TestRule and the latter one doesn't come with a FrameworkMethod... It's weird.

        I will reopen this issue and apply a fix.

        Show
        Dawid Weiss added a comment - Good catch, Steve – yes, I might have introduced it as part of the refactoring. JUnit has deprecated MethodRule in favor of TestRule and the latter one doesn't come with a FrameworkMethod... It's weird. I will reopen this issue and apply a fix.
        Hide
        Steve Rowe added a comment -

        Dawid, today I've seen the following test reproduction message (from Maven, but running Lucene/Solr tests under Maven has caused this before):

        NOTE: reproduce with: ant test -Dtestcase=UIMABaseAnalyzerTest -Dtestmethod=testRandomStrings(org.apache.lucene.analysis.uima.UIMABaseAnalyzerTest) -Dtests.seed=2be0c24a1df9b25e:-42f203968285c6ed:5f8c85cdbae32724 -Dargs="-Dfile.encoding=Cp1252"
        

        That is, the parenthetical class name after the method in the -Dtestmethod=... string doesn't work - you have to strip this out in order to actually use the given cmdline.

        Am I right in assuming that LUCENE-3762 is the source of this behavior change?

        Show
        Steve Rowe added a comment - Dawid, today I've seen the following test reproduction message (from Maven, but running Lucene/Solr tests under Maven has caused this before): NOTE: reproduce with: ant test -Dtestcase=UIMABaseAnalyzerTest -Dtestmethod=testRandomStrings(org.apache.lucene.analysis.uima.UIMABaseAnalyzerTest) -Dtests.seed=2be0c24a1df9b25e:-42f203968285c6ed:5f8c85cdbae32724 -Dargs="-Dfile.encoding=Cp1252" That is, the parenthetical class name after the method in the -Dtestmethod=... string doesn't work - you have to strip this out in order to actually use the given cmdline. Am I right in assuming that LUCENE-3762 is the source of this behavior change?
        Hide
        Robert Muir added a comment -

        +1 for 3.x too.

        Show
        Robert Muir added a comment - +1 for 3.x too.
        Hide
        Dawid Weiss added a comment -

        Backport patch. This removes state machine from backport/LuceneTestCase so that backport tests can pass.

        Show
        Dawid Weiss added a comment - Backport patch. This removes state machine from backport/LuceneTestCase so that backport tests can pass.
        Hide
        Dawid Weiss added a comment -

        I've committed to the trunk and I have a backport of this but I started to wonder if it's a good idea to apply it on 3x – this may cause issues with backport testing, if not anything else. Thoughts?

        Show
        Dawid Weiss added a comment - I've committed to the trunk and I have a backport of this but I started to wonder if it's a good idea to apply it on 3x – this may cause issues with backport testing, if not anything else. Thoughts?
        Hide
        Dawid Weiss added a comment -

        I plan to commit this in on Monday (so that I'm at the computer in case something breaks) unless there are objections.

        Show
        Dawid Weiss added a comment - I plan to commit this in on Monday (so that I'm at the computer in case something breaks) unless there are objections.
        Hide
        Dawid Weiss added a comment -

        Does this mean if we assumeTrue(message, ...) inside a setUp() that we will now actually see the message?

        So, the answer to this is yes, you will.

        Show
        Dawid Weiss added a comment - Does this mean if we assumeTrue(message, ...) inside a setUp() that we will now actually see the message? So, the answer to this is yes, you will.
        Hide
        Dawid Weiss added a comment -

        I checked assume/fail/error in combination with all the possible execution points. LuceneTestCase rule wrapper emits (as could be predicted) the right note for everything but:

        • static initializers,
        • @BeforeClass
        • initializers (constructor)
        • @AfterClass

        These cases are handled outside of @Rule scope and should be handled by JUnit and propagated as failures to report listeners.

        Show
        Dawid Weiss added a comment - I checked assume/fail/error in combination with all the possible execution points. LuceneTestCase rule wrapper emits (as could be predicted) the right note for everything but: static initializers, @BeforeClass initializers (constructor) @AfterClass These cases are handled outside of @Rule scope and should be handled by JUnit and propagated as failures to report listeners.
        Hide
        Dawid Weiss added a comment -

        Updated patch with tests of what's emitted and when.

        Show
        Dawid Weiss added a comment - Updated patch with tests of what's emitted and when.
        Hide
        Dawid Weiss added a comment -

        This already works on the branch I think, but I will re-check. I advanced junit4 branch and integrated junit4 parallel balanced runner instead of the default ANT's junit and previous set of macros. The code for this patch lives under LUCENE-3762.

        I'll look for corner cases tonight and commit this in. Alternatively we could set up a parallel jenkins build and commit this on a branch to see if everything is all right?

        Show
        Dawid Weiss added a comment - This already works on the branch I think, but I will re-check. I advanced junit4 branch and integrated junit4 parallel balanced runner instead of the default ANT's junit and previous set of macros. The code for this patch lives under LUCENE-3762 . I'll look for corner cases tonight and commit this in. Alternatively we could set up a parallel jenkins build and commit this on a branch to see if everything is all right?
        Hide
        Robert Muir added a comment -

        about the assumes() etc from setup, in general exceptions/assumes, I think we would like them to be treated the same whether they happen in the actual test method body or setup or teardown?

        So like today, the buggy behavior is that if an assumption fails from a test method itself, we get a message to stderr:
        NOTE: Assume failed in 'testFoo' (ignored): Some message explaining why this is ok!

        But, if it fails in setup, we get no message at all!

        The reason I think it was hard was because of how the TestWatcher didn't get an event if it failed in setup, so we didnt have a clean way to
        do this... but maybe its something we can fix in junit 4.8+ (doesn't need to be done on this issue!)

        Show
        Robert Muir added a comment - about the assumes() etc from setup, in general exceptions/assumes, I think we would like them to be treated the same whether they happen in the actual test method body or setup or teardown? So like today, the buggy behavior is that if an assumption fails from a test method itself, we get a message to stderr: NOTE: Assume failed in 'testFoo' (ignored): Some message explaining why this is ok! But, if it fails in setup, we get no message at all! The reason I think it was hard was because of how the TestWatcher didn't get an event if it failed in setup, so we didnt have a clean way to do this... but maybe its something we can fix in junit 4.8+ (doesn't need to be done on this issue!)
        Hide
        Dawid Weiss added a comment -

        All tests pass for me with this patch. I didn't attach a binary patch this time, the patched version is at github:
        https://github.com/dweiss/lucene_solr (junit4 branch).

        Show
        Dawid Weiss added a comment - All tests pass for me with this patch. I didn't attach a binary patch this time, the patched version is at github: https://github.com/dweiss/lucene_solr (junit4 branch).
        Hide
        Dawid Weiss added a comment -

        Didn't run the tests yet, doing it in the background.

        Show
        Dawid Weiss added a comment - Didn't run the tests yet, doing it in the background.
        Hide
        Dawid Weiss added a comment -

        If we're changing JUnit perhaps it's worth upgrading the infrastructure a bit to make things cleaner. I refactored all the hooks into a ruleset so that their nesting order is explicit:

          @Rule
          public final TestRule ruleChain = RuleChain
            .outerRule(new RememberThreadRule())
            .around(new TestResultInterceptorRule())
            .around(new InternalSetupTeardownRule())
            .around(new SubclassSetupTeardownRule());
        

        So, subclasses (setup/teardown) run inside, surrounded by internal cleanups, surrounded by test result tracker, surrounded by current thread remembering. I also removed _TestIgnoredException in favor of a subclass of AssumptionIgnoredException - this removes some conditional checks and unwinding code.

        I added some tests to detect the expected behavior of LTC (what Robert mentioned); I would feel great if we check that all the expectations are covered before we commit this in – if you can post a simple class along with: "this should result in this and that" I'll update the tests. There are examples of such expectations in the patch (static classes and tests inside TestSetupTeardownMethods class).

        Show
        Dawid Weiss added a comment - If we're changing JUnit perhaps it's worth upgrading the infrastructure a bit to make things cleaner. I refactored all the hooks into a ruleset so that their nesting order is explicit: @Rule public final TestRule ruleChain = RuleChain .outerRule( new RememberThreadRule()) .around( new TestResultInterceptorRule()) .around( new InternalSetupTeardownRule()) .around( new SubclassSetupTeardownRule()); So, subclasses (setup/teardown) run inside, surrounded by internal cleanups, surrounded by test result tracker, surrounded by current thread remembering. I also removed _TestIgnoredException in favor of a subclass of AssumptionIgnoredException - this removes some conditional checks and unwinding code. I added some tests to detect the expected behavior of LTC (what Robert mentioned); I would feel great if we check that all the expectations are covered before we commit this in – if you can post a simple class along with: "this should result in this and that" I'll update the tests. There are examples of such expectations in the patch (static classes and tests inside TestSetupTeardownMethods class).
        Hide
        Dawid Weiss added a comment -

        An updated patch with more serious refactorings of LTC.

        Show
        Dawid Weiss added a comment - An updated patch with more serious refactorings of LTC.
        Hide
        Dawid Weiss added a comment -

        There are functional differences between TestWatcher (before) and TestWatchman (current) – assumptions are no longer propagated as failures and the code in LTC.intercept() no longer applies:

            protected void failed(Throwable e, Description description) {
              // org.junit.internal.AssumptionViolatedException in older releases
              // org.junit.Assume.AssumptionViolatedException in recent ones
              if (e.getClass().getName().endsWith("AssumptionViolatedException")) {
                if (e.getCause() instanceof _TestIgnoredException)
        

        I'll write tests to cover these and rewrite the interceptor explicitly as a @Rule so that we don't rely on JUnit's implementation with regard as to what is considered what.

        Show
        Dawid Weiss added a comment - There are functional differences between TestWatcher (before) and TestWatchman (current) – assumptions are no longer propagated as failures and the code in LTC.intercept() no longer applies: protected void failed(Throwable e, Description description) { // org.junit.internal.AssumptionViolatedException in older releases // org.junit.Assume.AssumptionViolatedException in recent ones if (e.getClass().getName().endsWith( "AssumptionViolatedException" )) { if (e.getCause() instanceof _TestIgnoredException) I'll write tests to cover these and rewrite the interceptor explicitly as a @Rule so that we don't rely on JUnit's implementation with regard as to what is considered what.
        Hide
        Dawid Weiss added a comment -

        Just to clarify: I don't think the state machine was wrong, it just made assumptions that don't hold in JUnit4.10 (the order of calls). I decided to remove it because I think there is a cleaner way of ensuring setup/teardown was properly chained if overriden.

        I'll commit this one in to the trunk and we'll see if it breaks anything on Jenkins (I don't think it should, it doesn't locally).

        Show
        Dawid Weiss added a comment - Just to clarify: I don't think the state machine was wrong, it just made assumptions that don't hold in JUnit4.10 (the order of calls). I decided to remove it because I think there is a cleaner way of ensuring setup/teardown was properly chained if overriden. I'll commit this one in to the trunk and we'll see if it breaks anything on Jenkins (I don't think it should, it doesn't locally).
        Hide
        Uwe Schindler added a comment -

        I rewrote the state machine and used a different, I think simpler, although Uwe may disagree

        No, no, the state machine was Robert's work I only helped on improvements

        +1

        Show
        Uwe Schindler added a comment - I rewrote the state machine and used a different, I think simpler, although Uwe may disagree No, no, the state machine was Robert's work I only helped on improvements +1
        Hide
        Robert Muir added a comment -

        its definitely a separate issue... just curiousity!

        Show
        Robert Muir added a comment - its definitely a separate issue... just curiousity!
        Hide
        Dawid Weiss added a comment -

        Yes, it would be possible. Either at the runner level (worse) or by using a custom rule and not a TestWatcher subclass. What TestWatcher does is this:

        			public void evaluate() throws Throwable {
        				starting(description);
        				try {
        					base.evaluate();
        					succeeded(description);
        				} catch (AssumptionViolatedException e) {
        					throw e;
        				} catch (Throwable t) {
        					failed(t, description);
        					throw t;
        				} finally {
        					finished(description);
        				}
        			}
        

        so it effectively skips assumption-failed tests and they're not passed to LuceneTestCase. Doable, but I think worth a separate issue?

        Show
        Dawid Weiss added a comment - Yes, it would be possible. Either at the runner level (worse) or by using a custom rule and not a TestWatcher subclass. What TestWatcher does is this: public void evaluate() throws Throwable { starting(description); try { base.evaluate(); succeeded(description); } catch (AssumptionViolatedException e) { throw e; } catch (Throwable t) { failed(t, description); throw t; } finally { finished(description); } } so it effectively skips assumption-failed tests and they're not passed to LuceneTestCase. Doable, but I think worth a separate issue?
        Hide
        Dawid Weiss added a comment -

        I just committed a test case that verifies this to GitHub. The message gets propagated properly to RunListeners (as testAssumptionFailure). I don't see anything in Lucene's test output when I run a test with such an assumption, but I guess it has to be possible (if nothing else, then by capturing that with a custom RunListener).

        Show
        Dawid Weiss added a comment - I just committed a test case that verifies this to GitHub. The message gets propagated properly to RunListeners (as testAssumptionFailure). I don't see anything in Lucene's test output when I run a test with such an assumption, but I guess it has to be possible (if nothing else, then by capturing that with a custom RunListener).
        Hide
        Robert Muir added a comment -

        looks great, lets upgrade!

        JUnit 4.10 also changes (or fixes, depends how you look at it) the order in which @Before/@After hooks and @Rules are applied.

        Does this mean if we assumeTrue(message, ...) inside a setUp() that we will now actually see the message?

        Show
        Robert Muir added a comment - looks great, lets upgrade! JUnit 4.10 also changes (or fixes, depends how you look at it) the order in which @Before/@After hooks and @Rules are applied. Does this mean if we assumeTrue(message, ...) inside a setUp() that we will now actually see the message?
        Hide
        Dawid Weiss added a comment -

        git patch (binary data updates) corresponding to this:

        https://github.com/dweiss/lucene_solr/commit/b0608e21abcaebb7d39c769689e3c0e987b741f9

        All tests pass for me, but I would love if somebody else also tried (on a non-windows machine?).

        Show
        Dawid Weiss added a comment - git patch (binary data updates) corresponding to this: https://github.com/dweiss/lucene_solr/commit/b0608e21abcaebb7d39c769689e3c0e987b741f9 All tests pass for me, but I would love if somebody else also tried (on a non-windows machine?).

          People

          • Assignee:
            Dawid Weiss
            Reporter:
            Dawid Weiss
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development