Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

test: fix edge snapshot stack traces#49659

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

GeoffreyBooth
Copy link
Member

@GeoffreyBoothGeoffreyBooth commentedSep 15, 2023•
edited
Loading

Working on#49657, I discovered that some of the more unusual snapshot tests had stack traces that include specifics of the module loader internals. (Seehttps://github.com/nodejs/node/actions/runs/6191368639/job/16809521748?pr=49657.) Refactoring the modules code shouldn’t break these unrelated tests; these snapshots should be like the others that exclude detailed stack traces of modules internals. These two specific cases probably slipped through because they’re edge cases, having to do with source maps and colored stack trace output. This PR updates the tests and snapshots so that future modules changes don’t break these tests. @nodejs/test_runner @nodejs/testing

bricss reacted with thumbs up emoji
@GeoffreyBoothGeoffreyBooth added testIssues and PRs related to the tests. fast-trackPRs that do not need to wait for 48 hours to land. labelsSep 15, 2023
@github-actions

This comment was marked as outdated.

@nodejs-github-botnodejs-github-bot added the needs-ciPRs that need a full CI run. labelSep 15, 2023
@aduh95
Copy link
Contributor

IMO this is not a good candidate for fast-track, it's not a trivial change.

@aduh95aduh95 removed the fast-trackPRs that do not need to wait for 48 hours to land. labelSep 15, 2023
@GeoffreyBooth
Copy link
MemberAuthor

IMO this is not a good candidate for fast-track, it’s not a trivial change.

Well that’s fine, but then#49657 can’t land fast-track either because it depends on this.

@anonriganonrig added author readyPRs that have at least one approval, no pending requests for changes, and a CI started. request-ciAdd this label to start a Jenkins CI on a PR. labelsSep 17, 2023
@github-actionsgithub-actionsbot removed the request-ciAdd this label to start a Jenkins CI on a PR. labelSep 17, 2023
@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBoothGeoffreyBooth removed the author readyPRs that have at least one approval, no pending requests for changes, and a CI started. labelSep 17, 2023
@GeoffreyBooth
Copy link
MemberAuthor

I want to give@MoLow a chance to review this, it can wait a few more days.

Copy link
Member

@MoLowMoLow left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBoothGeoffreyBooth added author readyPRs that have at least one approval, no pending requests for changes, and a CI started. commit-queueAdd this label to land a pull request using GitHub Actions. commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one. labelsSep 17, 2023
@nodejs-github-botnodejs-github-bot added commit-queue-failedAn error occurred while landing this pull request using GitHub Actions. and removed commit-queueAdd this label to land a pull request using GitHub Actions. labelsSep 17, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/49659✔  Done loading data for nodejs/node/pull/49659----------------------------------- PR info ------------------------------------Title      test: fix edge snapshot stack traces (#49659)   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!Branch     GeoffreyBooth:fix-snapshot-stack-traces -> nodejs:mainLabels     test, author ready, needs-ci, commit-queue-squashCommits    1 - test: fix edge snapshot stack tracesCommitters 1 - Geoffrey Booth PR-URL: https://github.com/nodejs/node/pull/49659Reviewed-By: James M Snell Reviewed-By: Yagiz Nizipli Reviewed-By: Moshe Atlow ------------------------------ Generated metadata ------------------------------PR-URL: https://github.com/nodejs/node/pull/49659Reviewed-By: James M Snell Reviewed-By: Yagiz Nizipli Reviewed-By: Moshe Atlow --------------------------------------------------------------------------------   ℹ  This PR was created on Fri, 15 Sep 2023 05:07:36 GMT   ✔  Approvals: 3   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/49659#pullrequestreview-1629983258   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/49659#pullrequestreview-1630085441   ✔  - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/49659#pullrequestreview-1630114315   ✘  Last GitHub CI failed   ℹ  Last Full PR CI on 2023-09-17T18:55:39Z: https://ci.nodejs.org/job/node-test-pull-request/54010/- Querying data for job/node-test-pull-request/54010/   ✔  Last Jenkins CI successful--------------------------------------------------------------------------------   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/6215579253

@nodejs-github-bot
Copy link
Collaborator

@GeoffreyBoothGeoffreyBooth added commit-queueAdd this label to land a pull request using GitHub Actions. and removed commit-queue-failedAn error occurred while landing this pull request using GitHub Actions. labelsSep 17, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queueAdd this label to land a pull request using GitHub Actions. labelSep 17, 2023
@GeoffreyBoothGeoffreyBooth deleted the fix-snapshot-stack-traces branchOctober 17, 2023 02:52
codebytere pushed a commit to codebytere/node that referenced this pull requestOct 18, 2023
PR-URL:nodejs#49659Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
codebytere pushed a commit to codebytere/node that referenced this pull requestOct 18, 2023
PR-URL:nodejs#49659Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
alexfernandez pushed a commit to alexfernandez/node that referenced this pull requestNov 1, 2023
PR-URL:nodejs#49659Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit to targos/node that referenced this pull requestNov 11, 2023
PR-URL:nodejs#49659Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit that referenced this pull requestNov 11, 2023
PR-URL:#49659Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
targos pushed a commit to targos/node that referenced this pull requestNov 11, 2023
PR-URL:nodejs#49659Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
@targostargos mentioned this pull requestNov 12, 2023
@targostargos added backported-to-v20.xPRs backported to the v20.x-staging branch. and removed backport-open-v20.xIndicate that the PR has an open backport labelsNov 12, 2023
codebytere added a commit to electron/electron that referenced this pull requestNov 17, 2023
codebytere added a commit to electron/electron that referenced this pull requestNov 21, 2023
codebytere added a commit to electron/electron that referenced this pull requestNov 22, 2023
targos pushed a commit that referenced this pull requestNov 23, 2023
PR-URL:#49659Backport-PR-URL:#50669Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
codebytere added a commit to electron/electron that referenced this pull requestNov 28, 2023
@targostargos mentioned this pull requestNov 28, 2023
codebytere added a commit to electron/electron that referenced this pull requestNov 29, 2023
jkleinsc pushed a commit to electron/electron that referenced this pull requestNov 30, 2023
* chore: upgrade to Node.js v20* src: allow embedders to override NODE_MODULE_VERSIONnodejs/node#49279* src: fix missing trailing ,nodejs/node#46909* src,tools: initialize cppgcnodejs/node#45704* tools: allow passing absolute path of config.gypi in js2cnodejs/node#49162* tools: port js2c.py to C++nodejs/node#46997* doc,lib: disambiguate the old term, NativeModulenodejs/node#45673* chore: fixup Node.js BSSL tests*nodejs/node#49492*nodejs/node#44498* deps: upgrade to libuv 1.45.0nodejs/node#48078* deps: update V8 to 10.7nodejs/node#44741* test: use gcUntil() in test-v8-serialize-leaknodejs/node#49168* module: make CJS load from ESM loadernodejs/node#47999* src: make BuiltinLoader threadsafe and non-globalnodejs/node#45942* chore: address changes to CJS/ESM loading* module: make CJS load from ESM loader (nodejs/node#47999)* lib: improve esm resolve performance (nodejs/node#46652)* bootstrap: optimize modules loaded in the built-in snapshotnodejs/node#45849* test: mark test-runner-output as flakynodejs/node#49854* lib: lazy-load deps in modules/run_main.jsnodejs/node#45849* url: use private properties for brand checknodejs/node#46904* test: refactor `test-node-output-errors`nodejs/node#48992* assert: deprecate callTrackernodejs/node#47740* src: cast v8::Object::GetInternalField() return value to v8::Valuenodejs/node#48943* test: adapt test-v8-stats for V8 updatenodejs/node#45230* tls: ensure TLS Sockets are closed if the underlying wrapclosesnodejs/node#49327* test: deflake test-tls-socket-closenodejs/node#49575* net: fix crash due to simultaneous close/shutdown on JS Stream Socketsnodejs/node#49400* net: use asserts in JS Socket Stream to catch races in futurenodejs/node#49400* lib: fix BroadcastChannel initialization locationnodejs/node#46864* src: create BaseObject with node::Realmnodejs/node#44348* src: implement DataQueue and non-memory resident Blobnodejs/node#45258* sea: add support for V8 bytecode-only cachingnodejs/node#48191* chore: fixup patch indices* gyp: put filenames in variablesnodejs/node#46965* build: modify js2c.py into GN executable* fix: (WIP) handle string replacement of fs -> original-fs* [v20.x] backport vm-related memoryfixesnodejs/node#49874* src: make BuiltinLoader threadsafe and non-globalnodejs/node#45942* src: avoid copying string in fs_permissionnodejs/node#47746* look upon my works ye mightyand dispair* chore: patch cleanup* [api] Remove AllCan Read/Writehttps://chromium-review.googlesource.com/c/v8/v8/+/5006387* fix: missing include for NODE_EXTERN* chore: fixup patch indices* fix: fail properly when js2c fails in Node.js* build: fix js2c root_gen_dir* fix: lib/fs.js -> lib/original-fs.js* build: fix original-fs file xforms* fixup! module: make CJS load from ESM loader* build: get rid of CppHeap for now* build: add patch to prevent extra fs lookup on esm load* build: greatly simplify js2c modificationsMoves our original-fs modifications back into a super simple python script action, wires up the output of that action into our call to js2c* chore: update to handle moved internal/modules/helpers file* test: update @types/node test* feat: enable preventing cppgc heap creation* feat: optionally prevent calling V8::EnableWebAssemblyTrapHandler* fix: no cppgc initialization in the renderer* gyp: put filenames in variablesnodejs/node#46965* test: disable single executable tests* fix: nan tests failing on node headers missing file* tls,http2: send fatal alert on ALPN mismatchnodejs/node#44031* test: disable snapshot tests*nodejs/node#47887*nodejs/node#49684*nodejs/node#44193* build: use deps/v8 for v8/toolsNode.js hard depends on these in their builtins* test: fix edge snapshot stack tracesnodejs/node#49659* build: remove js2c //base dep* build: use electron_js2c_toolchain to build node_js2c* fix: don't create SafeSet outside packageResolveFixes failure in parallel/test-require-delete-array-iterator:=== release test-require-delete-array-iterator ===Path: parallel/test-require-delete-array-iteratornode:internal/per_context/primordials:426    constructor(i) { super(i); } // eslint-disable-line no-useless-constructor                     ^TypeError: object is not iterable (cannot read property Symbol(Symbol.iterator))    at new Set (<anonymous>)    at new SafeSet (node:internal/per_context/primordials:426:22)* fix: failing crashReporter tests on LinuxThese were failing because our change from node::InitializeNodeWithArgs tonode::InitializeOncePerProcess meant that we now inadvertently calledPlatformInit, which reset signal handling. This meant that our intentionalcrash function ElectronBindings::Crash no longer worked and the renderer processno longer crashed when process.crash() was called. We don't want to use Node.js'default signal handling in the renderer process, so we disable it by passingkNoDefaultSignalHandling to node::InitializeOncePerProcess.* build: only create cppgc heap on non-32 bit platforms* chore: clean up util:CompileAndCall* src: fix compatility with upcoming V8 12.1 APIsnodejs/node#50709* fix: use thread_local BuiltinLoader* chore: fixup v8 patch indices---------Co-authored-by: Keeley Hammond <vertedinde@electronjs.org>Co-authored-by: Samuel Attard <marshallofsound@electronjs.org>
codebytere added a commit to electron/electron that referenced this pull requestDec 8, 2023
codebytere added a commit to electron/electron that referenced this pull requestDec 8, 2023
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull requestDec 11, 2023
* chore: upgrade to Node.js v20* src: allow embedders to override NODE_MODULE_VERSIONnodejs/node#49279* src: fix missing trailing ,nodejs/node#46909* src,tools: initialize cppgcnodejs/node#45704* tools: allow passing absolute path of config.gypi in js2cnodejs/node#49162* tools: port js2c.py to C++nodejs/node#46997* doc,lib: disambiguate the old term, NativeModulenodejs/node#45673* chore: fixup Node.js BSSL tests*nodejs/node#49492*nodejs/node#44498* deps: upgrade to libuv 1.45.0nodejs/node#48078* deps: update V8 to 10.7nodejs/node#44741* test: use gcUntil() in test-v8-serialize-leaknodejs/node#49168* module: make CJS load from ESM loadernodejs/node#47999* src: make BuiltinLoader threadsafe and non-globalnodejs/node#45942* chore: address changes to CJS/ESM loading* module: make CJS load from ESM loader (nodejs/node#47999)* lib: improve esm resolve performance (nodejs/node#46652)* bootstrap: optimize modules loaded in the built-in snapshotnodejs/node#45849* test: mark test-runner-output as flakynodejs/node#49854* lib: lazy-load deps in modules/run_main.jsnodejs/node#45849* url: use private properties for brand checknodejs/node#46904* test: refactor `test-node-output-errors`nodejs/node#48992* assert: deprecate callTrackernodejs/node#47740* src: cast v8::Object::GetInternalField() return value to v8::Valuenodejs/node#48943* test: adapt test-v8-stats for V8 updatenodejs/node#45230* tls: ensure TLS Sockets are closed if the underlying wrapclosesnodejs/node#49327* test: deflake test-tls-socket-closenodejs/node#49575* net: fix crash due to simultaneous close/shutdown on JS Stream Socketsnodejs/node#49400* net: use asserts in JS Socket Stream to catch races in futurenodejs/node#49400* lib: fix BroadcastChannel initialization locationnodejs/node#46864* src: create BaseObject with node::Realmnodejs/node#44348* src: implement DataQueue and non-memory resident Blobnodejs/node#45258* sea: add support for V8 bytecode-only cachingnodejs/node#48191* chore: fixup patch indices* gyp: put filenames in variablesnodejs/node#46965* build: modify js2c.py into GN executable* fix: (WIP) handle string replacement of fs -> original-fs* [v20.x] backport vm-related memoryfixesnodejs/node#49874* src: make BuiltinLoader threadsafe and non-globalnodejs/node#45942* src: avoid copying string in fs_permissionnodejs/node#47746* look upon my works ye mightyand dispair* chore: patch cleanup* [api] Remove AllCan Read/Writehttps://chromium-review.googlesource.com/c/v8/v8/+/5006387* fix: missing include for NODE_EXTERN* chore: fixup patch indices* fix: fail properly when js2c fails in Node.js* build: fix js2c root_gen_dir* fix: lib/fs.js -> lib/original-fs.js* build: fix original-fs file xforms* fixup! module: make CJS load from ESM loader* build: get rid of CppHeap for now* build: add patch to prevent extra fs lookup on esm load* build: greatly simplify js2c modificationsMoves our original-fs modifications back into a super simple python script action, wires up the output of that action into our call to js2c* chore: update to handle moved internal/modules/helpers file* test: update @types/node test* feat: enable preventing cppgc heap creation* feat: optionally prevent calling V8::EnableWebAssemblyTrapHandler* fix: no cppgc initialization in the renderer* gyp: put filenames in variablesnodejs/node#46965* test: disable single executable tests* fix: nan tests failing on node headers missing file* tls,http2: send fatal alert on ALPN mismatchnodejs/node#44031* test: disable snapshot tests*nodejs/node#47887*nodejs/node#49684*nodejs/node#44193* build: use deps/v8 for v8/toolsNode.js hard depends on these in their builtins* test: fix edge snapshot stack tracesnodejs/node#49659* build: remove js2c //base dep* build: use electron_js2c_toolchain to build node_js2c* fix: don't create SafeSet outside packageResolveFixes failure in parallel/test-require-delete-array-iterator:=== release test-require-delete-array-iterator ===Path: parallel/test-require-delete-array-iteratornode:internal/per_context/primordials:426    constructor(i) { super(i); } // eslint-disable-line no-useless-constructor                     ^TypeError: object is not iterable (cannot read property Symbol(Symbol.iterator))    at new Set (<anonymous>)    at new SafeSet (node:internal/per_context/primordials:426:22)* fix: failing crashReporter tests on LinuxThese were failing because our change from node::InitializeNodeWithArgs tonode::InitializeOncePerProcess meant that we now inadvertently calledPlatformInit, which reset signal handling. This meant that our intentionalcrash function ElectronBindings::Crash no longer worked and the renderer processno longer crashed when process.crash() was called. We don't want to use Node.js'default signal handling in the renderer process, so we disable it by passingkNoDefaultSignalHandling to node::InitializeOncePerProcess.* build: only create cppgc heap on non-32 bit platforms* chore: clean up util:CompileAndCall* src: fix compatility with upcoming V8 12.1 APIsnodejs/node#50709* fix: use thread_local BuiltinLoader* chore: fixup v8 patch indices---------Co-authored-by: Keeley Hammond <vertedinde@electronjs.org>Co-authored-by: Samuel Attard <marshallofsound@electronjs.org>
codebytere added a commit to electron/electron that referenced this pull requestDec 11, 2023
* chore: bump node in DEPS to v20.10.0* chore: update feat_initialize_asar_support.patchno code changes; patch just needed an update due to nearby upstream changesXref:nodejs/node#49986* chore: update pass_all_globals_through_require.patchno manual changes; patch applied with fuzzXref:nodejs/node#49657* chore: update refactor_allow_embedder_overriding_of_internal_fs_callsXref:nodejs/node#49912no code changes; patch just needed an update due to nearby upstream changes* chore: update chore_allow_the_node_entrypoint_to_be_a_builtin_module.patchXref:nodejs/node#49986minor manual changes needed to sync with upstream change* update fix_expose_the_built-in_electron_module_via_the_esm_loader.patchXref:nodejs/node#50096Xref:nodejs/node#50314in lib/internal/modules/esm/load.js, update the code that checks for`format === 'electron'`. I'd like 👀 on thisXref:nodejs/node#49657add braces in lib/internal/modules/esm/translators.js to sync with upstream* fix: lazyload fs in esm loaders to apply asar patches*nodejs/node#50127*nodejs/node#50096* esm: jsdoc for modules codenodejs/node#49523* test: set test-cli-node-options as flakynodejs/node#50296* deps: update c-ares to 1.20.1nodejs/node#50082* esm: bypass CommonJS loader under --default-type=modulenodejs/node#49986* deps: update uvwasi to 0.0.19nodejs/node#49908* lib,test: do not hardcode Buffer.kMaxLengthnodejs/node#49876* crypto: account for disabled SharedArrayBuffernodejs/node#50034* test: fix edge snapshot stack tracesnodejs/node#49659* src: generate snapshot with --predictablenodejs/node#48749* chore: fixup patch indices* fs: throw errors from sync branches instead of separate implementationsnodejs/node#49913* crypto: ensure valid point on elliptic curve in SubtleCrypto.importKeynodejs/node#50234* esm: detect ESM syntax in ambiguous JavaScripnodejs/node#50096* fixup! test: fix edge snapshot stack traces* esm: unflag extensionless ES module JavaScript and Wasm in module scopenodejs/node#49974* [tagged-ptr] Arrowify objectshttps://chromium-review.googlesource.com/c/v8/v8/+/4705331---------Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>Co-authored-by: Charles Kerr <charles@charleskerr.com>Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull requestApr 25, 2024
PR-URL:nodejs/node#49659Backport-PR-URL:nodejs/node#50669Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
sercher added a commit to sercher/graaljs that referenced this pull requestApr 25, 2024
PR-URL:nodejs/node#49659Backport-PR-URL:nodejs/node#50669Reviewed-By: James M Snell <jasnell@gmail.com>Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@jasnelljasnelljasnell approved these changes

@anonriganonriganonrig approved these changes

@MoLowMoLowMoLow approved these changes

Assignees
No one assigned
Labels
author readyPRs that have at least one approval, no pending requests for changes, and a CI started.backported-to-v20.xPRs backported to the v20.x-staging branch.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

8 participants
@GeoffreyBooth@aduh95@nodejs-github-bot@ruyadorno@jasnell@anonrig@MoLow@targos
[8]ページ先頭
©2009-2025 Movatter.jp