Skip to content
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

lib: source maps filter null prefix #42522

Merged
merged 1 commit into from
Apr 3, 2022
Merged

lib: source maps filter null prefix #42522

merged 1 commit into from
Apr 3, 2022

Conversation

fabiancook
Copy link
Contributor

Fixes: #42417

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 30, 2022
fabiancook added a commit to virtualstate/focus that referenced this pull request Mar 30, 2022
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Looks good, can you please add a test?

@fabiancook
Copy link
Contributor Author

Added tests.

Before

AssertionError [ERR_ASSERTION]: The input did not match the regular expression /at Throw \([^)]+throw-async\.ts:4:9\)/. Input:

'/Users/fabiancook/src/virtualstate/node/test/fixtures/source-map/throw-async.ts:4\n' +
  '  throw new Error(message)\n' +
  '        ^\n' +
  '\n' +
  'Error: Message 0.3702386924909997\n' +
  '    at null.Throw (/Users/fabiancook/src/virtualstate/node/test/fixtures/source-map/throw-async.ts:4:9)\n' +
  '    at null.<anonymous> (/Users/fabiancook/src/virtualstate/node/test/fixtures/source-map/throw-async.ts:7:7)\n' +
  '    at ModuleJob.run (node:internal/modules/esm/module_job:198:25)\n' +
  '    at async Promise.all (index 0)\n' +
  '    at async ESMLoader.import (node:internal/modules/esm/loader:409:24)\n' +
  '    at async loadESM (node:internal/process/esm_loader:85:5)\n' +
  '    at async handleMainPromise (node:internal/modules/run_main:61:12)\n' +
  '\n' +
  'Node.js v18.0.0-pre\n'

    at Object.<anonymous> (/Users/fabiancook/src/virtualstate/node/test/parallel/test-source-map-enable.js:359:10)
    at Module._compile (node:internal/modules/cjs/loader:1106:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1160:10)
    at Module.load (node:internal/modules/cjs/loader:982:32)
    at Function.Module._load (node:internal/modules/cjs/loader:829:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47 {
  generatedMessage: true,
  code: 'ERR_ASSERTION',
  actual: '/Users/fabiancook/src/virtualstate/node/test/fixtures/source-map/throw-async.ts:4\n' +
    '  throw new Error(message)\n' +
    '        ^\n' +
    '\n' +
    'Error: Message 0.3702386924909997\n' +
    '    at null.Throw (/Users/fabiancook/src/virtualstate/node/test/fixtures/source-map/throw-async.ts:4:9)\n' +
    '    at null.<anonymous> (/Users/fabiancook/src/virtualstate/node/test/fixtures/source-map/throw-async.ts:7:7)\n' +
    '    at ModuleJob.run (node:internal/modules/esm/module_job:198:25)\n' +
    '    at async Promise.all (index 0)\n' +
    '    at async ESMLoader.import (node:internal/modules/esm/loader:409:24)\n' +
    '    at async loadESM (node:internal/process/esm_loader:85:5)\n' +
    '    at async handleMainPromise (node:internal/modules/run_main:61:12)\n' +
    '\n' +
    'Node.js v18.0.0-pre\n',
  expected: /at Throw \([^)]+throw-async\.ts:4:9\)/,
  operator: 'match'
}

After

 ✗ tools/test.py test/parallel/test-source-map-enable.js
[00:01|% 100|+   1|-   0]: Done

@fabiancook
Copy link
Contributor Author

I see a test failed, is there any way we could... re run it?

=== release test-vm-timeout-escape-promise-module ===
Path: parallel/test-vm-timeout-escape-promise-module
Error: --- stderr ---
(node:122116) ExperimentalWarning: VM Modules is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)


#
# Fatal error in , line 0
# Check failed: module->status() == kErrored.
#
#
#
#FailureMessage Object: 0x7ffec359bf00
 1: 0xbb6664 node::DumpBacktrace(_IO_FILE*) [out/Release/node]
 2: 0xf16753  [out/Release/node]
 3: 0x3f14201 V8_Fatal(char const*, ...) [out/Release/node]
 4: 0x205d43e v8::internal::SourceTextModule::Evaluate(v8::internal::Isolate*, v8::internal::Handle<v8::internal::SourceTextModule>) [out/Release/node]
 5: 0x1f99860 v8::internal::Module::Evaluate(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Module>) [out/Release/node]
 6: 0x1326c83 v8::Module::Evaluate(v8::Local<v8::Context>) [out/Release/node]
 7: 0xcb7b05 node::loader::ModuleWrap::Evaluate(v8::FunctionCallbackInfo<v8::Value> const&) [out/Release/node]
 8: 0x1466372 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [out/Release/node]
 9: 0x146518d  [out/Release/node]
10: 0x1462fd0 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [out/Release/node]
11: 0x2ee7df9  [out/Release/node]
Command: out/Release/node --experimental-vm-modules /home/runner/work/node/node/test/parallel/test-vm-timeout-escape-promise-module.js
--- CRASHED (Signal: 5) ---

===
=== 1 tests failed
=== 1 tests CRASHED
===
make[1]: *** [Makefile:528: test-ci] Error 1
make: *** [Makefile:557: run-ci] Error 2
Error: Process completed with exit code 2.

@benjamingr
Copy link
Member

I see a test failed, is there any way we could... re run it?

That looks flaky, I'll re-run and trigger a full CI

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 31, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 31, 2022
@nodejs-github-bot

This comment was marked as outdated.

@fabiancook
Copy link
Contributor Author

The same tests are failing

=== release test-vm-timeout-escape-promise-module ===
Path: parallel/test-vm-timeout-escape-promise-module
Error: --- stderr ---
(node:121410) ExperimentalWarning: VM Modules is an experimental feature. This feature could change at any time
(Use `node --trace-warnings ...` to show where the warning was created)


#
# Fatal error in , line 0
# Check failed: module->status() == kErrored.
#
#
#
#FailureMessage Object: 0x7fff5fc799a0
 1: 0xbb6664 node::DumpBacktrace(_IO_FILE*) [out/Release/node]
 2: 0xf16753  [out/Release/node]
 3: 0x3f14201 V8_Fatal(char const*, ...) [out/Release/node]
 4: 0x205d43e v8::internal::SourceTextModule::Evaluate(v8::internal::Isolate*, v8::internal::Handle<v8::internal::SourceTextModule>) [out/Release/node]
 5: 0x1f99860 v8::internal::Module::Evaluate(v8::internal::Isolate*, v8::internal::Handle<v8::internal::Module>) [out/Release/node]
 6: 0x1326c83 v8::Module::Evaluate(v8::Local<v8::Context>) [out/Release/node]
 7: 0xcb7b05 node::loader::ModuleWrap::Evaluate(v8::FunctionCallbackInfo<v8::Value> const&) [out/Release/node]
 8: 0x1466372 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [out/Release/node]
 9: 0x146518d  [out/Release/node]
10: 0x1462fd0 v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [out/Release/node]
11: 0x2ee7df9  [out/Release/node]
Command: out/Release/node --experimental-vm-modules /home/runner/work/node/node/test/parallel/test-vm-timeout-escape-promise-module.js
--- CRASHED (Signal: 5) ---

===
=== 1 tests failed
=== 1 tests CRASHED
===
make[1]: *** [Makefile:528: test-ci] Error 1
make: *** [Makefile:557: run-ci] Error 2
Error: Process completed with exit code 2.

@nodejs-github-bot
Copy link
Collaborator

@fabiancook
Copy link
Contributor Author

I can see the tests say passed! Hopefully it gets marked as passed too

@benjamingr
Copy link
Member

Hopefully it gets marked as passed too

It did, sorry for the CI experience Node.js has a lot of tests and some of them break in some environments in hard-to-debug cases.

@benjamingr
Copy link
Member

@bcoe any chance this could get a review from you too?

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2022
@nodejs-github-bot nodejs-github-bot merged commit 154fa41 into nodejs:master Apr 3, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 154fa41

@benjamingr
Copy link
Member

Congrats on your first commit in core @fabiancook and thanks for the fix 🙏 🎉


// To recreate:
//
// npx tsc --module esnext --target es2017 --outDir test/fixtures/source-map --sourceMap test/fixtures/source-map/throw-async.ts
Copy link
Member

Choose a reason for hiding this comment

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

If the target is es2018 or higher tsc will not down-level transpile async functions - so that'd how you'd test the thing

juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
Fixes: nodejs#42417

PR-URL: nodejs#42522
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Apr 5, 2022
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
Fixes: #42417

PR-URL: #42522
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
Fixes: nodejs#42417

PR-URL: nodejs#42522
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
Fixes: #42417

PR-URL: #42522
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
Fixes: #42417

PR-URL: #42522
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2022
Fixes: #42417

PR-URL: #42522
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Jul 31, 2022
Fixes: #42417

PR-URL: #42522
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Fixes: nodejs/node#42417

PR-URL: nodejs/node#42522
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error stacks in log includes null.thing instead of just thing
4 participants