Skip to content

fix: BazelJUnitOutputListener logging xml output on suite-level failures #328

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

Merged
merged 6 commits into from
Mar 22, 2025

Conversation

amishra-u
Copy link
Contributor

fixes: #327

  1. Updated code to propagate the test suite failure to each test case.
  2. The test suite failure will be propagated to each child node, even if the test is not executed.
  3. Additionally, this change fixes the bug introduced in Add support for @AfterAll in XML report #300, which incorrectly logged the test_suite as a testcase and logged incorrect values for the "tests" attribute in the node.

@fzakaria
Copy link
Contributor

fzakaria commented Mar 10, 2025

my hot take:

I wrote some tests that work on actual test classes rather than TestData -- maybe it would be good to continue in that direction.

The unit tests on TestData only take us so far; we need to write more "integration-style" tests running on real classes IMO since the JUnit data structure is kind of opaque.

@shs96c
Copy link
Collaborator

shs96c commented Mar 11, 2025

@fzakaria, I've no objection to a very limited number of test cases like this, but they tend to be slow to run, require a lot more set up, and can be opaque to debug and understand. If we introduce them, I'd rather they were an absolute minority of tests for this kind of functionality.

@fzakaria
Copy link
Contributor

@shs96c i agree hollistically but the JUnit stuff is so opaque -- feels like starting in the reverse direction is good.
i.e. writing "integration style tests" and then taking that information to encode it as a unit test with the TestData mock.
I guess they can always be tagged and run in a separate step ;P

Anyways -- just my thoughts. As someone who likes to contribute this can help me understand if I'm breaking other consumers.

@amishra-u
Copy link
Contributor Author

@shs96c Can you please take a look

Copy link
Collaborator

@shs96c shs96c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but could you please run bazel run scripts:format?

@amishra-u
Copy link
Contributor Author

@shs96c Can you please merge it. I have fixed formatting issue.

@illicitonion illicitonion merged commit 1013ee4 into bazel-contrib:main Mar 22, 2025
7 checks passed
@amishra-u amishra-u deleted the empty_suite branch March 27, 2025 01:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BazelJUnitOutputListener Logs Empty TestSuite on Test Suite Level Failure
4 participants