Backport test abort on timeout to v4.x #11351
Conversation
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
added
tools
|
/cc @nodejs/testing @nodejs/build @nodejs/python @nodejs/lts |
|
Just to make sure this won't break CI tests when it's enabled for LTS branches too, I started tests running with Current failures seem to be unrelated to these changes. If these tests are successful, I'll merge these commits in v4.x-staging and I'll enable the |
CI tests run looks fine, will land asap and will update nodejs/build#613 to mention my intention to re-enable |
|
Merged in 6034bdc...1f40d8e. |
|
@misterdjules we generally do not land things on staging until they have lived on a release for at least 2 weeks. For now this shouldn't be an issue as we already have an rc for the next release but please keep this in mind for future commits |
Is that documented somewhere? I searched for that information in nodejs/lts and in the collaborating guide, and did not found anything. If that's indeed not documented, what would be the best place to document this? |
|
@MylesBorins this wasn't a user-visible change, it only affects our CI testing infrastructure. I was under the impression that infrastructure changes, like doc changes, didn't require the two week rule. This was also discussed in the build issue, see @bnoordhuis's comment: nodejs/build#613 (comment) |
|
@misterdjules it will be documented in #11099 when that lands, and it is currently documented in the LTS Plan.
|
|
@gibfahn So I had missed it. Thank you very much for the info! |
|
@gibfahn thanks for the heads up, all makes sense. |
Backports #11086 and #11153 to v4.x-staging so that
--abort-on-timeoutcan be enabled on the CI platform. See nodejs/build#613 for more context.Another backport to v6.x-staging will be submitted asap.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test