-
-
Notifications
You must be signed in to change notification settings - Fork 33.8k
test_runner: fix isSkipped in JUnit reporter #59317
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
Conversation
The `isSkipped` function in the JUnit reporter was incorrectly checking for `node?.attrs.failures` instead of `node?.attrs.skipped`.
The junit reporter could crash with a TypeError if a test event was emitted without a `details` object. While `details` is common, its absence is a valid case that should not terminate the reporter.
|
Review requested:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #59317 +/- ##
==========================================
- Coverage 90.00% 89.36% -0.64%
==========================================
Files 649 655 +6
Lines 192180 192829 +649
Branches 37659 37493 -166
==========================================
- Hits 172970 172328 -642
- Misses 11830 13006 +1176
- Partials 7380 7495 +115 🚀 New features to boost your workflow:
|
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.
@mete0rfish how did the existing test work?
node/test/fixtures/test-runner/output/junit_reporter.js
Lines 6 to 11 in 6d8aa55
| spawn(process.execPath, | |
| [ | |
| '--no-warnings', '--test-reporter', 'junit', | |
| fixtures.path('test-runner/output/output.js'), | |
| ], | |
| { stdio: 'inherit' }); |
And what can be done so the test would catch such an error?
JakobJingleheimer
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.
Looks good but as Chemi mentioned, could you please update the test :)
The previous assertions for the JUnit reporter output were too strict about the order of XML attributes. The attribute order is not guaranteed and can vary, causing the test to fail. This commit updates the regular expressions to be independent of the attribute order, ensuring the test is more robust.
This commit removes trailing whitespace to fix a linting error identified by the `@stylistic/js/no-trailing-spaces` rule.
This commit adds explicit clarification to the Node.js threat model that path manipulation functions such as path.join() and path.normalize() trust their input. Issues related to these functions that rely on unsanitized input are not considered vulnerabilities requiring CVEs. PR-URL: nodejs#59262 Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#59267 Fixes: nodejs#59266 Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: theanarkh <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#59270 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: Filip Skokan <[email protected]>
PR-URL: nodejs#59273 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Harshitha K P <[email protected]>
PR-URL: nodejs#59299 Reviewed-By: Ruy Adorno <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
Notable changes: deps: * (SEMVER-MINOR) update amaro to 1.1.0 (Node.js GitHub Bot) nodejs#56350 doc: * add islandryu to collaborators (Shima Ryuhei) nodejs#58714 esm: * (SEMVER-MINOR) implement `import.meta.main` (Joe) nodejs#57804 fs: * (SEMVER-MINOR) allow correct handling of burst in fs-events with AsyncIterator (Philipp Dunkel) nodejs#58490 module: * (SEMVER-MINOR) remove experimental warning from type stripping (Marco Ippolito) nodejs#56350 * (SEMVER-MINOR) unflag `--experimental-strip-types` (Marco Ippolito) nodejs#56350 permission: * (SEMVER-MINOR) propagate permission model flags on spawn (Rafael Gonzaga) nodejs#58853 sqlite: * (SEMVER-MINOR) add support for `readBigInts` option in db connection level (Miguel Marcondes Filho) nodejs#58697 src,permission: * (SEMVER-MINOR) add support to `permission.has(addon)` (Rafael Gonzaga) nodejs#58951 url: * (SEMVER-MINOR) add `fileURLToPathBuffer` API (James M Snell) nodejs#58700 watch: * (SEMVER-MINOR) add `--watch-kill-signal` flag (Dario Piotrowicz) nodejs#58719 worker: * (SEMVER-MINOR) make `Worker` async disposable (James M Snell) nodejs#58385 PR-URL: nodejs#59256 Co-authored-by: Antoine du Hamel <[email protected]>
Notable changes:
cli:
* (SEMVER-MINOR) support `${pid}` placeholder in `--cpu-prof-name` (Haram Jeong) nodejs#59072
crypto:
* (SEMVER-MINOR) add `tls.setDefaultCACertificates()` (Joyee Cheung) nodejs#58822
deps:
* upgrade to openssl-3.5.1 (Node.js GitHub Bot) nodejs#59234
dns:
* (SEMVER-MINOR) support max timeout (theanarkh) nodejs#58440
doc:
* update the instruction on how to verify releases (Antoine du Hamel) nodejs#59113
esm:
* (SEMVER-MINOR) unflag `--experimental-wasm-modules` (Guy Bedford) nodejs#57038
http,https:
* (SEMVER-MINOR) add built-in proxy support in http/https.request and `Agent` (Joyee Cheung) nodejs#58980
net:
* (SEMVER-MINOR) update net.blocklist to allow file save and file management (alphaleadership) nodejs#58087
test:
* (SEMVER-MINOR) move http proxy tests to test/client-proxy (Joyee Cheung) nodejs#58980
worker:
* (SEMVER-MINOR) add web locks api (ishabi) nodejs#58666
PR-URL: nodejs#59257
PR-URL: nodejs#59280 Reviewed-By: Jason Zhang <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>
When a directory cannot be read due to permission issues, the async version of fs.glob() returns null from readdir(), while the sync version returns an empty array. This causes a TypeError when trying to access the 'length' property of null. PR-URL: nodejs#58674 Fixes: nodejs#58670 Fixes: nodejs#58276 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan-Arrowood <[email protected]> Reviewed-By: Juan José <[email protected]>
PR-URL: nodejs#58497 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs#59036 Fixes: nodejs#58852 Reviewed-By: Vladimir Morozov <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
The type name is determined by the constructor name of the receiver in a call site. PR-URL: nodejs#58976 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#59289 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
Ref nodejs#58535 Signed-off-by: Sebastian Beltran <[email protected]> PR-URL: nodejs#59293 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>
Co-authored-by: Luigi Pinca <[email protected]> PR-URL: nodejs#58948 Fixes: nodejs#58949 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Mattias Buelens <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]>
- Add typings for `async_context_frame`, `icu`, and `sea` bindings - Add a few missing exports on other bindings - Add a few missing primordials PR-URL: nodejs#59176 Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs#59295 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ruy Adorno <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Beth Griggs <[email protected]> Reviewed-By: Claudio Wunder <[email protected]>
Since we need to be able to use the openssl adapter provided by the ngtcp2 library, and because that adapter does not include any compile guards to ensure that OpenSSL 3.5 is being used and that the APIs are actually available, we need to add a compile time check for the openssl version in order to conditionally include the adapter to avoid build errors when using a shared openssl library that is not OpenSSL 3.5. Signed-off-by: James M Snell <[email protected]> PR-URL: nodejs#59249 Reviewed-By: Matteo Collina <[email protected]>
Refactors several `v.find(...) == v.end()` and `v.find(...) != v.end()` to use more expressive and readable C++20 `contains()` method. PR-URL: nodejs#59304 Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>
PR-URL: nodejs#59243 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Jason Zhang <[email protected]>
Adds optional dictionary support to zlib’s zstdCompress and zstdDecompress APIs. This enables better compression ratios when the dictionary matches expected input structure or content patterns. The implementation allows passing a `dictionary` buffer through the options object. Support was added to both streaming and convenience methods. Tests and documentation were also updated to reflect this new capability. Fixes: nodejs#59105 PR-URL: nodejs#59240 Reviewed-By: Anna Henningsen <[email protected]>
PR-URL: nodejs#59337 Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Ulises Gascón <[email protected]>
PR-URL: nodejs#59339 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]>
PR-URL: nodejs#59284 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: nodejs#59291 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: nodejs#59213 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#58253 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#59338 Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: nodejs#59343 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
Add missing preposition "by" to clarify how to change the global dispatcher. PR-URL: nodejs#59344 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Based on some recent confusion around the objection process for PRs, this commit adds some additional clarification to the collaborator guide. Specifically, it clarifies that: * Objections must be made in the PR itself * All objections are considered equal... no special additional weight is given to objections from TSC members. * When mistakes happen and a PR lands despite having an unresolved objection, any revert or fixup PR is subject to the same regular objection process, albeit with a callout that fast-tracking is possible if uncontroversial. PR-URL: nodejs#59096 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Ruy Adorno <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Stewart X Addison <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Fix OpenSSL version detection in `configure.py` when `pkg-config` is used to configure building with a shared OpenSSL library. PR-URL: nodejs#59353 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#59259 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#59381 Fixes: nodejs#59369 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Richard Lau <[email protected]>
PR-URL: nodejs#59381 Fixes: nodejs#59369 Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Minor warning about the use of FastOneByteString. PR-URL: nodejs#59275 Refs: cloudflare/workerd#4625 Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs#59356 Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Apple clang version number is not the same as the actual LLVM version PR-URL: nodejs#59358 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
For Clang >= 19.1.0, Xcode 16.3 or Visual Studio 17.13 is required. PR-URL: nodejs#59358 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Since these classes use virtual methods extensively, adding `final` should allow compilers to optimize accesses better. PR-URL: nodejs#59355 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Darshan Sen <[email protected]>
In illumos, madvise(3C) now takes `void *` for its first argument post-illumos#14418, but uses `caddr_t` pre-illumos#14418. This fix will detect if the illumos mman.h file in use is pre-or-post-illumos#14418 so builds can work either way. PR-URL: nodejs#58237 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#59371 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: nodejs#59371 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: nodejs#59278 Refs: https://github.com/nodejs/node/blob/main/doc/contributing/primordials.md#unsafe-array-iteration Reviewed-By: Jordan Harband <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]>
PR-URL: nodejs#59017 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Zeyu "Alex" Yang <[email protected]>
The
isSkippedfunction in the JUnit reporter was incorrectly checking fornode?.attrs.failuresinstead ofnode?.attrs.skipped.