Improve diagnostics when Vintage test engine is missing

[seanjreilly]

Overview

(tl;dr: I feel JUnit might be easier to use for developers if things failed more explicitly when developers make certain specific mistakes. I've got a suggestion for how to do this that I'd like to discuss. I'm willing to do much of the work if it's the right approach and if the submission would be acceptable.)

While using JUnit 5 (excellent work by the way!) I've encountered some situations where an innocent developer error can cause tests not to be executed in ways that are easy to miss.

• When using the @Nested annotation with a static inner class, the class (and any tests declared within it) are not executed. No error messages of any kind that I can see are emitted. This situation is difficult to spot by visual inspection of the code, and because the tests aren't marked as skipped, catching the mistake is difficult in a larger project — one would need to take a very close look at test totals, read test results by hand, etc.

• When using the JUnit 5 platform with JUnit 4 tests, if the JUnit Vintage dependency isn't on the classpath but JUnit 4 somehow is, tests can easily get skipped. I know that this sounds theoretical but it happened to some colleagues of mine just before Christmas. It took us an hour or so to debug (once they figured out that it was happening) because it wasn't obvious to them from looking at the gradle build file or output what the cause was. Even worse, the IDE (IntelliJ in this case) was still happily running the JUnit 4 tests, so it wasn't immediately obvious that something wrong was even happening until somebody took a close look at build script output on the CI server.

There are some commonalities between these problems:

• They both involve things that developers need to know, but sometimes don't.
• When a mistake happens, it's difficult to diagnose just by visual inspection unless you are aware of the thing you need to know.
• It can be easy to miss these mistakes if you aren't consciously looking for them.

It might be annoying to people in the short term, but
ultimately I think JUnit would be safer for developers if test runs explicitly failed in both of these conditions, rather than just not executing tests.

Proposal

I propose fixing this by causing a test run to fail whenever either of the above mistakes is made. I took a brief look at JUnit's source code, and the idea that occurred to me is some new Resolver implementations that detect each scenario and cause tests (or just the build) to fail.

I've got some ideas on how to programatically find the error conditions for each resolver:

• For the "non static @Nested class" case, it should just be inverting the existing predicate (org.junit.jupiter.engine.discovery.predicates.IsNestedTestClass) and checking for inner classes annotated with the @Nested that don't match the predicate.
• The Junit 4/5 case is a bit harder, but it should be possible to find methods annotated with org.junit.Test when the JUnit vintage classes are not on the runtime classpath. We could use reflection to check for the existence of the org.junit.Test annotation and JUnit vintage so no extra dependencies are be needed. This resolver could shorrt circuit into a no-op in cases where JUnit4 isn't present on the classpath, or JUnit4 and JUnit Vintage are both on the class path.

What wasn't immediately obvious to me was the best way for resolvers like this to cause tests (or just the whole build) to fail. I could really use some advice on this part.

Feedback requested:

• Is guarding against these issues a desirable feature?
• If so, is writing some new Resolvers the best way to go about it?
• If so, what's the best way for a Resolver or associated Descriptor to emit an error message and fail the build?
• If I develop a patch of appropriate quality for this, is it likely to be accepted?

Deliverables

If all of the above is acceptable then I imagine that it might be best to open a separate PR for each case when I've got something workable.

Any and all feedback gratefully requested!

[sbrannen]

This sounds like a duplicate of #242.

So I'd suggest the points raised here should be raised in conjunction with #242.

[seanjreilly]

Listing this as a related issue of #242 makes sense to me, and the proposals and deliverables listed there would certainly be helpful here.

Thank you.

[sbrannen]

FYI: I added the "invalid @Nested test class" to #242 and changed the title of this issue accordingly to focus solely on the absence of the Vintage test engine.