fix: support nested classes properly #351
Merged
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
christianladam
force-pushed
the
christianladam/fixDeeplyNestedClassesXml
branch
2 times, most recently
from
786369d to
9d26751
Compare
|
Changes after approval include:
There were no other changes beyond those. |
christianladam
force-pushed
the
christianladam/fixDeeplyNestedClassesXml
branch
from
9d26751 to
da244ba
Compare
|
Fixed the issue in the test output test where the children weren't being picked up because they weren't actually being added to the test plan. This is due to the original code always adding the groups but not necessarily adding tests. With the new code we only add the groups when there's actually a test. |
Previously if you had deeply nested classes the BazelJUnitOutputListener would throw an exception. In JUnit5 TestExecutionListeners are not expected to throw exceptions and any exception just gets swallowed and logged as a warning. This would cause cases where an exception was thrown to output no test results and result in a successful test run even though there was a failure in the listener itself. The new logic attempts to get the closest parent segment that is a class type. This doesn't match the way things like gradle will output test XML but solves the problem with nested classes while keeping the output similar. In terms of output this actually fixes an issue that is visible in the NestedClassesTest today, where the Second nested classes function result gets grouped into the outer class while the static First classes function result appears in a separate group as shown below: ``` NestedClassesTest shouldBeExecuted shouldBeExecuted NestedClassesTest$First shouldBeExecuted ``` With the changes the grouping is fixed as shown below: ``` NestedClassesTest shouldBeExecuted NestedClassesTest$First shouldBeExecuted NestedClassesTest$Second shouldBeExecuted NestedClassesTest$Second$Third shouldBeExecuted NestedClassesTest$Second$Third$Fourth shouldBeExecuted NestedClassesTest$Second$Third$Fourth$Fifth shouldBeExecuted ``` These groups appear unordered in UI like IntelliJ which isn't ideal, but these changes closely align with how things are done today, while avoiding just silently passing with no test output if you take this version of the NestedClassesTest and run it using the previous BazelJUnitOutputListener code.
christianladam
force-pushed
the
christianladam/fixDeeplyNestedClassesXml
branch
from
da244ba to
e906129
Compare
This was referenced Jul 24, 2025
This was referenced Jul 25, 2025
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.
Previously if you had deeply nested classes the BazelJUnitOutputListener would throw an exception. In JUnit5 TestExecutionListeners are not expected to throw exceptions and any exception just gets swallowed and logged as a warning. This would cause cases where an exception was thrown to output no test results and result in a successful test run even though there was a failure in the listener itself. The new logic attempts to get the closest parent segment that is a class type. This doesn't match the way things like gradle will output test XML but solves the problem with nested classes while keeping the output similar.
In terms of output this actually fixes an issue that is visible in the NestedClassesTest today, where the Second nested classes function result gets grouped into the outer class while the static First classes function result appears in a separate group as shown below:
With the changes the grouping is fixed as shown below:
These groups appear unordered in UI like IntelliJ which isn't ideal, but these changes closely align with how things are done today, while avoiding just silently passing with no test output if you take this version of the NestedClassesTest and run it using the previous BazelJUnitOutputListener code.