test: add --abort-on-timeout option to test.py #11086

Closed
wants to merge 1 commit into
from

Conversation

@misterdjules

misterdjules commented Jan 31, 2017

test: add --abort-on-timeout option to test.py

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.

Refs: #11026

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

test

/cc @nodejs/testing

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Jan 31, 2017

Member

Aren't there some tests that test the --abort flag? If so, will enabling core dumps on the hosts have any effect on that?

Member

evanlucas commented Jan 31, 2017

Aren't there some tests that test the --abort flag? If so, will enabling core dumps on the hosts have any effect on that?

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Jan 31, 2017

Aren't there some tests that test the --abort flag? If so, will enabling core dumps on the hosts have any effect on that?

That is a very good question. Enabling core files on any platform will not generate additional core files for tests that use the --abort-on-uncaught-exception command line flag.

The tests that are using the --abort-on-uncaught-exception flag are written so that the node processes that end up running with that flag are run with child_process.exec and the command line used prepends ulimit -c 0 &&, disabling core file generation for these processes on most platforms.

I'm saying on most platforms because I'm not sure about platforms that I'm not familiar with, like AIX. Maybe @mhdawson can confirm?

On SmartOS, it's a bit different than Linux and OSX in the sense that ulimit -c 0 doesn't necessarily prevent any core file from being generated. Other system-wide settings can still allow core files to be generated when ulimit -c outputs 0. However, in that case, what happens on that platform is that a directory path where all core files are stored is setup, and a cron job cleans up that directory periodically.

Now, another use case with the changes in this PR is that, when tests times out on a developer's machine, core files could be generated since not all tests run with ulimit -c 0. The default on a lot of Linux distributions and on OSX, AFAIK, is to set the core file size limit to 0, which means no core file would be generated. However we can't really rely on this for these platforms, as developers can change their limit settings, or for the defaults not to change.

Thus, I would suggest to add support for a new --abort-on-timeout command line option for tools/test.py that, if set, would make test.py use SIGABRT instead of SIGTERM in KillProcessWithID. That new command line option could be used by the test-ci make target, and would make tests that timeout generate a core file.

This way, individual developers running tests on their development machine would never have any core file generated, even if they change their OS' core file size limit, but core files would be generated when appropriate for tests running on the CI platform.

How does that sound?

Aren't there some tests that test the --abort flag? If so, will enabling core dumps on the hosts have any effect on that?

That is a very good question. Enabling core files on any platform will not generate additional core files for tests that use the --abort-on-uncaught-exception command line flag.

The tests that are using the --abort-on-uncaught-exception flag are written so that the node processes that end up running with that flag are run with child_process.exec and the command line used prepends ulimit -c 0 &&, disabling core file generation for these processes on most platforms.

I'm saying on most platforms because I'm not sure about platforms that I'm not familiar with, like AIX. Maybe @mhdawson can confirm?

On SmartOS, it's a bit different than Linux and OSX in the sense that ulimit -c 0 doesn't necessarily prevent any core file from being generated. Other system-wide settings can still allow core files to be generated when ulimit -c outputs 0. However, in that case, what happens on that platform is that a directory path where all core files are stored is setup, and a cron job cleans up that directory periodically.

Now, another use case with the changes in this PR is that, when tests times out on a developer's machine, core files could be generated since not all tests run with ulimit -c 0. The default on a lot of Linux distributions and on OSX, AFAIK, is to set the core file size limit to 0, which means no core file would be generated. However we can't really rely on this for these platforms, as developers can change their limit settings, or for the defaults not to change.

Thus, I would suggest to add support for a new --abort-on-timeout command line option for tools/test.py that, if set, would make test.py use SIGABRT instead of SIGTERM in KillProcessWithID. That new command line option could be used by the test-ci make target, and would make tests that timeout generate a core file.

This way, individual developers running tests on their development machine would never have any core file generated, even if they change their OS' core file size limit, but core files would be generated when appropriate for tests running on the CI platform.

How does that sound?

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Jan 31, 2017

Member

I like the suggestion of --abort-on-timeout command line option for tools/test.py. I would also give us flexibility to capture/not capture the core file depending on the platform as this could be add/not added in the specific subjob for each platform.

I believe ulimit -c 0 will disabled core dumps on AIX. One ref that seems to confirm this: https://www.ibm.com/developerworks/community/blogs/KRblog/entry/aix_core_dump_facility?lang=en

Member

mhdawson commented Jan 31, 2017

I like the suggestion of --abort-on-timeout command line option for tools/test.py. I would also give us flexibility to capture/not capture the core file depending on the platform as this could be add/not added in the specific subjob for each platform.

I believe ulimit -c 0 will disabled core dumps on AIX. One ref that seems to confirm this: https://www.ibm.com/developerworks/community/blogs/KRblog/entry/aix_core_dump_facility?lang=en

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Jan 31, 2017

@mhdawson

I believe ulimit -c 0 will disabled core dumps on AIX. One ref that seems to confirm this: https://www.ibm.com/developerworks/community/blogs/KRblog/entry/aix_core_dump_facility?lang=en

Thanks for the confirmation!

@mhdawson

I believe ulimit -c 0 will disabled core dumps on AIX. One ref that seems to confirm this: https://www.ibm.com/developerworks/community/blogs/KRblog/entry/aix_core_dump_facility?lang=en

Thanks for the confirmation!

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 1, 2017

Member

Sending SIGABRT unconditionally like the PR currently does isn't very sociable. --abort-on-timeout seems like a good middle ground.

Member

bnoordhuis commented Feb 1, 2017

Sending SIGABRT unconditionally like the PR currently does isn't very sociable. --abort-on-timeout seems like a good middle ground.

@gibfahn

gibfahn approved these changes Feb 1, 2017

LGTM, but we should get a review from @nodejs/python as well.

@misterdjules misterdjules changed the title from test: use SIGBART not SIGKILL to kill processes to test: add --abort-on-timeout option to test.py Feb 1, 2017

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Feb 1, 2017

Updated the changes according to recent discussions. I also updated the original comment and the title of this PR since the latest update changes its nature. Please take a look.

Updated the changes according to recent discussions. I also updated the original comment and the title of this PR since the latest update changes its nature. Please take a look.

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Feb 2, 2017

Member

@misterdjules It might make sense to enable this flag in the test-ci target in this PR, then we can run CI to confirm that everything is working.

Member

gibfahn commented Feb 2, 2017

@misterdjules It might make sense to enable this flag in the test-ci target in this PR, then we can run CI to confirm that everything is working.

@bnoordhuis

LGTM with style nits.

+ # difficult to reproduce.
+ signal_to_send = signal.SIGABRT
+ KillProcessWithID(pid, signal_to_send)
+

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 2, 2017

Member

Can you add an extra blank line here?

@bnoordhuis

bnoordhuis Feb 2, 2017

Member

Can you add an extra blank line here?

This comment has been minimized.

tools/test.py
@@ -1385,6 +1396,9 @@ def BuildOptions():
result.add_option('--repeat',
help='Number of times to repeat given tests',
default=1, type="int")
+ result.add_option('--abort-on-timeout',
+ help='Send SIGABRT instead of SIGTERM to kill processes that time out',
+ default=False, dest="abort_on_timeout")

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 2, 2017

Member

Indent by four spaces here.

@bnoordhuis

bnoordhuis Feb 2, 2017

Member

Indent by four spaces here.

This comment has been minimized.

@gibfahn

gibfahn Feb 2, 2017

Member

Do we not have a linter for the python files? Not sure if eslint can handle it.

@gibfahn

gibfahn Feb 2, 2017

Member

Do we not have a linter for the python files? Not sure if eslint can handle it.

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 2, 2017

Member

We don't. We don't maintain much python code, though. Pointing out the occasional style issue isn't much of a burden.

@bnoordhuis

bnoordhuis Feb 2, 2017

Member

We don't. We don't maintain much python code, though. Pointing out the occasional style issue isn't much of a burden.

This comment has been minimized.

@cjihrig

cjihrig approved these changes Feb 2, 2017

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Feb 2, 2017

@gibfahn

It might make sense to enable this flag in the test-ci target in this PR, then we can run CI to confirm that everything is working.

My intention was to set the TEST_CI_ARGS environment variable for the Jenkins job for which we want to enable this feature. For now, that would be for SmartOS only. Then if there's a need/desire to enable this across all platforms, we can indeed update the test-ci make target.

Does that make sense?

@gibfahn

It might make sense to enable this flag in the test-ci target in this PR, then we can run CI to confirm that everything is working.

My intention was to set the TEST_CI_ARGS environment variable for the Jenkins job for which we want to enable this feature. For now, that would be for SmartOS only. Then if there's a need/desire to enable this across all platforms, we can indeed update the test-ci make target.

Does that make sense?

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Feb 2, 2017

Member

@misterdjules That does make sense, I was just thinking about the easiest way to test that this actually runs on CI for all platforms might be to change the Makefile for all platforms, run CI, and then change back.

So what's the reason not to enable this on other (non-win) platforms?

Member

gibfahn commented Feb 2, 2017

@misterdjules That does make sense, I was just thinking about the easiest way to test that this actually runs on CI for all platforms might be to change the Makefile for all platforms, run CI, and then change back.

So what's the reason not to enable this on other (non-win) platforms?

@jasnell

This comment has been minimized.

Show comment
Hide comment
@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Feb 2, 2017

@gibfahn

I was just thinking about the easiest way to test that this actually runs on CI for all platforms might be to change the Makefile for all platforms, run CI, and then change back.

Change and change back as in "commit and push" these changes to the repository? I don't think I'd want to test this change by committing/reverting, when we can test it by setting TEST_CI_ARGS, which is how I'd like us to use this functionality at least for now (see below).

So what's the reason not to enable this on other (non-win) platforms?

The reason for not enabling this on other non-windows platforms is that it's not clear whether all CI machines/platforms are correctly setup for storing and cleaning up core files. AFAIK, the SmartOS machines are correctly setup with nodejs/build#492.

I wouldn't mind to enable this on all non-windows platforms, but it seems introducing it gradually doesn't have any cons and allows us to not disrupt the CI platform too much.

What do you think?

@gibfahn

I was just thinking about the easiest way to test that this actually runs on CI for all platforms might be to change the Makefile for all platforms, run CI, and then change back.

Change and change back as in "commit and push" these changes to the repository? I don't think I'd want to test this change by committing/reverting, when we can test it by setting TEST_CI_ARGS, which is how I'd like us to use this functionality at least for now (see below).

So what's the reason not to enable this on other (non-win) platforms?

The reason for not enabling this on other non-windows platforms is that it's not clear whether all CI machines/platforms are correctly setup for storing and cleaning up core files. AFAIK, the SmartOS machines are correctly setup with nodejs/build#492.

I wouldn't mind to enable this on all non-windows platforms, but it seems introducing it gradually doesn't have any cons and allows us to not disrupt the CI platform too much.

What do you think?

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Feb 2, 2017

Member

Okay, fair enough. It's just that it's not really possible to run CI on this feature before landing right?

Member

gibfahn commented Feb 2, 2017

Okay, fair enough. It's just that it's not really possible to run CI on this feature before landing right?

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Feb 2, 2017

@gibfahn

It's just that it's not really possible to run CI on this feature before landing right?

It is possible to test it. I could clone e.g the test-commit-smartos job on Jenkins, make it point to the branch from this PR, and change the job configuration to set TEST_CI_ARGS to --abort-on-timeout. @nodejs/build Does anyone object to me running that test?

misterdjules commented Feb 2, 2017

@gibfahn

It's just that it's not really possible to run CI on this feature before landing right?

It is possible to test it. I could clone e.g the test-commit-smartos job on Jenkins, make it point to the branch from this PR, and change the job configuration to set TEST_CI_ARGS to --abort-on-timeout. @nodejs/build Does anyone object to me running that test?

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Feb 2, 2017

Member

I could clone e.g the test-commit-smartos job on Jenkins, make it point to the branch from this PR, and change the job configuration to set TEST_CI_ARGS to --abort-on-timeout. @nodejs/build Does anyone object to me running that test?

SGTM

Member

gibfahn commented Feb 2, 2017

I could clone e.g the test-commit-smartos job on Jenkins, make it point to the branch from this PR, and change the job configuration to set TEST_CI_ARGS to --abort-on-timeout. @nodejs/build Does anyone object to me running that test?

SGTM

Julien Gilli
test: add --abort-on-timeout option to test.py
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.

Refs: #11026
@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Feb 2, 2017

@bnoordhuis Updated to fix the nits you brought up.

@bnoordhuis Updated to fix the nits you brought up.

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Feb 2, 2017

A new temporary node-test-commit-smartos-timeout-on-abort Jenkins job was created.

It completed successfully for SmartOS 16 images, you can take a look at the console output and see that --abort-on-timeout was passed to tools/test.py.

The same tests are running for SmartOS 15 images. EDIT: these tests completed successfully too now.

Note that no test timed out though. We could introduce changes to make a test artificially time out and make sure it aborts. It seems it would be a departure from the current practice though, since AFAIK we currently don't have tests that test the tests driver.

misterdjules commented Feb 2, 2017

A new temporary node-test-commit-smartos-timeout-on-abort Jenkins job was created.

It completed successfully for SmartOS 16 images, you can take a look at the console output and see that --abort-on-timeout was passed to tools/test.py.

The same tests are running for SmartOS 15 images. EDIT: these tests completed successfully too now.

Note that no test timed out though. We could introduce changes to make a test artificially time out and make sure it aborts. It seems it would be a departure from the current practice though, since AFAIK we currently don't have tests that test the tests driver.

@mhdawson

LGTM

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn Feb 2, 2017

Member

I think a test suite for the test runner is definitely something we should have, but well outside the scope of this PR.

LGTM, thanks for doing the tests @misterdjules!

Member

gibfahn commented Feb 2, 2017

I think a test suite for the test runner is definitely something we should have, but well outside the scope of this PR.

LGTM, thanks for doing the tests @misterdjules!

jasnell added a commit that referenced this pull request Feb 3, 2017

test: add --abort-on-timeout option to test.py
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: #11086
Ref: #11026
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 3, 2017

Member

Landed in 3b488ec

Member

jasnell commented Feb 3, 2017

Landed in 3b488ec

@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Feb 3, 2017

Thanks everyone for the feedback, and @jasnell for merging it!

Thanks everyone for the feedback, and @jasnell for merging it!

misterdjules pushed a commit to misterdjules/node-1 that referenced this pull request Feb 3, 2017

Julien Gilli
test: fix test.py command line options processing
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

@misterdjules misterdjules referenced this pull request Feb 3, 2017

Closed

test: fix test.py command line options processing #11153

2 of 2 tasks complete
@misterdjules

This comment has been minimized.

Show comment
Hide comment
@misterdjules

misterdjules Feb 3, 2017

Unfortunately, this PR introduced a regression, which is documented and fixed by #11153. My apologies.

Unfortunately, this PR introduced a regression, which is documented and fixed by #11153. My apologies.

italoacasas added a commit that referenced this pull request Feb 4, 2017

test: add --abort-on-timeout option to test.py
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: #11086
Ref: #11026
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax added a commit that referenced this pull request Feb 4, 2017

test: fix test.py command line options processing
#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: #11086
PR-URL: #11153
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 5, 2017

test: fix test.py command line options processing
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

misterdjules pushed a commit to misterdjules/node-1 that referenced this pull request Feb 13, 2017

Julien Gilli
test: add --abort-on-timeout option to test.py
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 <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

misterdjules pushed a commit to misterdjules/node-1 that referenced this pull request Feb 13, 2017

Julien Gilli
test: fix test.py command line options processing
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: add --abort-on-timeout option to test.py
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 <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: fix test.py command line options processing
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

misterdjules pushed a commit that referenced this pull request Feb 15, 2017

Julien Gilli
test: add --abort-on-timeout option to test.py
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: #11086
Ref: #11026
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

misterdjules pushed a commit that referenced this pull request Feb 15, 2017

Julien Gilli
test: fix test.py command line options processing
#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: #11086
PR-URL: #11153
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

misterdjules pushed a commit that referenced this pull request Feb 15, 2017

Julien Gilli
test: add --abort-on-timeout option to test.py
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: #11086
Ref: #11026
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

misterdjules pushed a commit that referenced this pull request Feb 15, 2017

Julien Gilli
test: fix test.py command line options processing
#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: #11086
PR-URL: #11153
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request Feb 21, 2017

test: add --abort-on-timeout option to test.py
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: #11086
Ref: #11026
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request Feb 21, 2017

test: fix test.py command line options processing
#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: #11086
PR-URL: #11153
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request Feb 21, 2017

test: add --abort-on-timeout option to test.py
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: #11086
Ref: #11026
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

MylesBorins added a commit that referenced this pull request Feb 21, 2017

test: fix test.py command line options processing
#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: #11086
PR-URL: #11153
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

test: add --abort-on-timeout option to test.py
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 <jasnell@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

test: fix test.py command line options processing
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

@misterdjules misterdjules deleted the misterdjules:tests-kill-abort branch Jul 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment