Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global JUnit rules revisited #1219

Open
tburny opened this issue Nov 17, 2015 · 13 comments
Open

Global JUnit rules revisited #1219

tburny opened this issue Nov 17, 2015 · 13 comments

Comments

@tburny
Copy link

tburny commented Nov 17, 2015

A word in advance: I never have written such an extensive request in such a major project before, so please bear with me. :) My goal was to provide a comprehensive background as well as concrete suggestions for a solution. I hope that does not seem impolite :)

Due to the topic dating back as far as 2009, I tried to sum up previous discussions, so this issue has become a bit lengthy (sorry :/).

TLDR:

  • some users in the JUnit community would love to have global rules
  • I coin these global rules Guards, to differentiate from normal TestRules, RunListeners and Runners which mostly are scoped to test class level only
  • Guards should be deactivated by default, with a global opt-in so they don't have to be declared inside every test class.
  • Guards should not cause too much friction with existing code or tools (e.g. Maven Surefire)
  • I would like to contribute time and code to solve the problem (as a prototype at first, then as a real solution)
  • Your feedback is very much appreciated :)

Problem setting

At our company we want to run a set of global checks against each test to ensure it complies with company standards. If not, the according tests should fail.

Some actual use cases are:

  • nothing is printed to standard error
  • our dependency injection container has been shut down properly in @After(Class)#tearDown()).

There are more cases I can name on demand, but I wanted to keep the list short. All these cases have in common that they are some sort of global TestRule. In some cases it's not that obvious that a rule has been violated or someone simply forgot a line of code(nobody is perfect).

History

"Global rules" have been initially requested in 2009 on the JUnit mailing list, which resulted in #69 Global rules. This discussion then has been continued in #444 JUnit should have an exhaustive listener framework in 2012. I will pick up some suggestions from these issues far further below. Besides the discussion of other use cases, @BenRomberg has been making some important arguments for having such a feature.

Some of the main arguments against global rules have been

  • integration with other tools that use their custom runners, like Maven or other IDEs
  • Tests should not fail due to some "obscure" magic (package level annotations, junit.properties).
  • The same could be achieved with: TestListener, Runner, ...

However it seems a part of the community is asking for a long time. There has been a lot of discussion and I guess that JUnit also evolved much more in the past 3 years (hence "revisited").

Tried approaches

We tried to solve the "no standard error output" and the "container is always destroyed problem". These are basically additional assertions/rules after the tests in a test class are run.

  • For the first, we tried using a JUnit RunListener configured into Maven Surefire. However the RunListener is removed from the test run if an exception (such an AssertionError thrown by Assert.* methods) is thrown. My colleague @globalworming asked on Stackoverflow (suggesting using a custom Runner) and the JUnit mailing list, however there was no global solution. As stated by @dsaff in JUnit should have an exhaustive listener framework #444 RunListeners are read-only and I totally agree this should stay the way it is.
  • Using a custom runner would not work with Parameterized tests(which also requires @RunWith(Parameterized.class)) and adding a rule (a set of rules) to every single class is not an option.
  • Using a "company TestRule" which delegates to all other rules we need would still require changing every test class.

Suggestion

With hundreds of test classes in mind, changing every single one of them is no viable option.

To distinguish from existing terms (such as runner, listener or rule) which are declared in classes, I would like to introduce the term Guard. Here, imagine a historic city with solid high walls and one or more guards standing in front. These are checking on people (tests) going in(@Before) and out(@After). Any people which fail their criteria will be put to jail(AssertionError).

By default there are no guards in front of the city and everyone is allowed in and out as they please (current JUnit), although there might be laws in the city (->TestRule) for certain groups of people (test classes). You can then opt-in to place Guards in front of your city gates.

Some folk tend to come to the city and, as they leave, they are shouting mean things. This is a very polite city, so we tell the guards to put these visitors into jail for their offensive behaviour. That is, we ensure there is no output on standard error. (I can also provide a more comprehensive example, but I left it out to keep things short)

My intention using this analogy was to have an abstract, extensible story for this problem. This takes the focus away from the details towards the main problem.

How to implement this?

I'm not an JUnit expert as many of you are, so I would be glad for ideas and feedback especially on this part.

Before opening this issue, I already tried taking a look at JUnit's and also Maven Surefire's source code to evaluate some options. The goal is to create a global opt-in based approach to Guards which doesn't interfere with existing code, such as test runners + annotations (e.g. parameterized tests). The solution should be resilient to human failure, i.e. it should be possible to declare it in one place instead of for every test class.

RunListeners can already be declared in the JUnitCore. As far as I know, this is JUnits common entry point for all applications, such as IDE's, build systems, command line, etc. Adding guards at this point would allow seamless integration with existing test runners (e.g. @RunWith(Parameterized.class)) and leaving existing test code unchanged.

Confguring Guards

A common question in the past was how Guards should be configured. I could think of the following options

  1. Adding a method addGuard(Guard) to JUnitCore. I think this should always be possible, as external applications (e.g. an IDE, Command line tools or Maven Surefire) can integrate themself here (i.e. adding a Guard to the Maven build via Surefire's configuration)
  2. Having a junit.properties file. However I fear that it takes away much of the flexibility(extensibility) available in normal source code. Also this is error prone to renaming tests/packages and simply not inside the code, which is the way users are used to.
  3. Automatic discovery of configured guards using (package) annotations and normal source code. Renaming things inside an IDE would also integrate with the according changes in the configuration automatically. Also exclusions can be managed on source code vs properties file level.

(Note: The basic ideas of 2. and 3. were already mentioned in #69 and #444) Any of these options would allow leaving pre-Guard code in user projects as is (no change required whatsoever).

From my point of view options 1) and 3) are the best, with 3) extending 1) with auto-config.

To keep the initial post shorter, I will put my suggestion for a package-based configuration into a separate comment below.

Inside a Guard, it would be nice to reuse the normal JUnit infrastructure using rules and class rules:

public class MyGuard {
  // Checks before/after each test class
  @ClassRule
  public final NoContainerExistsRule noContainerExistsRule = new ContainerExistsRule();

  // Checks before/after each method
  @Rule
  public final NoSystemErrorRule noSystemErrorRule = new NoSystemErrorRule();

  // Chaining rules would also be possible if the @Rule annotations above are removed
  @Rule
  public final RuleChain dependantRule =
    // This would check that destroying the container does not output to stderr
    RuleChain
    .outerRule(noSystemErrorRule)
    .around(noContainerExistsRule);
}

This comes quite close to the TestFragments idea in #444. (Side note: Guards must not contain test cases O:-) )

What do you think of my ideas so far? Would it be good if MyGuard implemented a certain interface (similar to RunListener, but with the ability to fail tests) for lifecycle events? I'm not certain, but if we used @Before and @After annotations any JUnit user could easily create/adapt his/her own Guards.

On the technical side, it may be better to manage Guards similar to the existing RunListener/RunNotifier code? However, because to the "Exceptions remove the listener" rule, we must not reuse this.

If a guard fails, the error should be reported in a way that it's very clear where it came from, e.g.

AssertionError: A rule in com.example.MyGuard declared in the package-info.java file for package com.example failed.
  at org.junit.runner.JUnitCore.run(Runner):132
  [...]
Cause: AssertionError: There has been output to System.err
  at NoSystemErrorRule:42

This should weaken the "my test fails and I don't know why as I didn't declare anything in the test class" argument a bit.

Possible next steps

I really would like to contribute to JUnit if possible, so my proposal would be the following:

  • to discuss possible approaches and find a consensus on open questions
  • after that, I'd like to implement a prototype according to JUnit's contribution guidelines
  • Make the prototype production-ready and ship it as part of the next upcoming release as soon as possible to receive more feedback from the community. For that reason, I would like use JUnit 4.x as a foundation (porting it to JUnit 5 afterwards).

So, what do you think? :)

@tburny
Copy link
Author

tburny commented Nov 17, 2015

Package annotation based configuration

Declaring Guards for packages

Typically projects are structured into packages com.example.projectA and com.example.projectB etc. By placing a package-info.java into

  • com.example a set of global rules for both projectA and projectB can be declared
  • com.example.projectA a set of rules only for project A can be declared. Same goes for com.example.projectB

The package-info.java could look like

@GuardedBy({MyFirstGuard.class, MySecondGuard.class})
package com.example

Excluding Guards for packages, classes and methods

As a counter-piece @NotGuardedBy can be used to exclude Guards

package com.example;

// Only guarded by MyFirstGuard
@NotGuardedBy(MySecondGuard.class)
public class MyTest {

}

All annotations are inherited by default, so they do not have to be declared for every package (it is possible to list all packages in the current classloader, then use their names for the hierarchy).

A more complete example:

@GuardedBy({MyFirstGuard.class, MySecondGuard.class})
package com.example

// inherits MyFirstGuard, as MySecondGuard is excluded
@GuardedBy(OtherGuard.class)
@NotGuardedBy(MySecondGuard.class)
package com.example.projectA

Project A initially inherits the set of Guards {MyFirstGuard, MySecondGuard}. Due to the @NotGuardedBy annotation, it then becomes {MyFirstGuard, MySecondGuard} - {MySecondGuard} = {MyFirstGuard}. Now building the set union with the GuardedBy annotation results in {MyFirstGuard} ∪ {OtherGuard} = {MyFirstGuard, OtherGuard} as the set of effective guards.

In the bad case this should not be possible(e.g. due to problems with Java not having a real package hierarchy or with package annotation processing), we could resort to the properites file solution. There we e.g. could point to classes containing the guard configuration (similar to package-info.java, but as a class).

@Tibor17
Copy link
Contributor

Tibor17 commented Nov 18, 2015

In this case I would create super class which has test @rules . That's easy. You can force the devs to inherit such class in every test. You can write a plugins discovering public classes and check if they extend particular super class.
The System.err restriction is strange to me. If I was in your position, I would ignore such restriction and teach the devs to use assertion statements instead of printing to std/err. If this does not work, then there is problem with the organization and not the problem with code.
Not everything lives in code!

btw. the Deltaspike`s test module has JUnit runner which stops the container after test. No need for any rule.

@BenRomberg
Copy link

@tburny: Impressive summary, I like it!

I ended up writing a test enforcing the presence of a certain rule in all other tests, take a look if you're interested: RuleEnforcingTestBase and the implementing RuleEnforcingTest (I needed the test in multiple modules).

It would however be much more elegant if the test framework would support this. Your API suggestion is a good start. I don't know if it's worth to introduce yet another concept (Guard vs. Rule) but haven't thought about it too much.

Maybe a few words on the arguments that have been brought up against Package Rules:

  • integration with other tools that use their custom runners, like Maven or other IDEs
    • That will be fixed with the JUnit Lambda project, which gives a proper API for test running environments and should make the need for custom solutions obsolete.
  • Tests should not fail due to some "obscure" magic (package level annotations, junit.properties).
    • I don't consider it "obscure magic" if the Assertion Error would state where the Error is coming from. The framework could wrap the thrown Error and enhance the Error message with the location of the Package-level declaration.
  • The same could be achieved with: TestListener, Runner, ...
    • That's not an argument, as the point is not having to adjust every test class.

I ended up having the exact same problem with 2 more projects and ended up implementing workarounds that are not really elegant, but do the job (one example being #1074 which would make implementation a lot easier using Package Rules).

@tburny
Copy link
Author

tburny commented Dec 28, 2015

Thank you both for your comments :) @BenRomberg sums up the problem quite perfectly. Guards are easier to review and maintain than to modfiy each test (250+ at my company) and go through all the VCS and QA workflows. Appearantly Global Rules are listed in the Quo Vadis Wiki page, which is great!

It would be awesome to receive some feedback of the @junit-team also 😃

To see how difficult it is to implement Guards, I implemented a throughly tested(!) proof of concept with the features I described above - including annotation based configuration - and integrates naturally with Maven Surefire and IntelliJ. I only had to modify BlockJUnit4ClassRunner and minor parts of ParentRunner. No API changes were required.

There are two problems where I would appreciate some help:

  • Currently it is possible to annotate methods with @GuardedBy and @NotGuardedBy. Unfortunately I couln't find a way to wrap execution of @ClassRule in Guards around the class rules declared in the test itself by ParentRunner, as these work on a class-level only and then pass the resulting Statement to the block running the test method. Any ideas? Maybe this is also an important point for Junit Lambda?
  • The code for test validation and rule discovery should become more reusable by non-Runner-classes. See also partly Move test class validation aside #290. For that reason I currently skipped this part.

I propose that I'll open a pull request, so we have a good base for further discussions about the architecture and implementation. 😃

Final word: I guess it should be easy to add Guards to JUnit Lambda by reusing the existing code.

@Tibor17
Copy link
Contributor

Tibor17 commented Dec 28, 2015

I agree that junit.properties and package level annotations are the step
back.

I was supporting JUnit for long time and now I maintain Surefire project.
The Maven project allows me to see users from wider perspective. All the
time I had to struggle with broken backwards compatibility of TestNG and
JUnit. Therefore I want to simplify all and let the users implement pure
interface TestCase where the behavior of Runner is up to the them. Thanks
to Java 8 the interface would have only default implementation.
All the Surefire project needs is to intercept test methods with Proxy and
notify listeners. I think everything else should be apart of the framework
and that's why only a pure interface should come into framework. The
behavior of Runner should be apart of Surefire. The developers should
deploy good Runners to Maven Central which would finally speed up the
evolution.

Nowadays people are able to implement their own runners, but more problems
they have with existing JUnit runners which must be changed with a lot of
staff inside if just one change is proposed.

Now we have an idea in Maven Surefire to introduce SPI interfaces where the
user would be able o customize plugin's behavior. Later we will introduce
fully incremental build and tests which is with SPI or Plexus/CDI
components possible.
It seems that opening interfaces for public use will be the way to go
instead of maintaining some framework's implementations. This would give us
and users more freedom to concentrate our energy on new goals.

Cheers
Tibor

On Mon, Dec 28, 2015 at 12:52 PM, Tobias Brennecke <notifications@github.com

wrote:

Thank you both for your comments :) @BenRomberg
https://github.com/BenRomberg sums up the problem quite perfectly.
Guards are easier to review and maintain than to modfiy each test (250+ at
my company) and go through all the VCS and QA workflows. Appearantly Global
Rules are listed in the Quo Vadis Wiki page
https://github.com/junit-team/junit/wiki/Quo-Vadis-JUnit#topics-ideas-concerns,
which is great!

It would be awesome to receive some feedback of the @junit-team
https://github.com/junit-team also [image: 😃]

To see how difficult it is to implement Guards, I implemented a throughly
tested(!) proof of concept with the features I described above. It works
with the annotation based configuration I described above and integrates
naturally with Maven Surefire and IntelliJ
, as I only had to modify
BlockJUnit4ClassRunner and minor parts of ParentRunner - no API changes
required.

There are two problems where I would appreciate some help:

  • Currently it is possible to annotate methods with @GuardedBy and
    @NotGuardedBy. Unfortunately I couln't find a way to wrap execution of
    @ClassRule in Guards around the class rules declared in the test
    itself by ParentRunner, as these work on a class-level only and then
    pass the resulting Statement to the block running the test method. Any
    ideas? Maybe this is also an important point for Junit Lambda?
  • The code for test validation and rule discovery should become more
    reusable by non-Runner-classes. See also partly Move test class validation aside #290
    Move test class validation aside #290. For that reason I
    currently skipped this part.

I propose that I'll open a pull request, so we have a good base for
further discussions about the architecture and implementation. [image:
😃]

Final word: I guess it should be easy to add Guards to JUnit Lambda by
reusing the existing code.


Reply to this email directly or view it on GitHub
#1219 (comment).

Cheers
Tibor

@tburny
Copy link
Author

tburny commented Apr 28, 2016

Nowadays people are able to implement their own runners, but more problems
they have with existing JUnit runners which must be changed with a lot of
staff inside if just one change is proposed.

I saw a few runners since your last post and I figured out that your argument is perfectly valid.

In JUnit 5 the concept of annotation based extensions replaces Rules. These extensions (should) allow modifying the test execution without modifing the test runner. In summary these allow having the required flexibility for extending JUnit on a class-level.

However still it is quite easy to forget adding an extension annotation or it isn't feasible to do that for hundreds of classes.

Maybe leveraging the extension concept in JUnit 5 to a package level would be a good solution, what do you think on this?
Almost all programmers are using a sort of organization package like com.example, so one could add an annotation in a package-info.java file in that package (or any subpackage), similar to what's already happening for classes. For instance when having a package containing lots of tests that use Guice, one simply could annotate that package with a GuiceExtension and everything is done, so basically it would be just a level up the hierarchy.

Would there be reasons against that?

@kcooney
Copy link
Member

kcooney commented Apr 28, 2016

@tburny JUnit 5 feature requests should be filed at https://github.com/junit-team/junit5

I doubt we will support adding behavior via package level annotations for JUnit4-style tests. Currently we don't even support adding behavior via class annotations (ouside of @RunWith; you add behavior by annotating methods or fields.

@paulduffin
Copy link
Contributor

I've created another issue similar to this but focused on Android that references this. See #1307. I also have some prototype changes to add support. Anyone who's interested please take a look at let me know what you think.

@m9aertner
Copy link

Using a "Global Rules" approach, I was able to replace an even lower-level solution using AspectJ that had the goal of fixing certain things centrally, i.e. without having to change the unit test classes.
While this is not a shippable amendment right now, I'd like to share some of the points that I found along the way:

  • We have thousands of unit test classes. With and without base classes, JUnit 4 mostly, some JUnit 3. Points mentioned above like "just add base classes", "ask the developers to..." and "use @RunWith on the unit test class" were not so attractive. Global rules are great because they allow one to change lots of things centrally which is great at a certain scale.
  • We're on JUnit 4.12, by the way, and call tests from Gradle via Ant and IDEs.
  • My entry point is AllDefaultPossibilitiesBuilder. This class determines which "Runner" to use for a specific test. Unfortunately, I did not find a way in JUnit to inject a variant of this class using JUnit configuration, so I leveraged an existing classpath ordering mechanism to inject my variant implementation.
  • For the JUnit 4 and JUnit3 cases, I create new ClassRunners which extend the original implementations, i.e. BlockJUnit4ClassRunner for JUnit4.
  • Extending these is not super straightforward, as some methods are private (withClassRules is, withAfterClasses is not, this is inconsistent), but doable. It's a different discussion on how to improve the extensibility of JUnit ...
  • That Runner adds further "Statements" to the existing sequence. This is done on two levels: class rules and method rules.
  • I read the rules from a GuardingRules Java file, converted to TestClass, which is on the side and not directly connect the the unit tests being guarded. Code is available in JUnit for reading the rules off that TestClass and for running the rules in a custom Statement (RunRules).
  • For now, I've hard-coded that GuardingRules.java file. I've toyed with above-mentioned approach to annotate package-info.java classes, but that did not work satisfactorily. I though it would be nice to find the "nearest" annotated package-info, walking to the root. That worked, but it looks like one cannot have a top-level (default package) package-info file. This means for each of our top-level packages (com, org, ...) we'd have to have (likely redundant) guards files. Not good for us, but smaller codebases probably have a single root anyway, so this may not be an issue.
  • For JUnit3 it was possible to wrap the existing runner inside the above Rule-based one, so we're mixing rules and JUnit 3 for these cases (so far this looks good, but clearly that's most experimental).
  • And finally: the classpath injection approach works nicely in all environments, i.e. Eclipse, IDEA, and on the command line, without any re-configuration of the environment.

And why all this? This appraoch allows us to do monitoring (limit execution time, i.e. global @timeout), logging and resource control globally. Also this is granular on @BeforeClass, test method(s) and @afterclass level! (@Before/After is lumped together with the method execution, though. Would need more code to separate these apart).

Cheers,
Matthias

@kevdowney
Copy link

@m9aertner Is there any parts you can share on your implementation for global rules... we have a large codebase without mixed tests but looking at efficient ways to apply a global rules?

@Unlimity
Copy link

Hey everyone. Our team also had struggles with lacking of such feature. So I made a PR, which you can grab for yourself (if it will not be merged).
Cheers!

@m9aertner
Copy link

@Alviere Your PR with explicit command line options looks great. Thanks for sharing this.

@Unlimity
Copy link

@m9aertner You're welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants