Details

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.

Robert Muir
added a comment - 08/Feb/12 20:16 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?

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).

Dawid Weiss
added a comment - 08/Feb/12 20:42 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).

Uwe Schindler
added a comment - 08/Feb/12 21:13 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

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).

Dawid Weiss
added a comment - 09/Feb/12 08:15 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).

Dawid Weiss
added a comment - 09/Feb/12 08:58 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.

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).

Dawid Weiss
added a comment - 09/Feb/12 09:54 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).

Dawid Weiss
added a comment - 09/Feb/12 10:10 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).

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!)

Robert Muir
added a comment - 10/Feb/12 12:10 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!)

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?

Dawid Weiss
added a comment - 10/Feb/12 15:14 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?

Dawid Weiss
added a comment - 10/Feb/12 22:47 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.

Dawid Weiss
added a comment - 14/Feb/12 10:52 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?

Steve Rowe
added a comment - 14/Feb/12 23:53 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?

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.

Dawid Weiss
added a comment - 15/Feb/12 09:55 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.