Before/AfterClass invoked at wrong time for abstract classes in suites

[seanf]

The invocation order with JUnit's standard runner:

BeforeClass: AbstractTest
BeforeClass: PlainTestA
Running:     PlainTestA.test1
Running:     PlainTestA.test2
AfterClass:  PlainTestA
AfterClass:  AbstractTest
BeforeClass: AbstractTest
BeforeClass: PlainTestB
Running:     PlainTestB.test1
Running:     PlainTestB.test2
AfterClass:  PlainTestB
AfterClass:  AbstractTest

Whereas with DataProviderRunner:

BeforeClass: AbstractTest
BeforeClass: DataProviderTestA
BeforeClass: AbstractTest
BeforeClass: DataProviderTestB
Running:     DataProviderTestA.test1
Running:     DataProviderTestA.test2
AfterClass:  DataProviderTestA
AfterClass:  AbstractTest
Running:     DataProviderTestB.test1
Running:     DataProviderTestB.test2
AfterClass:  DataProviderTestB
AfterClass:  AbstractTest

If AbstractTest manipulates static fields within its own class (which it probably will), the second invocation of AbstractTest#beforeClassAbstractTest may overwrite fields which were just set by the first invocation, but not yet used by DataProviderTestA.test1. Also, the first invocation of AbstractTest#afterClassAbstractTest may clear out the fields which are about to be used by DataProviderTestB.test1.

Note: This problem also occurs with IntelliJ's implicitly generated suites (like run all unit tests matching .*Test$).

Possibly related: https://github.com/TNG/junit-dataprovider/issues/49

[seanf]

Demonstration project: https://gist.github.com/seanf/8c2fe3d7e6507befd6bb

[aaschmid]

Hi @seanf, I totally agree and the behavior is currently expected. I just haven't thought about the AbstractClass initialisations, though.

The actual problem is that you could either have the described some time incorrect behavior above or @BeforeClass will not work at all using junit-dataprovider because of the very strange, inflexible and IMHO broken implementation of JUnit. I can explain you the details if you are interested. I will also re-think about a solution but as far as I know there is no straight forward way if you want to access static fields initialized by @BeforeClass within your @DataProvider :-(

So what would you prefer?

[seanf]

Would it perhaps be possible to invoke the BeforeClass for AbstractClass only once per suite, and the AfterClass only once? I'm still a little unclear on how the different classes, which are separately annotated RunWith(DataProviderRunner.class) can be overlapping. I suppose that's part of the brokenness of JUnit suites?

I notice that even with the default runner, AbstractClass's static methods are not invoked just once, but once per subclass, which was a bit surprising. But perhaps that could be changed, if it helps at all.

I would be interested to hear the details, if you don't mind.

In my use case, I don't think my data providers actually depend on the parent class. The parent class sets up the environment, the data providers just create test objects. But of course that's just one use case.

On 10 November 2015 at 19:50, Andreas Schmid notifications@github.com wrote:

Hi @seanf https://github.com/seanf, I totally agree and the behavior is currently expected. I just haven't thought about the AbstractClass initialisations, though.

The actual problem is that you could either have the described some time incorrect behavior above or @BeforeClass will not work at all using junit-dataprovider because of the very strange, inflexible and IMHO broken implementation of JUnit. I can explain you the details if you are interested. I will also re-think about a solution but as far as I know there is no straight forward way if you want to access static fields initialized by @BeforeClass within your @DataProvider :-(

So what would you prefer?

— Reply to this email directly or view it on GitHub https://github.com/TNG/junit-dataprovider/issues/64#issuecomment-155372908 .

[aaschmid]

Would it perhaps be possible to invoke the BeforeClass for AbstractClass only once per suite, and the AfterClass only once? I'm still a little unclear on how the different classes, which are separately annotated RunWith(DataProviderRunner.class) can be overlapping. I suppose that's part of the brokenness of JUnit suites?

Nice idea, this would require some static remember function but possible.

I notice that even with the default runner, AbstractClass's static methods are not invoked just once, but once per subclass, which was a bit surprising. But perhaps that could be changed, if it helps at all.

Hm ... good to know so this is the default behavior and junit-dataprovider is just different in the order because all the dataproviders has to be evaluated at the startup to determine the amount of test methods to be executed ...

I would be interested to hear the details, if you don't mind.

Let's see if I get it all together. The BlockJUnit4Runner internally wraps together a list of Statements to be executed which means the inner on is an "execute testmethod". This gets wrapped into a @Before before and @After after the execution of the inner statement. This gets combined with other "test methods" and is finally wrapped with the class before and after (I missed out rules but they work quite similar). This is fine an work out for the dataprovider as well but if you now want to access statically initialized fields in your static dataprovider (which is obvious IMHO) than the @BeforeClass must be executed much earlier and the Statements can only contain exceptions thrown by much earlier executed befores. The reason for this early invocation is that JUnit (I acutally don't remember why or which parent classes forces you to) initializes and validates all test methods within the Runners constructor. Otherwise the test methods are not reported at all. And as the junit-dataprovider creates test methods upon the result of the referenced dataproviders and as the Runner is initialized at the start of JUnit ...

Does that help? Otherwise don't hesitate to ask. I am looking forward to the results of https://github.com/junit-team/junit-lambda. I know one person of the team who has the same problems with his junit extension ...

Another question: Is it urgent? Because otherwise I would try to focus on a complete new implementation of DataProviderRunner without any dependency to BlockJUnit4Runner and maybe ParentRunner in a version 2.0 or wait for the first results of above mentioned project junit-lambda.

[aaschmid]

After looking deeper into JUnit internals, the dilemma unfortunately stays the same even if we implement the DataProviderRunner completely independent from BlockJUnit4Runner. Because we first have to determine the test cases which depends on the execution result of @UseDataProvider which might depend on @BeforeClass methods. And as JUnit first requires all the test Descriptions before running a single test, this means that all before mentioned methods has to be executed for all test class and cases before running the first test case ...

This means either we run all @BeforeClass beforehand (even if we could avoid running it multiple times for abstract base classes) or with the normal JUnit behavior. First means, @BeforeClass results are available within @UseDataProvider methods, seconds @UseDataProvider must not use the @BeforeClass because they are not available at the time of test method calculation. There is another way of just providing the test unenhanced methods when the Descriptions are requested the first time, though neither the test count nor the assignment of the tests to its parent (= class) will work out. The tests instead are show as "Unrooted tests" which is very ugly and unintuitive.

This is unfortunately caused due to the lack of extensibility of JUnit ... :-(

[seanf]

Thanks, I think I got some of that! Basically, junit-lambda seems to be our only hope! JUnit is really due for an overhaul.

It's not really urgent for me, more of a nuisance. It means I can't run "all unit tests" in the IDE (IntelliJ apparently creates a suite when using patterns), but apparently maven-surefire doesn't create a suite, because the DataProvider tests seem to be working in Maven.

If I'm understanding correctly, part of the problem is that to create the Description, you need to know which tests there will be (and thus how many). And some DataProviders can't generate their data until the BeforeClass methods have run. (I really get the impression JUnit doesn't really expect extra tests to be generated dynamically.)

But perhaps we don't need to run the actual DataProviders during Runner.getDescription(). What if DataProviders had two stages - stage 1 (invoked by Runner.getDescription()) just generates a list of tests for the Description (can't depend on BeforeClass, and may or may not actually generate the data), and stage 2 (invoked by Runner.run(), between Before- and AfterClass) actually generates the data?

I could even suggest something unpleasant like returning an empty Description and adding the children later on! Who knows how JUnit or IDEs might react if the number of tests were to change? Could be an interesting experiment though. I have a feeling some Runners might do this, because I think I have seen the number of tests changing in IntelliJ during execution. That might have been with TestNG though.

On 12 November 2015 at 07:38, Andreas Schmid notifications@github.com wrote:

After looking deeper into JUnit internals, the dilemma unfortunately stays the same even if we implement the DataProviderRunner completely independent from BlockJUnit4Runner. Because we first have to determine the test cases which depends on the execution result of @UseDataProvider which might depend on @BeforeClass methods. And as JUnit first requires all the test Descriptions before running a single test, this means that all before mentioned methods has to be executed for all test class and cases before running the first test case ...

This means either we run all @BeforeClass beforehand (even if we could avoid running it multiple times for abstract base classes) or with the normal JUnit behavior. First means, @BeforeClass results are available within @UseDataProvider methods, seconds @UseDataProvider must not use the @BeforeClass because they are not available at the time of test method calculation.

This is unfortunately caused due to the lack of extensibility of JUnit ... :-(

— Reply to this email directly or view it on GitHub https://github.com/TNG/junit-dataprovider/issues/64#issuecomment-155917854 .

[aaschmid]

It's not really urgent for me, more of a nuisance. It means I can't run "all unit tests" in the IDE (IntelliJ apparently creates a suite when using patterns), but apparently maven-surefire doesn't create a suite, because the DataProvider tests seem to be working in Maven.

Ok, good to know.

If I'm understanding correctly, part of the problem is that to create the Description, you need to know which tests there will be (and thus how many). And some DataProviders can't generate their data until the BeforeClass methods have run. (I really get the impression JUnit doesn't really expect extra tests to be generated dynamically.)

At least I understand it like that, yes! IMHO if you look at JUnit Theories they have the same problem there. And that could be the reason for just showing a single test method / result for every test which contains multiple theories!!! (Btw. one point why I don't like theories at all, it's not reproducible easily which one failed ...)

But perhaps we don't need to run the actual DataProviders during Runner.getDescription(). What if DataProviders had two stages - stage 1 (invoked by Runner.getDescription()) just generates a list of tests for the Description (can't depend on BeforeClass, and may or may not actually generate the data), and stage 2 (invoked by Runner.run(), between Before- and AfterClass) actually generates the data?

That is actually what I meant with "There is another way of just providing the test unenhanced methods when the Descriptions are requested the first time, ...". This will lead to the correct behavior but to incorrect reports in a way that test will no longer be part of their test class, instead will be show as "Unrooted tests" (tested in Eclipse).

[aaschmid]

What does other think about it?

[janschaefer]

We had no problem so far with the BeforeClass methods. Changing the order of execution now might break existing test code.

[DanielDressler]

I would suggest not to evaluate the BeforeClass methods at all for the computation of the test cases. We do have tests that expect to be run in the usual bracket of BeforeClass/AfterClass that fail if a DataProviderRunner is used and picks up multiple test classes and interleaves these brackets.

In addition, we've run into the problem that DataProviderRunner's handling of BeforeClass does not play nicely with JUnit-Filters: Our test classes are annotated with Categories (say, Unit, GUI, Integration), and only one group is executed from maven at a time. The corresponding Filter removes the tests after the DataProvider has scanned the test methods (and executed the BeforeClass methods), but once the test classes are filtered out, the AfterClass is never run! So any cleanup in the AfterClass is not executed in skipped test classes, even though the BeforeClass methods were run for all classes.

If functionality like the current BeforeClass methods running before the DataProvider methods is really needed, I would suggest something like @BeforeDataProvider annotated methods in the test classes. At least, one could document that these methods are all executed as a block. (And in a further step deduplicate the executes in class hierarchies?) On the other hand, such an annotation might not be needed, given that static initializers would do something very similar. Overall, I'm not convinced of such a new annotation, or at least, my suggestion doesn't convince me. ;)