test: run abort tests #14013

Trott · 2017-06-30T20:18:42Z

Currently, tests in test/abort do not run in CI.

This change configures the test runner to not write core files, and run
the abort tests.

This should land only after #13985 because that fixes a problem that causes one of the tests to fail. (In other words, had we been running these tests, that bug would have been caught!)

@addaleax @refack @nodejs/build @nodejs/testing

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

build test tools

Trott · 2017-06-30T20:18:51Z

@bnoordhuis

refack · 2017-06-30T20:27:13Z

tools/test.py


 from os.path import join, dirname, abspath, basename, isdir, exists
 from datetime import datetime
 from Queue import Queue, Empty

+resource.setrlimit(resource.RLIMIT_CORE, (0,0))


I'm thinking specialize SimpleTestConfiguration and do it there.
Maybe we'll want a test that examines the core-dump and deletes it?
Also there was an issue where we wanted the core for post-mortem...

Also there was an issue where we wanted the core for post-mortem...

Fwiw, we’d still need to configure the machines to actually store them in order to do that.

Found it: #13227
apparently the smartOS machines are configured

I'm thinking specialize SimpleTestConfiguration and do it there.

I'd do it in test/abort/testcfg.py.

edit: hm, or not. It still affects multi-suite runs (tools/test.py abort addon) so it's not materially different from the current pull request.

edit: hm, or not. It still affects multi-suite runs (tools/test.py abort addon) so it's not materially different from the current pull request.

Hmmm test.TestConfiguration doesn't have an end hook 😕

Edit: could put it last in the list in Makefile and vcbuild.bat

If you set both the hard and the soft limit (what this PR does), it's permanent; you can't change the limit back again in the same process.

Edit: could put it last in the list in Makefile and vcbuild.bat

Seems a little fragile. Better always X than sometimes X, sometimes Y, depending on how it's invoked.

If you set both the hard and the soft limit (what this PR does), it's permanent; you can't change the limit back again in the same process.

Did not know that...

Seems a little fragile. Better always X than sometimes X, sometimes Y, depending on how it's invoked.

Ack.

So trying to be creative

maybe change Makefile/vcbuild to run the about suite in a subsequent python invocation

or make the new test.AbortTestConfiguration spawn a child process...

I want my cake and eat it too...

I took a few tentative steps towards using preexec_fn in the subprocess.Popen() call so that the resource limit only applies when needed, but my Python is not-so-good and I ran into not being sure how to get the necessary configuration indicators passed from the (will have to create it) AbortTestConfiguration.

https://stackoverflow.com/a/1689991/436641

I can take over if you want but what you have now is Good Enough, IMO. Threading through the ulimit setting from test/abort/testcfg.py to subprocess.Popen() is quite a bit of work for not all that much gain.

@Trott tasked me with the digging... (since I was the nudnik who wanted this)

cjihrig

LGTM if it works :-)

Trott · 2017-08-24T21:06:38Z

@refack @addaleax @cjihrig @mhdawson @bnoordhuis I dug into the Python enough to make it so that it only disables core files for the tests in the abort directory. Please take a look.

Trott · 2017-08-24T22:25:05Z

CI: https://ci.nodejs.org/job/node-test-pull-request/9827/

Trott · 2017-08-25T05:01:13Z

Heh! Well, running the abort tests in CI seems like a good idea since there are failures on certain platforms. Will dig in...

Currently, tests in test/abort do not run in CI. This change configures the test runner to not write core files for abort tests and to run them. PR-URL: #14013 Fixes: #14012 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

refack · 2017-09-18T19:38:16Z

So I think smartOS does not respect resource.setrlimit(resource.RLIMIT_CORE, (0,0))

@misterdjules does this makes sense?

Trott · 2017-09-19T03:25:41Z

@refack If that's the case, I'd be OK with setting smartOS to skip all the abort tests via an abort.status file.

gibfahn · 2017-09-19T06:42:37Z

So I think smartOS does not respect resource.setrlimit(resource.RLIMIT_CORE, (0,0))

cc/ @nodejs/platform-smartos , I thought we purposefully set the smartos boxes to dump cores at some point, so maybe that setting is conflicting?

misterdjules · 2017-09-19T20:50:54Z

The reason why core files are created even when the RLIMIT_CORE limit is set to 0 for those processes is that the current setup of SmartOS instances enables "global" core dumps with:

 coreadm -g /home/iojs/cores/core.%f.%p -e global

When global core dumps are enabled, they are created for all processes, even those whose RLIMITE_CORE is set to 0.

I suggest we disable global core dumps, and enable only per-process core dumps with the following command:

coreadm -i /home/iojs/cores/core.%f.%p -e process -d global

MylesBorins · 2017-09-20T18:47:35Z

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

gibfahn · 2017-09-22T13:38:42Z

@misterdjules is that something that should be done in the test runner or in an ansible script?

misterdjules · 2017-09-22T16:05:14Z

@gibfahn I would think at https://github.com/nodejs/build/blob/8bc3a5f64937f377edbe79e4cdb4396591bf2d63/ansible/roles/baselayout/tasks/main.yml#L25.

gibfahn · 2017-09-22T16:20:07Z

@misterdjules would you mind opening a PR in nodejs/build referencing this PR? I'm happy to review.

misterdjules · 2017-09-22T16:41:02Z

@gibfahn Sounds good, will do that.

This allows for controlling core files limits per process (e.g per tests). Ref: nodejs/node#14013

misterdjules · 2017-09-22T17:08:42Z

@gibfahn See nodejs/build#894.

PR-URL: nodejs#15056 Fixes: nodejs#14012 Refs: nodejs#14013 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #15056 Fixes: #14012 Refs: #14013 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This allows for controlling core files limits per process (e.g per tests). PR-URL: #894 Refs: nodejs/node#14013

MylesBorins · 2017-10-16T22:09:52Z

safe to assume there is nothing else actionable here?

Trott · 2017-10-17T03:30:13Z

@MylesBorins First two commits should likely be cherry-picked over. They land cleanly right now.

Third commit, I'd say no. It doesn't land cleanly and it's not clear to me that running the abort tests on v6.x-staging adds much value at this point. (We had not been running the abort tests regularly for years.) As long as they run on master and v8.x-staging, I think we're good.

PR-URL: nodejs#15056 Fixes: nodejs#14012 Refs: nodejs#14013 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Backport-PR-URL: #16442 PR-URL: #15056 Fixes: #14012 Refs: #14013 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Trott added build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jun 30, 2017

nodejs-github-bot added build Issues and PRs related to build files or the CI. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels Jun 30, 2017

addaleax approved these changes Jun 30, 2017

View reviewed changes

refack suggested changes Jun 30, 2017

View reviewed changes

bnoordhuis approved these changes Jun 30, 2017

View reviewed changes

Trott force-pushed the run-abort-tests branch from 78081a2 to 33db5e6 Compare July 3, 2017 19:59

mhdawson approved these changes Jul 6, 2017

View reviewed changes

cjihrig approved these changes Jul 6, 2017

View reviewed changes

This was referenced Jul 6, 2017

src: document --abort-on-uncaught-exception #13931

Merged

test: define and test validity of core-dumps #14110

Closed

Trott added the python PRs and issues that require attention from people who are familiar with Python. label Jul 24, 2017

Trott force-pushed the run-abort-tests branch from 33db5e6 to 0bba519 Compare August 6, 2017 05:18

Trott force-pushed the run-abort-tests branch 2 times, most recently from 807205a to 3dcff92 Compare August 24, 2017 21:05

Trott force-pushed the run-abort-tests branch 4 times, most recently from c654836 to ac48ed5 Compare August 24, 2017 21:08

addaleax approved these changes Aug 24, 2017

View reviewed changes

Trott force-pushed the run-abort-tests branch 2 times, most recently from ea93f5d to b1fe802 Compare August 25, 2017 05:46

refack mentioned this pull request Sep 18, 2017

Investigate flaky parallel/test-http-pipeline-regr-3332 on smartos15-64 #15418

Closed

MylesBorins added the lts-watch-v6.x label Sep 20, 2017

misterdjules pushed a commit to misterdjules/build that referenced this pull request Sep 22, 2017

switch from global to per-process core dumps on SmartOS

f34ed72

This allows for controlling core files limits per process (e.g per tests). Ref: nodejs/node#14013

misterdjules mentioned this pull request Sep 22, 2017

switch from global to per-process core dumps on SmartOS nodejs/build#894

Merged

gibfahn pushed a commit to nodejs/build that referenced this pull request Sep 29, 2017

switch from global to per-process core dumps on SmartOS

29bc6bc

This allows for controlling core files limits per process (e.g per tests). PR-URL: #894 Refs: nodejs/node#14013

refack mentioned this pull request Oct 24, 2017

[v6.x backport] test,process: run 'abort' suite on Windows #16442

Closed

2 tasks

MylesBorins added backported-to-v6.x and removed lts-watch-v6.x labels Nov 14, 2017

Trott deleted the run-abort-tests branch January 13, 2022 22:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: run abort tests #14013

test: run abort tests #14013

Trott commented Jun 30, 2017

Trott commented Jun 30, 2017

refack Jun 30, 2017

addaleax Jun 30, 2017

refack Jun 30, 2017

bnoordhuis Jun 30, 2017 •

edited

Loading

refack Jun 30, 2017 •

edited

Loading

bnoordhuis Jun 30, 2017

refack Jun 30, 2017 •

edited

Loading

Trott Jun 30, 2017

bnoordhuis Jul 2, 2017

refack Jul 2, 2017 •

edited

Loading

cjihrig left a comment

Trott commented Aug 24, 2017

Trott commented Aug 24, 2017

Trott commented Aug 25, 2017 •

edited

Loading

refack commented Sep 18, 2017

Trott commented Sep 19, 2017

gibfahn commented Sep 19, 2017

misterdjules commented Sep 19, 2017

MylesBorins commented Sep 20, 2017

gibfahn commented Sep 22, 2017

misterdjules commented Sep 22, 2017

gibfahn commented Sep 22, 2017

misterdjules commented Sep 22, 2017

misterdjules commented Sep 22, 2017

MylesBorins commented Oct 16, 2017

Trott commented Oct 17, 2017

test: run abort tests #14013

test: run abort tests #14013

Conversation

Trott commented Jun 30, 2017

Checklist

Affected core subsystem(s)

Trott commented Jun 30, 2017

refack Jun 30, 2017

Choose a reason for hiding this comment

addaleax Jun 30, 2017

Choose a reason for hiding this comment

refack Jun 30, 2017

Choose a reason for hiding this comment

bnoordhuis Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

refack Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

bnoordhuis Jun 30, 2017

Choose a reason for hiding this comment

refack Jun 30, 2017 • edited Loading

Choose a reason for hiding this comment

Trott Jun 30, 2017

Choose a reason for hiding this comment

bnoordhuis Jul 2, 2017

Choose a reason for hiding this comment

refack Jul 2, 2017 • edited Loading

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

Trott commented Aug 24, 2017

Trott commented Aug 24, 2017

Trott commented Aug 25, 2017 • edited Loading

refack commented Sep 18, 2017

Trott commented Sep 19, 2017

gibfahn commented Sep 19, 2017

misterdjules commented Sep 19, 2017

MylesBorins commented Sep 20, 2017

gibfahn commented Sep 22, 2017

misterdjules commented Sep 22, 2017

gibfahn commented Sep 22, 2017

misterdjules commented Sep 22, 2017

misterdjules commented Sep 22, 2017

MylesBorins commented Oct 16, 2017

Trott commented Oct 17, 2017

bnoordhuis Jun 30, 2017 •

edited

Loading

refack Jun 30, 2017 •

edited

Loading

refack Jun 30, 2017 •

edited

Loading

refack Jul 2, 2017 •

edited

Loading

Trott commented Aug 25, 2017 •

edited

Loading