-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
test_runner: remove failure attribute in junit_report testcase element #59685
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
base: main
Are you sure you want to change the base?
Conversation
|
Review requested:
|
| children: [inspectWithNoCustomRetry(error, inspectOptions)], | ||
| }); | ||
| currentTest.failures = 1; | ||
| currentTest.attrs.failure = error?.message ?? ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the change the good, I wonder if we have to treat this as potentially breaking? @nodejs/test_runner @nodejs/tsc any thoughts? I'm fine landing as a semver-patch but want to be sure.
MoLow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. also not sure rgarding semver-major vs patch
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #59685 +/- ##
==========================================
- Coverage 89.91% 89.87% -0.04%
==========================================
Files 667 667
Lines 196600 196855 +255
Branches 38601 38646 +45
==========================================
+ Hits 176780 176933 +153
- Misses 12269 12359 +90
- Partials 7551 7563 +12
🚀 New features to boost your workflow:
|
Fixes: #59593
This PR aligns the Node.js test runner’s JUnit XML output with the official JUnit specification by removing the non-standard failure attribute from elements and replacing it with a proper nested tag.
Changes
• Remove non-compliant failure attribute from elements
• Update snapshot tests accordingly
Background
The JUnit XML specification mandates that failure details must be represented via a nested (child)
<failure>element inside a<testcase>, rather than as an attribute.However, the Node.js test runner previously:
• Incorrectly embedded the failure message as a failure="..." attribute on , which is not spec-compliant
• This caused compatibility issues with tools like GitLab, Jenkins, or any CI/CD pipeline that parses JUnit reports strictly
Before:
After:
Testing