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.

Sign up for GitHub

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

Jump to bottom

test: fix test-vm-sigint flakiness #7854

Closed
wants to merge 1 commit into from
Closed

test: fix test-vm-sigint flakiness #7854

wants to merge 1 commit into from

Conversation

santigimeno
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Set the SIGUSR2 handler before spawning the child process to make sure
the signal is always handled.

The test was sometimes crashing in my OS X box when running the test suite because of SIGUSR2 not being handled.

@santigimeno
test: fix test-vm-sigint flakiness
5f995dd 
Set the `SIGUSR2` handler before spawning the child process to make sure
the signal is always handled.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jul 23, 2016
@santigimeno
Copy link
Member Author

Maybe fixes #7767 ?

@santigimeno santigimeno added the vm Issues and PRs related to the vm subsystem. label Jul 23, 2016
@addaleax
Copy link
Member

LGTM

CI: https://ci.nodejs.org/job/node-test-commit/4238/
Stress test on the platform #7767 was reported for: https://ci.nodejs.org/job/node-stress-single-test/814/nodes=centos5-32/

@Trott
Copy link
Member

Trott commented Jul 24, 2016

Unfortunately, I don't think the stress test means #7767 is fixed because the stress test doesn't fail on current master either. (Or at least it hasn't yet. 1000 runs and counting: https://ci.nodejs.org/job/node-stress-single-test/815/nodes=centos5-32/console) So there's probably some weird interaction with a previous test or a specific build peculiarity or something like that going on. Regardless, if this is an improvement to the test, then awesome. I'm OK with a "maybe fixes".

@santigimeno
Copy link
Member Author

@Trott I couldn't reproduce it running the test multiple times sequentially in my OS X either. Only could do it running multiple instances of the test in parallel. Maybe it's time to retake nodejs/testing#3

@santigimeno
Copy link
Member Author

@Trott does this LGTY?

@Trott
Copy link
Member

Trott commented Jul 30, 2016

@santigimeno Yes, LGTM.

If it fixes it (which, judging from what you say, it does) then great.

If it turns out that it doesn't fix it, then it's a harmless change.

@santigimeno
Copy link
Member Author

santigimeno commented Jul 31, 2016 •
edited
Loading

@Trott Just to confirm that it fixes the issue, we can wait for #7859 to land, so the test runner prints the signal code when a test crashes. Then wait for this test to fail again. If the test crashes with signal code SIGUSR2, this PR mostly sure fixes the issue.

@Trott
Copy link
Member

Trott commented Jul 31, 2016

@santigimeno Great! (I don't think there's anything holding up #7859 if you want to land it, is there?)

@santigimeno
Copy link
Member Author

You're right. Just landed it.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 1, 2016

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Aug 1, 2016

LGTM

@Trott
Copy link
Member

Trott commented Aug 2, 2016

Now that there's code added to print the signal, it fails like this:

not ok 1072 parallel/test-vm-sigint
# CRASHED (Signal: 12)

https://ci.nodejs.org/job/node-test-commit-linux/4427/nodes=centos5-64/console

@Trott
Copy link
Member

Trott commented Aug 2, 2016

12 seems to be SIGUSR2, so I guess that's confirmation that this PR is expected to fix the problem.

@addaleax
Copy link
Member

addaleax commented Aug 2, 2016

Yup, sounds good. I don’t think there’s anything speaking against landing this (and adding a Fixes: line)?

@addaleax
Copy link
Member

addaleax commented Aug 2, 2016

Landed in 93ac2ea, thanks!

@addaleax addaleax closed this Aug 2, 2016
addaleax pushed a commit that referenced this pull request Aug 2, 2016
@santigimeno @addaleax
test: fix test-vm-sigint flakiness
93ac2ea 
Set the `SIGUSR2` handler before spawning the child process to make sure
the signal is always handled.

Fixes: #7767
PR-URL: #7854
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Aug 4, 2016
Closed
addaleax added a commit to addaleax/node that referenced this pull request Aug 4, 2016
@addaleax
test: fix flaky test-vm-sigint-existing-handler
f0bf2c2 
Set the `SIGUSR2` handler before spawning the child process to make sure
the signal is always handled.

Ref: nodejs#7854
Fixes: nodejs#7981
@addaleax addaleax mentioned this pull request Aug 4, 2016
Closed
3 tasks
addaleax added a commit that referenced this pull request Aug 8, 2016
@addaleax
test: fix flaky test-vm-sigint-existing-handler
be73480 
Set the `SIGUSR2` handler before spawning the child process to make sure
the signal is always handled.

Ref: #7854
Fixes: #7981
PR-URL: #7982
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig cjihrig mentioned this pull request Aug 8, 2016
Closed
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
@santigimeno @cjihrig
test: fix test-vm-sigint flakiness
d94063a 
Set the `SIGUSR2` handler before spawning the child process to make sure
the signal is always handled.

Fixes: #7767
PR-URL: #7854
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
cjihrig pushed a commit that referenced this pull request Aug 10, 2016
@addaleax @cjihrig
test: fix flaky test-vm-sigint-existing-handler
a385277 
Set the `SIGUSR2` handler before spawning the child process to make sure
the signal is always handled.

Ref: #7854
Fixes: #7981
PR-URL: #7982
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@cjihrig cjihrig mentioned this pull request Aug 11, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
test Issues and PRs related to the tests. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@santigimeno @addaleax @Trott @cjihrig @jasnell @nodejs-github-bot