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.

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

Backport test abort on timeout to v6.x #11354

Conversation

misterdjules
Copy link

Backports #11086 and #11153 to v6.x-staging so that --abort-on-timeout can be enabled on the CI platform. See nodejs/build#613 for more context.

Another backport to v4.x-staging was submitted at #11351.

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

test

Julien Gilli added 2 commits February 13, 2017 12:23
Currently, when a process times out, it is terminated by sending it the
SIGTERM signal. Sending SIGBART instead allows the operating system to
generate a core file that can be investigated later using post-mortem
debuggers such as llnode or mdb_v8.

This can be very useful when investigating flaky tests that time out,
since in that case the failure is difficult to reproduce, and being able
to look at a core file makes a big difference.

With these changes, passing the --abort-on-timeout command line option
to tools/test.py now sends SIGABRT to processes timing out on all
platforms but Windows.

PR-URL: nodejs#11086
Ref: nodejs#11026
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
nodejs#11086 had introduced a regression
that broke command line options processing for tools/test.py.

Basically, it made tools/test.py discard the command line argument that
would be passed after `--abort-on-timeout`. For instance, when running:

```
$ python tools/test.py --abort-on-timeout path/to/some-test
```

all tests would be run because the last command line argument
(`/path/to/some-test`) would be discarded.

This change fixes this regression.

Refs: nodejs#11086
PR-URL: nodejs#11153
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@nodejs-github-bot nodejs-github-bot added tools Issues and PRs related to the tools directory. v6.x labels Feb 13, 2017
@misterdjules
Copy link
Author

/cc @nodejs/testing @nodejs/build @nodejs/python @nodejs/lts

@misterdjules misterdjules added the test Issues and PRs related to the tests. label Feb 13, 2017
@misterdjules
Copy link
Author

Just to make sure this won't break CI tests when it's enabled for LTS branches too, I started tests running with --abort-on-timeout enabled by a custom Jenkins job on these changes at https://ci.nodejs.org/job/node-test-commit-smartos-abort-on-timeout/1/.

Current failures seem to be unrelated to these changes.

If these tests are successful, I'll merge these commits in v6.x-staging and I'll enable the --abort-on-timeout flag for the node-test-commit-smartos Jenkins job.

@misterdjules
Copy link
Author

If these tests are successful, I'll merge these commits in v6.x-staging and I'll enable the --abort-on-timeout flag for the node-test-commit-smartos Jenkins job.

CI tests run looks fine, will land asap and will update nodejs/build#613 to mention my intention to re-enable --abort-on-timeout for the node-test-commit-smartos Jenkins job.

@misterdjules
Copy link
Author

Merged in 6614bd1...ce682eb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants