Fix OTel JUnit5 Extension cleanup when using Nested test classes #7999
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7999 +/- ##
============================================
- Coverage 90.16% 90.15% -0.02%
- Complexity 7475 7476 +1
============================================
Files 836 836
Lines 22551 22552 +1
Branches 2224 2225 +1
============================================
- Hits 20333 20331 -2
- Misses 1515 1517 +2
- Partials 703 704 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@jkwatson @austince I expected this commit to fail as Test reproduces the issue. But it seems its not part of CI. ./gradlew :sdk:testing:build
Parallel Configuration Cache is an incubating feature.
Calculating task graph as configuration cache cannot be reused because cached version information for io.opentelemetry:opentelemetry-api:latest.release has expired.
Kotlin does not yet support 25 JDK target, falling back to Kotlin JVM_24 JVM target
> Task :buildSrc:compileKotlin UP-TO-DATE
Kotlin does not yet support 25 JDK target, falling back to Kotlin JVM_24 JVM target
> Task :sdk:testing:test
OpenTelemetryExtensionNestedTest > FirstNestedClass > recordSpan() FAILED
java.lang.AssertionError:
Expected size: 1 but was: 0 in:
[]
at io.opentelemetry.sdk.testing.junit5.OpenTelemetryExtensionNestedTest$FirstNestedClass.recordSpan(OpenTelemetryExtensionNestedTest.java:30)
80 tests completed, 1 failed
> Task :sdk:testing:test FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':sdk:testing:test'. |
Reproduces issue open-telemetry#7919 where extension's `afterAll` hook is called prematurely after one nested test class execution completes, making the extension unusable for other nested test class.
The afterAll lifecycle callback requires a valid ExtensionContext to determine if cleanup should occur at the top level. The test was previously passing null, causing NPE after the nested class handling logic was added.
The nested class path (where close() is not called) was not covered, causing codecov patch coverage to fall below the 80% threshold. Added a test for NestedClass scenario to validate that sdk remains functional when afterAll is called post Nested class test.
d0c2c3f to
bf52143
Compare
Contributor
Author
|
Modified Extension tests to include mocked ExtensionContext. It looks ok now. |
Contributor
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JUnit5 executes the extension lifecycle per container class (Top or Nested). As a result
afterAllis called immediately post execution of one nested class.opentelemetry-java/sdk/testing/src/main/java/io/opentelemetry/sdk/testing/junit5/OpenTelemetryExtension.java
Lines 196 to 200 in 18f8a13
Making it unavailable for other Nested classes followed by failures.
As discussed in #7919 (comment), this solution preserves the Extension until
afterAllis called at the top level.We can look into more elaborate cleanup via ExtensionContext.Store.CloseableResource if required in future.