-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
test: remove CommonJS loader from stack traces #44197
test: remove CommonJS loader from stack traces #44197
Conversation
at Module._load (node:internal/modules/cjs/loader:*:*) | ||
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:*:*) | ||
at node:internal/main/run_main_module:*:* { | ||
[AggregateError: original] { |
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.
Not sure why this change is needed?
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.
I don’t understand why this output changed either.
};`, | ||
]); | ||
|
||
assert.match(stderr, /Thrown at:/); |
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.
It looks this test is now testing less than it was before, why not keeping it as a message/
test and add Error.stackTraceLimit
like the others?
assert.match(stderr, /Thrown at:/); | |
assert.match(stderr, /^Thrown at:$/m); |
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.
See the explanation in the PR description.
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.
An alternative would be to replace CJS-related stuff with *
, e.g.:
*:4
throw { // eslint-disable-line no-throw-literal
^
[object Object]
Thrown at:
at *throw_error_with_getter_throw_traced.js:*:*
at *
at *
at *
at *
at *
at *
Node.js *
It would break only if a future refactor made more (or less) intermediate calls, and the diff to make the test pass would be tiny.
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.
Then the test breaks as soon as the number of calls changes, since the trace length is less than 10.
I really don't think keeping this in the .out style is important; the test is meant to validate that a trace is printed, not the format of the trace.
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.
I think it's meant to validate the format of the trace, where else is this tested otherwise?
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.
Some of the .out files contain lines like [... lines matching original stack trace ...]
This comes from util.inspect
on some errors, I believe.
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.
This comes from
util.inspect
on some errors, I believe.
As in, the actual output contains that text, and it’s not just some weird regex replacement?
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 like I was wrong. It's specific to errors from event emitters:
Line 447 in 3711503
' [... lines matching original stack trace ...]'); |
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.
But yeah, the actual stack trace value contains that text.
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.
Okay, so then we’re back to the original issue, which is that I don’t know of any way to have *
(or something similar) match multiple lines in an .out
file. If there’s no way to do a multiline match, we can’t use .out
files to test for varying-length stack traces, which is a requirement for what I’m trying to achieve here.
I can change the new tests to test for more things in the output or use a huge regex that’s essentially a representation of what the .out
file gets parsed into by the Python script runner; the question there is really what is our intention? As in, for the test throw-error-with-getter-throw-traced.mjs
what in the output are we really trying to validate? My impression is that we’re checking for the existence of a stack trace, but not anything about its presentation. Regardless of the original intent when the test was written, what do we really want to validate for this feature today?
3e255df
to
0bbdb35
Compare
0bbdb35
to
3de4e2a
Compare
'--eval', | ||
`throw ${value};`, | ||
]); | ||
assert.match(stderr, /^Thrown at:$/m); |
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.
consider using assert.snapshot()
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.
The stack trace contains unstable parts (absolute paths, line numbers), so we can't just snapshot the values.
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.
👍🏻 we can assert.snapshot(stabilize(trace))
just like currently python does in message.py
, I was planning on doing that regardless of this pr, so I might just revisit it after this lands.
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.
@MoLow do you mind doing that in a follow up? I would love to get rid of the message/out tests altogether, as they're a pain to debug and don't follow the pattern of all the other tests.
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.
Sure will do
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.
side node - I was planning on refactoring this suite to use assert.snapshot
making it easier to update snapshots (and relay on a js test runner instead of python)
Emitted 'error' event at: | ||
at quux (*events_unhandled_error_common_trace.js:*:*) | ||
at Object.<anonymous> (*events_unhandled_error_common_trace.js:*:*) | ||
at Module._compile (node:internal/modules/cjs/loader:*:*) | ||
[... lines matching original stack trace ...] |
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.
If it's the only test checking this line, maybe we should also move it to a JS test with RegExp.
Landed in d6e626d |
PR-URL: #44197 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: #44197 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#44197 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
After resolving merge conflicts in "out" messages, this is not landing clean. Adding the backport label |
Is this something for me to tackle, or someone else? |
Well, I tried, hahaha. You could give it a try as well, or somebody else. |
I’m not sure if/when I’ll have time, but what are the steps? Open a PR against a particular branch? |
What I usually do is follow this guideline. |
PR-URL: nodejs#44197 Backport-PR-URL: nodejs#46535 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: nodejs#44197 Backport-PR-URL: nodejs#46535 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
PR-URL: #44197 Backport-PR-URL: #46535 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [node](https://github.com/nodejs/node) | stage | minor | `16.19.1-bullseye` -> `16.20.0-bullseye` | --- ### Release Notes <details> <summary>nodejs/node</summary> ### [`v16.20.0`](https://github.com/nodejs/node/releases/tag/v16.20.0): 2023-03-29, Version 16.20.0 'Gallium' (LTS), @​BethGriggs [Compare Source](nodejs/node@v16.19.1...v16.20.0) ##### Notable Changes - **deps:** - update undici to 5.20.0 (Node.js GitHub Bot) [#​46711](nodejs/node#46711) - update c-ares to 1.19.0 (Michaël Zasso) [#​46415](nodejs/node#46415) - upgrade npm to 8.19.4 (npm team) [#​46677](nodejs/node#46677) - update corepack to 0.17.0 (Node.js GitHub Bot) [#​46842](nodejs/node#46842) - **(SEMVER-MINOR)** **src**: add support for externally shared js builtins (Michael Dawson) [#​44376](nodejs/node#44376) ##### Commits - \[[`de6dd67790`](nodejs/node@de6dd67790)] - **crypto**: avoid hang when no algorithm available (Richard Lau) [#​46237](nodejs/node#46237) - \[[`4617512788`](nodejs/node@4617512788)] - **crypto**: ensure auth tag set for chacha20-poly1305 (Ben Noordhuis) [#​46185](nodejs/node#46185) - \[[`24972164fc`](nodejs/node@24972164fc)] - **deps**: update undici to 5.20.0 (Node.js GitHub Bot) [#​46711](nodejs/node#46711) - \[[`85f88c6a8d`](nodejs/node@85f88c6a8d)] - **deps**: V8: cherry-pick [`90be99f`](nodejs/node@90be99fab31c) (Michaël Zasso) [#​46646](nodejs/node#46646) - \[[`b4ebe6d47b`](nodejs/node@b4ebe6d47b)] - **deps**: update c-ares to 1.19.0 (Michaël Zasso) [#​46415](nodejs/node#46415) - \[[`56cbc7fdda`](nodejs/node@56cbc7fdda)] - **deps**: V8: cherry-pick [`c2792e5`](nodejs/node@c2792e58035f) (Jiawen Geng) [#​44961](nodejs/node#44961) - \[[`7af9bdb31e`](nodejs/node@7af9bdb31e)] - **deps**: upgrade npm to 8.19.4 (npm team) [#​46677](nodejs/node#46677) - \[[`962a7471b5`](nodejs/node@962a7471b5)] - **deps**: update corepack to 0.17.0 (Node.js GitHub Bot) [#​46842](nodejs/node#46842) - \[[`748bc96e35`](nodejs/node@748bc96e35)] - **deps**: update corepack to 0.16.0 (Node.js GitHub Bot) [#​46710](nodejs/node#46710) - \[[`a467782499`](nodejs/node@a467782499)] - **deps**: update corepack to 0.15.3 (Node.js GitHub Bot) [#​46037](nodejs/node#46037) - \[[`1913b6763d`](nodejs/node@1913b6763d)] - **deps**: update corepack to 0.15.2 (Node.js GitHub Bot) [#​45635](nodejs/node#45635) - \[[`809371a15f`](nodejs/node@809371a15f)] - **module**: require.resolve.paths returns null with node schema (MURAKAMI Masahiko) [#​45147](nodejs/node#45147) - \[[`086bb2f8d4`](nodejs/node@086bb2f8d4)] - ***Revert*** "**src**: let http2 streams end after session close" (Rich Trott) [#​46721](nodejs/node#46721) - \[[`6a01d39120`](nodejs/node@6a01d39120)] - **(SEMVER-MINOR)** **src**: add support for externally shared js builtins (Michael Dawson) [#​44376](nodejs/node#44376) - \[[`d081032a60`](nodejs/node@d081032a60)] - **test**: fix test-net-connect-reset-until-connected (Vita Batrla) [#​46781](nodejs/node#46781) - \[[`efe1be47ec`](nodejs/node@efe1be47ec)] - **test**: skip test depending on `overlapped-checker` when not available (Antoine du Hamel) [#​45015](nodejs/node#45015) - \[[`fc47d58abe`](nodejs/node@fc47d58abe)] - **test**: remove cjs loader from stack traces (Geoffrey Booth) [#​44197](nodejs/node#44197) - \[[`cf76d0790d`](nodejs/node@cf76d0790d)] - **test**: fix WPT title when no META title is present (Filip Skokan) [#​46804](nodejs/node#46804) - \[[`0d1485b924`](nodejs/node@0d1485b924)] - **test**: fix default WPT titles (Filip Skokan) [#​46778](nodejs/node#46778) - \[[`088e9cde3d`](nodejs/node@088e9cde3d)] - **test**: add WPTRunner support for variants and generating WPT reports (Filip Skokan) [#​46498](nodejs/node#46498) - \[[`908c4dff44`](nodejs/node@908c4dff44)] - **test**: mark test-crypto-key-objects flaky on Linux (Richard Lau) [#​46684](nodejs/node#46684) - \[[`768e56227e`](nodejs/node@768e56227e)] - **tools**: make `utils.SearchFiles` deterministic (Bruno Pitrus) [#​44496](nodejs/node#44496) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4yMy4zIiwidXBkYXRlZEluVmVyIjoiMzUuMjMuMyJ9--> Reviewed-on: https://git.walbeck.it/mwalbeck/docker-cyberchef/pulls/187 Co-authored-by: renovate-bot <[email protected]> Co-committed-by: renovate-bot <[email protected]>
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [node](https://github.com/nodejs/node) | stage | minor | `16.19.1-bullseye-slim` -> `16.20.0-bullseye-slim` | --- ### Release Notes <details> <summary>nodejs/node</summary> ### [`v16.20.0`](https://github.com/nodejs/node/releases/tag/v16.20.0): 2023-03-29, Version 16.20.0 'Gallium' (LTS), @​BethGriggs [Compare Source](nodejs/node@v16.19.1...v16.20.0) ##### Notable Changes - **deps:** - update undici to 5.20.0 (Node.js GitHub Bot) [#​46711](nodejs/node#46711) - update c-ares to 1.19.0 (Michaël Zasso) [#​46415](nodejs/node#46415) - upgrade npm to 8.19.4 (npm team) [#​46677](nodejs/node#46677) - update corepack to 0.17.0 (Node.js GitHub Bot) [#​46842](nodejs/node#46842) - **(SEMVER-MINOR)** **src**: add support for externally shared js builtins (Michael Dawson) [#​44376](nodejs/node#44376) ##### Commits - \[[`de6dd67790`](nodejs/node@de6dd67790)] - **crypto**: avoid hang when no algorithm available (Richard Lau) [#​46237](nodejs/node#46237) - \[[`4617512788`](nodejs/node@4617512788)] - **crypto**: ensure auth tag set for chacha20-poly1305 (Ben Noordhuis) [#​46185](nodejs/node#46185) - \[[`24972164fc`](nodejs/node@24972164fc)] - **deps**: update undici to 5.20.0 (Node.js GitHub Bot) [#​46711](nodejs/node#46711) - \[[`85f88c6a8d`](nodejs/node@85f88c6a8d)] - **deps**: V8: cherry-pick [`90be99f`](nodejs/node@90be99fab31c) (Michaël Zasso) [#​46646](nodejs/node#46646) - \[[`b4ebe6d47b`](nodejs/node@b4ebe6d47b)] - **deps**: update c-ares to 1.19.0 (Michaël Zasso) [#​46415](nodejs/node#46415) - \[[`56cbc7fdda`](nodejs/node@56cbc7fdda)] - **deps**: V8: cherry-pick [`c2792e5`](nodejs/node@c2792e58035f) (Jiawen Geng) [#​44961](nodejs/node#44961) - \[[`7af9bdb31e`](nodejs/node@7af9bdb31e)] - **deps**: upgrade npm to 8.19.4 (npm team) [#​46677](nodejs/node#46677) - \[[`962a7471b5`](nodejs/node@962a7471b5)] - **deps**: update corepack to 0.17.0 (Node.js GitHub Bot) [#​46842](nodejs/node#46842) - \[[`748bc96e35`](nodejs/node@748bc96e35)] - **deps**: update corepack to 0.16.0 (Node.js GitHub Bot) [#​46710](nodejs/node#46710) - \[[`a467782499`](nodejs/node@a467782499)] - **deps**: update corepack to 0.15.3 (Node.js GitHub Bot) [#​46037](nodejs/node#46037) - \[[`1913b6763d`](nodejs/node@1913b6763d)] - **deps**: update corepack to 0.15.2 (Node.js GitHub Bot) [#​45635](nodejs/node#45635) - \[[`809371a15f`](nodejs/node@809371a15f)] - **module**: require.resolve.paths returns null with node schema (MURAKAMI Masahiko) [#​45147](nodejs/node#45147) - \[[`086bb2f8d4`](nodejs/node@086bb2f8d4)] - ***Revert*** "**src**: let http2 streams end after session close" (Rich Trott) [#​46721](nodejs/node#46721) - \[[`6a01d39120`](nodejs/node@6a01d39120)] - **(SEMVER-MINOR)** **src**: add support for externally shared js builtins (Michael Dawson) [#​44376](nodejs/node#44376) - \[[`d081032a60`](nodejs/node@d081032a60)] - **test**: fix test-net-connect-reset-until-connected (Vita Batrla) [#​46781](nodejs/node#46781) - \[[`efe1be47ec`](nodejs/node@efe1be47ec)] - **test**: skip test depending on `overlapped-checker` when not available (Antoine du Hamel) [#​45015](nodejs/node#45015) - \[[`fc47d58abe`](nodejs/node@fc47d58abe)] - **test**: remove cjs loader from stack traces (Geoffrey Booth) [#​44197](nodejs/node#44197) - \[[`cf76d0790d`](nodejs/node@cf76d0790d)] - **test**: fix WPT title when no META title is present (Filip Skokan) [#​46804](nodejs/node#46804) - \[[`0d1485b924`](nodejs/node@0d1485b924)] - **test**: fix default WPT titles (Filip Skokan) [#​46778](nodejs/node#46778) - \[[`088e9cde3d`](nodejs/node@088e9cde3d)] - **test**: add WPTRunner support for variants and generating WPT reports (Filip Skokan) [#​46498](nodejs/node#46498) - \[[`908c4dff44`](nodejs/node@908c4dff44)] - **test**: mark test-crypto-key-objects flaky on Linux (Richard Lau) [#​46684](nodejs/node#46684) - \[[`768e56227e`](nodejs/node@768e56227e)] - **tools**: make `utils.SearchFiles` deterministic (Bruno Pitrus) [#​44496](nodejs/node#44496) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4yMy4zIiwidXBkYXRlZEluVmVyIjoiMzUuMjMuMyJ9--> Reviewed-on: https://git.walbeck.it/mwalbeck/docker-jellyfin-livestream/pulls/243 Co-authored-by: renovate-bot <[email protected]> Co-committed-by: renovate-bot <[email protected]>
Partially resolves #44168.
This PR refactors all the tests that are verifying a particular stack trace that includes the CommonJS loader. None of these tests are modules tests, and are asserting a longer stack trace than is necessary for the systems that the tests are intended for. These excessive traces make refactoring the CommonJS loader difficult, as any changes that affect the functions called within the loader break these unrelated tests.
For the most part I used
Error.stackTraceLimit
to request only the number of trace frames that were relevant to the particular test, excluding the CommonJS loader from the output. This approach didn’t work for the tests that were testing--trace-uncaught
, as that flag doesn’t seem to respectError.stackTraceLimit
; I rewrote those tests to use the child process pattern that many of the modules tests use, where we’re explicit about what we’re looking for in the output.@targos can you please verify that I edited
error_with_nul.out
correctly and didn’t break the file somehow by saving it wrong? cc @nodejs/testing