Skip to content

Test reporting name #325

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Test reporting name #325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5530cf
fix: Use legacy reporting name
amishra-u Mar 1, 2025
63a7fc7
update test
amishra-u Mar 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 1 addition & 12 deletions java/src/com/github/bazel_contrib/contrib_rules_jvm/junit5/TestCaseXmlRenderer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,18 +33,7 @@ public void toXml(XMLStreamWriter xml, TestData test) throws XMLStreamException
decimalFormat.setRoundingMode(RoundingMode.HALF_UP);

TestIdentifier id = test.getId();

String name;
if (test.isDynamic()) {
name = id.getDisplayName(); // [ordinal] name=value...
} else {
// Massage the name
name = id.getLegacyReportingName();
int index = name.indexOf('(');
if (index != -1) {
name = name.substring(0, index);
}
}
String name = id.getLegacyReportingName();
Copy link
Contributor

@christianladam christianladam Jul 28, 2025

Choose a reason for hiding this comment

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

This breaks being able to set custom names in annotations for things like ParameterizedTests and would cause the xml rendering for junit5 in bazel to be significantly different from how it's done in maven and gradle. The comparative folder has all the xml that those tools generate. It's definitely less than ideal that one can have custom names that break uniqueness and defaults for certain types like Repeated and Parameterized in junit5 are not descriptive enough. That said those (the junit5 Repeated/Parameterized) can be set via name formatters in junit5.

Copy link
Contributor

@christianladam christianladam Jul 28, 2025

Choose a reason for hiding this comment

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

I wonder if it would make sense to have a flag you could set that gets checked on line 38 and fall backs into the else block if so.

Copy link
Contributor Author

@amishra-u amishra-u Jul 28, 2025

Choose a reason for hiding this comment

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

You're right — this would break the custom name logic.

Just as an FYI, we've been running this code for the past six months. One aspect we appreciated about LegacyReportingName is that it produces deterministic test names, which greatly simplifies tooling. In contrast, with custom names, developers have many ways to make test names non-deterministic, which complicates things.

Some developers did raise concerns about not being able to set custom names. We acknowledged this and decided to allow custom names locally but not in CI. So, it might be best to enforce this behavior via a system property check.

Copy link
Contributor

@christianladam christianladam Jul 28, 2025

Choose a reason for hiding this comment

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

JUnit5 has both the https://docs.junit.org/snapshot/user-guide/#junit-platform-reporting-open-test-reporting and https://docs.junit.org/snapshot/user-guide/#junit-platform-reporting-legacy-xml options for XML reporting. rules_jvm tries to match the default junit4 test runner's xml structure in bazel while closely following gradle/maven in terms of how they do junit5 xml. Seems like having some sort of flag you can set in a .bazelrc file could be cool that would change the reported naming since custom naming is pretty inconsistent and makes metrics gathering on test times more difficult. I can take a stab at doing something here, but I'm not a maintainer or anything so we'd need to get feedback from one of them.


xml.writeStartElement("testcase");
xml.writeAttribute("name", escapeIllegalCharacters(name));
Expand Down
2 changes: 1 addition & 1 deletion .../test/com/github/bazel_contrib/contrib_rules_jvm/junit5/BazelJUnitOutputListenerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ public void ensureTestCaseNamesAreProperlyEscaped() {
Node item = xml.getElementsByTagName("testcase").item(0);
String testName = item.getAttributes().getNamedItem("name").getNodeValue();

assertEquals("[engine:mocked]/[class:ExampleTest]/[method:Weirdname", testName);
assertEquals("[engine:mocked]/[class:ExampleTest]/[method:Weirdname()]", testName);
}

@Test
Expand Down
Loading