Skip to content

Loading…

skip failing test on linux #4663

Closed
wants to merge 1 commit into from

7 participants

@robertkowalski
npm member

The GNU shell returns a code and no signal if it gets a SIGSEGV
signal, so we have to skip this. Hat tip to @tjfontaine

This test fails on Linux and therefore travis. I first thought it would be a node code issue and created some test-code (https://github.com/robertkowalski/node-signal-debian), but happily TJ helped me to identify the issue with https://github.com/npm/npm/blob/9e12b1a174bcbcebb26732016cd1961a93947751/lib/utils/lifecycle.js#L207:

19:29:27 <@tjfontaine> robertkowalski: ok, I can explain what's going on here
19:29:34 <@tjfontaine> robertkowalski: we both just made the same mistake
19:29:54 <@tjfontaine> robertkowalski: the exit code and signal you're seeing are from 'sh' 
                                      but not from your node
19:38:24 <@tjfontaine> robertkowalski: does that make sense? iow change from spawn('sh'... 
                                      to spawn(process.execPath or execFile(process.execPath, and 
                                      you'll see the signal

After that commit every test for node 0.10 (0.8 still one missing) is running on travis, so we could start using travis for CI which could help us alot in daily work (e.g. pretesting PRs and giving automatic feedback, providing a single point of truth for tests). I tested it already with my github account, a run takes 7 minutes (including the old tests)

@tjfontaine

can the tests be changed instead to merely invoke node and not use the shell instead?

If the goal is to accurately be notified when a process is signalled that's the preferred mechanism.

@robertkowalski
npm member

@tjfontaine changing the tests doesn't work I think.

The problem already starts in lifecycle.js (where preinstall-hooks & co run):

https://github.com/npm/npm/blob/9e12b1a174bcbcebb26732016cd1961a93947751/lib/utils/lifecycle.js#L207

npm spawns a child already there with the shell of choice (sh -c for unix/linux and for cmd.exe windows) and in that sub-sub-shell the signal is sent

@rlidwka

+1, it's once single test that I have not been able to launch successfully

introduced in #4110

@domenic
npm member

Wait, so we are already skipping this test on Windows; you are saying it fails on Linux too? So it only passes on Mac? This seems like pretty lame functionality.

@robertkowalski
npm member

@tjfontaine does my explanation makes sense to you?

@isaacs
npm member

@robertkowalski What code does linux die with?

The point of this test is to make sure that npm install fails if a lifecycle script is abruptly terminated. Ideally, it would pass the signal exit upwards, but if the GNU shell is thwarting that, then let's at least verify that we're getting the exit code we expect.

@robertkowalski robertkowalski fix failing test on linux
The GNU shell returns a code and no signal if it gets a SIGSEGV
signal, so we have to skip this. Hat tip to @tjfontaine
ba38256
@robertkowalski
npm member

@robertkowalski What code does linux die with?

Just a usual 1... I updated the test..

@bcoe
npm member

:+1: having switched all the tap tests to using the mock registry, this is one of the last blockers with regards to running the tap tests on Travis.

@isaacs
npm member

Landed on ba38256. Thanks!

@isaacs isaacs closed this
@robertkowalski
npm member

:purple_heart:

@robertkowalski robertkowalski deleted the robertkowalski:disable-test-linux branch
@othiym23 othiym23 added the bug label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 17, 2014
  1. @robertkowalski

    fix failing test on linux

    robertkowalski committed
    The GNU shell returns a code and no signal if it gets a SIGSEGV
    signal, so we have to skip this. Hat tip to @tjfontaine
This page is out of date. Refresh to see the latest.
Showing with 9 additions and 0 deletions.
  1. +9 −0 test/tap/lifecycle-signal.js
View
9 test/tap/lifecycle-signal.js
@@ -8,10 +8,19 @@ var pkg = path.resolve(__dirname, "lifecycle-signal")
test("lifecycle signal abort", function (t) {
// windows does not use lifecycle signals, abort
if (process.platform === "win32") return t.end()
+
+
var child = spawn(node, [npm, "install"], {
cwd: pkg
})
child.on("close", function (code, signal) {
+ // GNU shell returns a code, no signal
+ if (process.platform === "linux") {
+ t.equal(code, 1)
+ t.equal(signal, null)
+ return t.end()
+ }
+
t.equal(code, null)
t.equal(signal, "SIGSEGV")
t.end()
Something went wrong with that request. Please try again.