-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Introduce error handling mechanism for validation and discovery errors #242
Comments
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. |
FYI: I have introduced a minimum set of Deliverables. |
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! 😃 |
@gaganis, thanks for sharing your ideas. A few notes... No need to use We're on Java 8 and can use suppressed exceptions. Plus, we already have a |
The scope of this issue has been broadened in order to encompass the Platform as well. |
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. |
FYI: title and description have updated. |
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. |
Following up here on the issue posted at #2659 (whoops): This behavior can cause quite a headache and be very confusing. 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?
This was a multi-day investigation for me =/ |
Should this also cover |
Yes, this should also apply to |
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:
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! |
I've added the |
Thanks @sbrannen |
@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. |
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. |
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. |
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 |
This is definitely on the short list for when I manage to set aside more time for JUnit but nothing concrete has happened, yet. |
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
@BeforeAll
,@AfterAll
,@BeforeEach
, and@AfterEach
method declarations@Test
,@TestFactory
,@RepeatedTest
, and@ParameterizedTest
method declarations (see IsTestableMethod silently ignores @Test annotated methods that return a value #2244)@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)@Suite
class declarationsProposals
TestDescriptor
such as anAlwaysFailingTestDescriptor
,DeadOnArrivalTestDescriptor
, orErrorReportingTestDescriptor
.TestDescriptor
could then be thrown as an exception during the execution phase instead of executing the corresponding container or test.TestDescriptor
that signals an error that was encountered during the discovery phase.Launcher
into eachTestEngine
to report errors.Related Issues
@Nested
are not ignored / not handled properly #2717Deliverables
TODO [#242]
within the code base.ClassTestDescriptor
.The text was updated successfully, but these errors were encountered: