test: remove CommonJS loader from stack traces by GeoffreyBooth · Pull Request #44197 · nodejs/node · GitHub

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 respect Error.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

Not sure why this change is needed?

I don’t understand why this output changed either.

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?

Suggested change

      
                assert.match(stderr, /Thrown at:/);
          
                assert.match(stderr, /^Thrown at:$/m);

See the explanation in the PR description.

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.

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.

I think it's meant to validate the format of the trace, where else is this tested otherwise?

Some of the .out files contain lines like [... lines matching original stack trace ...]

This comes from util.inspect on some errors, I believe.

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?

Looks like I was wrong. It's specific to errors from event emitters:

node/lib/events.js

Line 447 in 3711503

' [... lines matching original stack trace ...]');

But yeah, the actual stack trace value contains that text.

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?

consider using assert.snapshot()

The stack trace contains unstable parts (absolute paths, line numbers), so we can't just snapshot the values.

👍🏻 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.

@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.

Sure will do

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)

If it's the only test checking this line, maybe we should also move it to a JS test with RegExp.

CI: https://ci.nodejs.org/job/node-test-pull-request/46027/

Landed in d6e626d

PR-URL: #44197 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Michaël Zasso <targos@protonmail.com>

After resolving merge conflicts in "out" messages, this is not landing clean. Adding the backport label

Adding the backport label

Is this something for me to tackle, or someone else?

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.

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?

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 <moshe@atlow.co.il> Reviewed-By: Michaël Zasso <targos@protonmail.com>

PR-URL: #44197 Backport-PR-URL: #46535 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Michaël Zasso <targos@protonmail.com>

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. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.walbeck.it/mwalbeck/docker-cyberchef/pulls/187 Co-authored-by: renovate-bot <bot@walbeck.it> Co-committed-by: renovate-bot <bot@walbeck.it>

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. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.walbeck.it/mwalbeck/docker-jellyfin-livestream/pulls/243 Co-authored-by: renovate-bot <bot@walbeck.it> Co-committed-by: renovate-bot <bot@walbeck.it>

GeoffreyBooth added the test Issues and PRs related to the tests. label Aug 10, 2022

GeoffreyBooth requested review from aduh95 and targos August 10, 2022 06:01

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 10, 2022

targos reviewed Aug 10, 2022

View reviewed changes

Comment thread test/errors/throw-undefined-traced.mjs Outdated

targos reviewed Aug 10, 2022

View reviewed changes

Comment thread test/errors/throw-error-with-getter-throw-traced.mjs Outdated

targos reviewed Aug 10, 2022

View reviewed changes

aduh95 reviewed Aug 10, 2022

View reviewed changes

Comment thread test/errors/throw-null-traced.mjs Outdated

targos reviewed Aug 10, 2022

View reviewed changes

Comment thread test/errors/throw-error-with-getter-throw-traced.mjs Outdated

GeoffreyBooth force-pushed the tests-stack-traces branch from 3e255df to 0bbdb35 Compare August 10, 2022 20:08

test: remove cjs loader from stack traces

3de4e2a

GeoffreyBooth force-pushed the tests-stack-traces branch from 0bbdb35 to 3de4e2a Compare August 10, 2022 20:09

GeoffreyBooth requested review from aduh95 and targos August 10, 2022 20:09

MoLow reviewed Aug 14, 2022

View reviewed changes

MoLow approved these changes Aug 14, 2022

View reviewed changes

targos reviewed Aug 14, 2022

View reviewed changes

targos approved these changes Aug 14, 2022

View reviewed changes

GeoffreyBooth added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Aug 14, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2022

GeoffreyBooth added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 14, 2022

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 14, 2022

nodejs-github-bot merged commit d6e626d into nodejs:main Aug 14, 2022

GeoffreyBooth deleted the tests-stack-traces branch August 14, 2022 22:59

danielleadams pushed a commit that referenced this pull request Aug 16, 2022

test: remove cjs loader from stack traces

a82e768

PR-URL: #44197 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Michaël Zasso <targos@protonmail.com>

danielleadams mentioned this pull request Aug 16, 2022

v18.8.0 release proposal #44246

Closed

ruyadorno pushed a commit that referenced this pull request Aug 23, 2022

test: remove cjs loader from stack traces

f5e2f6c

PR-URL: #44197 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Michaël Zasso <targos@protonmail.com>

ruyadorno mentioned this pull request Aug 23, 2022

v18.8.0 release proposal #44353

Merged

juanarbol added the backport-requested-v16.x label Sep 29, 2022

GeoffreyBooth mentioned this pull request Feb 7, 2023

[v16.x backport] test: remove cjs loader from stack traces #46535

Closed

GeoffreyBooth added backport-open-v16.x and removed backport-requested-v16.x labels Feb 7, 2023

GeoffreyBooth added a commit to GeoffreyBooth/node that referenced this pull request Feb 7, 2023

test: remove cjs loader from stack traces

200f270

PR-URL: nodejs#44197 Backport-PR-URL: nodejs#46535 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Michaël Zasso <targos@protonmail.com>

BethGriggs pushed a commit that referenced this pull request Mar 27, 2023

test: remove cjs loader from stack traces

fc47d58

PR-URL: #44197 Backport-PR-URL: #46535 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Michaël Zasso <targos@protonmail.com>

BethGriggs mentioned this pull request Mar 27, 2023

v16.20.0 proposal #47272

Merged

GeoffreyBooth mentioned this pull request Apr 3, 2023

test: migrate test runner message tests to snapshot #47392

Merged

GeoffreyBooth mentioned this pull request Dec 11, 2023

lib: add diagnostics_channel events to module loading #44340

Closed

+                      '--eval',
+                      `throw ${value};`,
+                    ]);
+                    assert.match(stderr, /^Thrown at:$/m);

                   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 ...]

	assert.match(stderr, /Thrown at:/);
	assert.match(stderr, /^Thrown at:$/m);

test: remove CommonJS loader from stack traces by GeoffreyBooth · Pull Request #44197 · nodejs/node · GitHub

Uh oh!

Uh oh!

test: remove CommonJS loader from stack traces - #44197

test: remove CommonJS loader from stack traces#44197nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom GeoffreyBooth:tests-stack-tracesGeoffreyBooth/node:tests-stack-tracesCopy head branch name to clipboard

Conversation

GeoffreyBooth commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MoLow Aug 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MoLow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 14, 2022

Uh oh!

nodejs-github-bot commented Aug 14, 2022

Uh oh!

juanarbol commented Sep 29, 2022

Uh oh!

GeoffreyBooth commented Sep 29, 2022

Uh oh!

juanarbol commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeoffreyBooth commented Sep 29, 2022

Uh oh!

juanarbol commented Sep 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

test: remove CommonJS loader from stack traces#44197
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
GeoffreyBooth:tests-stack-traces

GeoffreyBooth commented Aug 10, 2022 •
edited
Loading

aduh95 Aug 10, 2022 •
edited
Loading

aduh95 Aug 10, 2022 •
edited
Loading

MoLow Aug 14, 2022 •
edited
Loading

juanarbol commented Sep 29, 2022 •
edited
Loading