[BUG] Tests crash out on ctrl-c

[rotu]

Is there an existing issue for this?

• I have searched the existing issues

This issue exists in the latest npm version

• I am using the latest npm

Current Behavior

Interrupting tests with ctrl-c results in an error:

TypeError: The "code" argument must be of type number. Received type string ('128SIGINT')
at process.set [as exitCode] (node:internal/bootstrap/node:123:9)
at ChildProcess. (/Users/dan/Source/npm-cli/node_modules/nyc/node_modules/foreground-child/index.js:63:22)
at ChildProcess.emit (node:events:514:28)
at ChildProcess.emit (node:domain:489:12)
at maybeClose (node:internal/child_process:1105:16)
at Process.ChildProcess._handle.onexit (node:internal/child_process:305:5)

This seems to come from bad logic adding a number to a string in foreground-child@2.0.0 (outdated) via nyc@15.1.0 (latest, but unmaintained).

Expected Behavior

Tests should exit cleanly.

Steps To Reproduce

1. In Mac or Windows
2. Run npm test
3. Use ctrl-c to abort the tests
4. Observe error

Environment

• npm: 10.0.0
• Node.js: 20.6.0
• OS Name: macOS
• System Model Name: Macbook Air
• npm config:

; "builtin" config from /opt/homebrew/lib/node_modules/npm/npmrc

prefix = "/opt/homebrew" 

; "user" config from /Users/dan/.npmrc

auto-install-peers = true 

; "project" config from /Users/dan/Source/npm-cli/.npmrc

package-lock = true 

; node bin location = /opt/homebrew/Cellar/node/20.6.0/bin/node
; node version = v20.6.0
; npm local prefix = /Users/dan/Source/npm-cli
; npm version = 9.8.1
; cwd = /Users/dan/Source/npm-cli
; HOME = /Users/dan
; Run `npm config ls -l` to show all defaults.

[rotu]

FYI, @lukekarrys. I wonder if whatever necessitated b8a5764 is related to this issue.

[lukekarrys]

I haven't looked into this too deeply yet, but I believe this is due to a change in Node 20 of new restrictions on process.code. I think this should only affect our tests for now, but still something that should be fixed once the current batch of priority bugs in npm 10 are squashed.

[rotu]

Ah. Can confirm this happens on my machine in Node 20 but not Node 18.

Node 19 gives a deprecation warning:

(node:38601) [DEP0164] DeprecationWarning: Implicit coercion to integer for exit code is deprecated.
    at process.set [as exitCode] (node:internal/bootstrap/node:118:9)
    at ChildProcess. (/Users/dan/Source/npm-cli/node_modules/nyc/node_modules/foreground-child/index.js:63:22)
    at ChildProcess.emit (node:events:513:28)
    at ChildProcess.emit (node:domain:489:12)
    at maybeClose (node:internal/child_process:1098:16)
    at Process.ChildProcess._handle.onexit (node:internal/child_process:304:5)

[rotu]

@lukekarrys Actually, you may want to bump up the priority. Further experimentation shows that, in Node 18, the process actually exits with exit code 0. This could very well be causing tests that should fail to accidentally pass!

node -e 'process.exitCode=42; process.exit()'; echo $?
# 42
node -e 'process.exitCode=42+"SIGINT"; process.exit()'; echo $?
# 0

[rotu]

FYI @isaacs, this seems important to your work on tapjs.

[isaacs]

I already have much more sophisticated signal and error handling code in tap 18. Unfortunately, I'm a bit delayed in shipping it, because the changes to the loader system in node have required quite a lot of work to get everything converted over. The mockImport method is almost done being converted, and I'm waiting on https://github.com/TypeStrong/ts-node/pulls/2009 to land for ts-node to work there.

Once those are done, I can ship tap 18, and ignore this and all NYC errors forever 😅

Can you get it to happen in foreground-child v3? If not, I'm inclined to say it's just WONTFIX, not out of any malice, just because the 2.x codebase is long out of date.

[isaacs]

Omg, I just looked at the link you posted. Adding 128 to the signal byte to get it to exit properly. Those were the ""good"" old days, for sure.

[rotu]

I already have much more sophisticated signal and error handling code in tap 18. Unfortunately, I'm a bit delayed in shipping it, because the changes to the loader system in node have required quite a lot of work to get everything converted over. The mockImport method is almost done being converted, and I'm waiting on TypeStrong/ts-node#2009 to land for ts-node to work there.

Once those are done, I can ship tap 18, and ignore this and all NYC errors forever 😅

Can you get it to happen in foreground-child v3? If not, I'm inclined to say it's just WONTFIX, not out of any malice, just because the 2.x codebase is long out of date.

Awesome! I didn't know you were ditching nyc - that makes this much less a problem moving forward.

It's also good that this doesn't fail so quietly on Node 20. Why it ever failed silently makes me fear for humanity a little bit.

Omg, I just looked at the link you posted. Adding 128 to the signal byte to get it to exit properly. Those were the ""good"" old days, for sure.

Heh. It seems like the close handler always got a string for the signal, so setting the exit code based on a signal never worked quite as intended.

[isaacs]

Heh. It seems like the close handler always got a string for the signal, so setting the exit code based on a signal never worked quite as intended.

Kids these days thinking node 0.10 was the first node version, get off my lawn etc. Yeah, istr that is copy-pasta from an on('exit') handler code dating back to around node 0.1.100-ish days. It's never worked as written, and frankly surprising to me that it was never caught until now. In any case, exiting with a status code byte with the high bit set to indicate a signal exit is an ancient idiom, and has never worked on Windows.

It's also good that this doesn't fail so quietly on Node 20. Why it ever failed silently makes me fear for humanity a little bit.

You and me both.