Broken Lifecycle Signal Tests

[deckar01]

The tests in test/tap/lifecycle-signal.js are skipped on Windows and Travis, but are failing on Linux.

• Ubuntu 15.04
• node v5.10.0

lifecycle signal abort

This test fails because regardless of the signal used to kill the process in preinstall the child's exit code is 1 and exit signal is null. There seems to be an error handler stripping the signal and changing the code to 1. I tested that node -e wasn't eating the error, so it must be somewhere around the install action or the lifecycle.

test/tap/lifecycle-signal.js#L45

Originally added by @TooTallNate in d885f81

lifecycle propagate signal term to child

This test fails, because the test assumes killing a child process kills its subprocess, but that is incorrect.

I think you have the wrong idea about processes and process groups. On exit, child processes get reparented, they're not forcibly killed by io.js or the kernel.
nodejs/node#2098 (comment)

http://stackoverflow.com/questions/33325301/killing-a-bash-script-does-not-kill-child-processes

test/tap/lifecycle-signal.js#L67

Originally added by @daniel-pedersen in 5454347.

---

ubuntu@ubuntu:~/npm$ DEPLOY_VERSION=testing npm run tap -- "test/tap/lifecycle-signal.js" 

> npm@3.8.6 tap /home/ubuntu/npm
> tap --timeout 240 "test/tap/lifecycle-signal.js"

test/tap/lifecycle-signal.js .......................... 5/9 4m
  lifecycle signal abort
  not ok should be equal
    +++ found                                                          
    --- wanted                                                         
    -[null]                                                            
    +1                                                                 
    compare: ===
    at:
      line: 44
      column: 7
      file: test/tap/lifecycle-signal.js
      type: ChildProcess
    stack: |
      ChildProcess.<anonymous> (test/tap/lifecycle-signal.js:44:7)
      Pipe._onclose (net.js:477:12)

  lifecycle signal abort
  not ok should be equal
    +++ found                                                          
    --- wanted                                                         
    -"SIGSEGV"                                                         
    +[null]                                                            
    compare: ===
    at:
      line: 45
      column: 7
      file: test/tap/lifecycle-signal.js
      type: ChildProcess
    stack: |
      ChildProcess.<anonymous> (test/tap/lifecycle-signal.js:45:7)
      Pipe._onclose (net.js:477:12)

  lifecycle propagate signal term to child
  not ok expected to throw
    at:
      line: 68
      column: 7
      file: test/tap/lifecycle-signal.js
      type: ChildProcess
    stack: |
      ChildProcess.<anonymous> (test/tap/lifecycle-signal.js:68:7)

  not ok timeout!
    at:
      line: 3
      column: 11
      file: node_modules/tap/lib/root.js
    signal: SIGTERM
    handles:
      - type: Socket
        events:
          - end
          - finish
          - _socketEnd
          - close
      - type: Socket
        events:
          - end
          - finish
          - _socketEnd
          - close
          - data

total ................................................. 5/9


  5 passing (4m)
  4 failing

[gibfahn]

@deckar01 @othiym23 @iarna Was a decision reached about making npm use bash instead of sh (ref: #12221 (comment)) ?

Is it just a question of changing the shebang in bin/npm#L1 from #!/bin/sh to #!/bin/bash?

It'd be nice to get these tests passing in order to run make test-npm from node.

[othiym23]

Was a decision reached about making npm use bash instead of sh?

Yes, although the CLI team has been focused on high-priority crashers, and as such hasn't worked in this yet.

Is it just a question of changing the shebang in bin/npm#L1 from #!/bin/sh to #!/bin/bash?

Not exactly, because it's not uncommon for bash to be in /usr/bin instead. Either the code should just invoke bash assuming that it's on PATH, or use which to find an absolute path to bash. The latter is preferred, given that we're moving npm towards being able to use a configurable shell (overriding, in particular, cmd.exe on Windows).

[gibfahn]

@othiym23 So would something like cli.js#L1 (i.e. using #!/usr/bin/env bash) be the way to get it from the PATH?

Also would you mind explaining what you mean by being able to use a configurable shell? Do you mean allowing the user to specify which shell they want to use?

[odinho]

Bah :( Wasted time on this, I mistakenly thought I'd broken this test with my code, and didn't first test the rev before mine since I wrongly assumed all tests would normally pass. I see there was quite a lot of activity on this, but then it all stopped. I think @gibfahn's suggestion sounds like a good quick fix.

It'd be good to even just disable this test. Having broken tests is anti pattern :)

[npm-robot]

We're closing this issue as it has gone thirty days without activity. In our experience if an issue has gone thirty days without any activity then it's unlikely to be addressed. In the case of bug reports, often the underlying issue will be addressed but finding related issues is quite difficult and often incomplete.

If this was a bug report and it is still relevant then we encourage you to open it again as a new issue. If this was a feature request then you should feel free to open it again, or even better open a PR.

For more information about our new issue aging policies and why we've instituted them please see our blog post.

[deckar01]

As far as I know, this is still an issue. The travis exclusion is still in those tests, and the shell config variable is only for the explore command. 🤖

I'm not using Ubuntu or contributing to npm anymore, so I will leave it up to someone else to create a new issue.