Introduce error handling mechanism for validation and discovery errors

[marcphilipp]

Overview

Validation errors (e.g., for invalid @BeforeEach method declarations) should not abort the entire discovery phase. Instead the discovery phase should continue, with the error tracked and reported during the execution phase.

Areas of Applicability

• invalid @BeforeAll, @AfterAll, @BeforeEach, and @AfterEach method declarations
• invalid @Test, @TestFactory, @RepeatedTest, and @ParameterizedTest method declarations (see IsTestableMethod silently ignores @Test annotated methods that return a value #2244)
• invalid @Nested test class declarations (see Improve diagnostics when Vintage test engine is missing #1223, Abstract classes with @Nested are not ignored / not handled properly #2717, and Feature suggestion: Detect missing @Nested annotations #1736)
• invalid @Suite class declarations
• blank display names supplied by the user, as discussed in Display names must not be allowed to be blank #743
• exceptions thrown by test engines, as discussed in DefaultLauncher should catch exceptions thrown by test engines #750
• unresolvable unique IDs, as discussed in Handle unparseable UniqueID consistently #210
  • note, however, that a TestEngine should not report a unique ID as unresolvable if the unique ID does not apply to the particular TestEngine (see JupiterTestEngine attempts to resolve unique IDs that belong to other engines #1026 ).
• non-static test class declaration (see Better diagnostics for orphaned selectClass() on a nested class #2311)

Proposals

1. Allow engines to track errors by creating a special type of TestDescriptor such as an AlwaysFailingTestDescriptor, DeadOnArrivalTestDescriptor, or ErrorReportingTestDescriptor.
   • Tracked errors for such a corresponding TestDescriptor could then be thrown as an exception during the execution phase instead of executing the corresponding container or test.
2. Introduce a new property in TestDescriptor that signals an error that was encountered during the discovery phase.
3. Pass a reporting object from the Launcher into each TestEngine to report errors.
   • See Introduce error handling mechanism for validation and discovery errors #242 (comment)

Related Issues

• Introduce validation rules for tests #121
• Handle unparseable UniqueID consistently #210
• Display names must not be allowed to be blank #743
• DefaultLauncher should catch exceptions thrown by test engines #750
• Forbid return values from test and lifecycle methods #835
• Improve error message if multiple "test" annotations are present on a method #876
• Move @TestFactory return type validation to IsTestFactoryMethod #949
• Static @BeforeEach method causes entire test plan to abort #971
• JupiterTestEngine attempts to resolve unique IDs that belong to other engines #1026
• Complain when @Test is applied to an inappropriate method #1074
• Improve diagnostics when Vintage test engine is missing #1223
• Unsupported or missing version of JUnit 4 should fail the build #1944
• IsTestableMethod silently ignores @Test annotated methods that return a value #2244
• Better diagnostics for orphaned selectClass() on a nested class #2311
• Abstract classes with @Nested are not ignored / not handled properly #2717
• Tests within inner classes that are children of an abstract class are not executed despite @Nested #4125

Deliverables

• Introduce a mechanism that allows validation and configuration errors to be tracked during the test engine discovery phase and then thrown or reported during the execution phase.
• Use the new mechanism to replace the current ad hoc use of logging and transparent defaulting as a work-around.
  • Search for TODO [#242] within the code base.
  • See DefaultLauncher should catch exceptions thrown by test engines #750 (comment)
• Determine where else the new mechanism can be used and apply it.
• Revisit the results of Forbid return values from test and lifecycle methods #835 and update the code accordingly by tracking such errors instead of just ignoring such incorrect usage.
• Revisit the changes made in Static @BeforeEach method causes entire test plan to abort #971 and determine if it makes sense to move the look-up of lifecycle methods back to the constructor of ClassTestDescriptor.

[sbrannen]

The same is true for invalid custom display names supplied by the user, as described in #743.

I will therefore update the description of this issue to reflect the broader scope.

[sbrannen]

FYI: I have introduced a minimum set of Deliverables.

[gaganis]

I am thinking this reminds me the approach we took for vintage to handle multiple assertion errors using a MultipleFailuresError.

We could be possible that we could store all validation errors in a Throwable - using a MultipleFailuresError when there is more than one, that would be carried over to the execution phase and when this is nonEmpty we would throw the exception instead of executing the test.

---

It might also be worth considering for this mechanism to track all exceptions that occur while processing a single discovered test and throw it in the execution phase.

This way instead of the whole process stopping, we could still process the rest of the tests that encountered no problems and only fail on single tests.

PS While I was writing, this @sbrannen changed the title and description and now it sound more like what I am writing here! 😃

[sbrannen]

@gaganis, thanks for sharing your ideas.

A few notes...

No need to use MultipleFailuresError.

We're on Java 8 and can use suppressed exceptions.

Plus, we already have a ThrowableCollector within the JUnit Jupiter engine that could potentially be used for this purpose as well.

[sbrannen]

The scope of this issue has been broadened in order to encompass the Platform as well.

[jlink]

Rather than using exceptions of any kind, having some kind of notification collector as a return or in/out parameter seems more flexible since any exception will stop an engine's discovery phase whereas many notifications are just warnings or pieces of information .

[sbrannen]

Rather than using exceptions of any kind, having some kind of notification collector as a return or in/out parameter seems more flexible since any exception will stop an engine's discovery phase whereas many notifications are just warnings or pieces of information .

Good point!

I'll change the title of this issue accordingly.

[sbrannen]

FYI: title and description have updated.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. Thank you for your contribution.

[GavinRay97]

Following up here on the issue posted at #2659 (whoops):

This behavior can cause quite a headache and be very confusing.
If I were to try to PR a minimal change which just did the below:

Map<String, JavaType> methodNameTypeMap = new Map()
for test in tests:
  methodNameTypeMap.insert(test.name, test.method.returnType)
  
when "test results are reported":
  for (testName, testFnType) in methodNameTypeMap.entries():
    if testFnType != "void" then:
      print(f"[WARNING]: Found test {testName} with type {testFnType}. JUnit only resolves tests of type void -- test was skipped")

How difficult would this change be to make as someone who didn't know the codebase?
The reason why I ask is because this leads to scenarios like these:

• No tests were found when trying to run JUnit tests with Scala 2 or WIP Scala 3 extension quarkusio/quarkus#18373
• https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/.22No.20Tests.20Found.22.2C.20last.20issue.20for.20Scala.203.20support

This was a multi-day investigation for me =/
I don't want anyone else to have to go through that again. Can we make this better for everyone?

[Marcono1234]

Should this also cover @Suite? Its predicate class IsSuiteClass seems to silently ignore the class if the predicate does not match, despite being explicitly annotated.

[sbrannen]

Yes, this should also apply to @Suite classes. I'll add that to this issue's description.

[simondean]

Hi. This is big issue when writing unit tests in Kotlin. It's really easy to create unit test in Kotlin that implicitly returns a none void return type. JUnit then just silently fails to run any of your Kotlin unit test that have a none void return type.

As an example, in a codebase with about 800 unit tests, I found that about 20 of the unit tests were being silently ignored by JUnit 5 because of this issue.

Here a repo with a minimal reproducible example for easily hitting this bug in Kotlin:

https://github.com/simondean/kotlin-junit-run-blocking-issue/blob/main/src/test/kotlin/IssueTest.kt

    @Test
    fun `also will be silently skipped`() = runBlocking {
        // This bug is not specific to Mockito but the bug is easily triggered by putting a verify() call at the end of a unit test
        val value = "anything"
       
       // Because verify() has a return value, so does the whole unit test, which causes JUnit to silently ignore the test (and any others like it in the codebase)
        verify(value).toString()
    }

See the following duplicate issues for more info/context:

• IsTestableMethod silently ignores @Test annotated methods that return a value #2244
• Silently skipped tests when last line of Kotlin test method returns a coroutine #2742

Given this issue has been open for 5 years and it's arguably a more serious issue now that people are using Kotlin with JUnit, would it be worth boosting the priority of this issue? Thanks!

[sbrannen]

would it be worth boosting the priority of this issue? Thanks!

I've added the new label to signal to the team that it needs to be reconsidered.

[simondean]

Thanks @sbrannen

[timdawborn]

As an example, in a codebase with about 800 unit tests, I found that about 20 of the unit tests were being silently ignored by JUnit 5 because of this issue.

@simondean Out of interest, how did you detect these? I'm looking to implement similar logic and am searching for prior art currently before implementing something bespoke.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. Given the limited bandwidth of the team, it will be automatically closed if no further activity occurs. Thank you for your contribution.

[stale]

This issue has been automatically closed due to inactivity. If you have a good use case for this feature, please feel free to reopen the issue.

[JohnnyMorganz]

Hey folks, just wondering whether there was any movement on this?

I recently stumbled on the exact same issue as in #1223 where we had junit4 tests being run in junit5 without vintage on the classpath, and the tests silently passed with 0 test cases run. Hoping that this could be better reported to the developer if they don't realise their tests didn't actually run

[marcphilipp]

This is definitely on the short list for when I manage to set aside more time for JUnit but nothing concrete has happened, yet.