lib: fix ERR_INVALID_ARG_TYPE with --enable-source-maps#63215
lib: fix ERR_INVALID_ARG_TYPE with --enable-source-maps#63215kimjune01 wants to merge 3 commits into
Conversation
|
@kimjune01 I think the commit's module should be |
|
Good point — the root cause is in the source map resolution path, not assert itself. I'll update the title. |
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
@kimjune01 please reword the commit as well 🙏🏽 |
629761d to
bef74dd
Compare
|
Done — reworded commit to |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #63215 +/- ##
=======================================
Coverage 90.12% 90.12%
=======================================
Files 743 743
Lines 242415 242422 +7
Branches 45656 45653 -3
=======================================
+ Hits 218476 218491 +15
Misses 15438 15438
+ Partials 8501 8493 -8
🚀 New features to boost your workflow:
|
|
@kimjune01 the CI failures all seem related to the changes |
6eddc82 to
4b39cd1
Compare
|
Rebased onto current main and fixed the two lint failures:
The earlier build failures looked unrelated to this lib/ change; should be clean on the fresh run. |
4b39cd1 to
6a0b05f
Compare
|
Done. Moved both cases into test-node-output-sourcemaps.mjs as snapshot fixtures under source-map/output/ with a |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: June Kim <kimjune01@gmail.com>
Fold the .mjs and .cjs --enable-source-maps cases from the standalone test-assert-ok-source-maps.js into the existing snapshot output test, following the setup in test-node-output-sourcemaps.mjs as requested in review. The fixtures live under test/fixtures/source-map/output/ with a `// Flags: --enable-source-maps` directive and assert AssertionError output via snapshots instead of bespoke spawnSync assertions. Refs: nodejs#63169 Signed-off-by: June Kim <kimjune01@gmail.com>
The literal first argument to assert() is intentional in these regression fixtures (the .snapshot files pin the exact source line and column), so disable the rule inline rather than rewriting the call.
b15e1c6 to
da520ed
Compare
|
Rebased onto main. Could someone run CI? |
When
--enable-source-mapsis enabled but no source map exists for the script (e.g. a plain.mjsfile),getErrorSourceLocation()returnedundefined. This causedgetErrMessage()to returnundefined, which madeinnerOk()pass[undefined]as the message tuple toinnerFail(), hitting theERR_INVALID_ARG_TYPEguard instead of throwingAssertionError.Fix all code paths in
getErrorSourceLocation()that returned bareundefinedto fall back to the raw generated source line, matching the behavior when source maps are disabled entirely. Also add a guard for when V8 fails to provide a source line at all.Includes tests for both
.mjsand.cjsfiles under--enable-source-maps.Fixes: #63169