Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

test: make test-abort-fatal-error non flaky #25755

Conversation

misterdjules
Copy link

Before this change, test/simple/test-abort-fatal-error.js would fail in
some environments for reasons I wasn't able to fully understand. It was
marked as flaky on some systems, but not on others on which it was
failing sometimes (OSX).

This change basically syncs test-abort-fatal-error with how it's
implemented in v0.12. It back ports 429b587 (or rather the parts that
apply to it since it's a merge commit), 2f5e77f and 114bff4.

After backporting these changes in v0.10, test-abort-fatal-error is not
flaky anymore in environments for which it was flaky. It also has the
added benefit of being more robust because it checks exit codes and
signals instead of error messages.

Tested on OSX and SmartOS, the only platforms on which I could reproduce
the flakiness of this test.

This change also removes test-abort-fatal-error from the list of flaky
tests in test/simple/simple.status.

Fixes #25720.

@misterdjules
Copy link
Author

This change is needed to land #25569.

@misterdjules
Copy link
Author

/cc @joyent/node-collaborators

@joaocgreis
Copy link
Member

@misterdjules Commit message: you mention 429b5870q (extra q), but also include changes made in 2f5e77f and 114bff4. Maybe change that paragraph to something like (edit as you see fit):

This change back ports this test as it was in v0.12 at 114bff46. The
test was changed to adapt to changes in V8 in the v0.12 branch.

@joaocgreis
Copy link
Member

Tested on SmartOS, LGTM.

@misterdjules misterdjules force-pushed the make-abort-on-fatal-error-not-flaky branch from 76c69b3 to d1ea245 Compare July 23, 2015 16:51
Before this change, test/simple/test-abort-fatal-error.js would fail in
some environments for reasons I wasn't able to fully understand. It was
marked as flaky on some systems, but not on others on which it was
failing sometimes (OSX).

This change basically syncs test-abort-fatal-error with how it's
implemented in v0.12. It back ports 429b587 (or rather the parts that
apply to it since it's a merge commit), 2f5e77f and 114bff4.

After backporting these changes in v0.10, test-abort-fatal-error is not
flaky anymore in environments for which it was flaky. It also has the
added benefit of being more robust because it checks exit codes and
signals instead of error messages.

Tested on OSX and SmartOS, the only platforms on which I could reproduce
the flakiness of this test.

This change also removes test-abort-fatal-error from the list of flaky
tests in test/simple/simple.status.

Fixes nodejs#25720.
@misterdjules misterdjules force-pushed the make-abort-on-fatal-error-not-flaky branch from d1ea245 to a8e2a76 Compare July 23, 2015 16:52
@misterdjules
Copy link
Author

Good catch and thank you @joaocgreis 👍 Updated the commit message, will land asap.

misterdjules pushed a commit to misterdjules/node that referenced this pull request Jul 23, 2015
Before this change, test/simple/test-abort-fatal-error.js would fail in
some environments for reasons I wasn't able to fully understand. It was
marked as flaky on some systems, but not on others on which it was
failing sometimes (OSX).

This change basically syncs test-abort-fatal-error with how it's
implemented in v0.12. It back ports 429b587 (or rather the parts that
apply to it since it's a merge commit), 2f5e77f and 114bff4.

After backporting these changes in v0.10, test-abort-fatal-error is not
flaky anymore in environments for which it was flaky. It also has the
added benefit of being more robust because it checks exit codes and
signals instead of error messages.

Tested on OSX and SmartOS, the only platforms on which I could reproduce
the flakiness of this test.

This change also removes test-abort-fatal-error from the list of flaky
tests in test/simple/simple.status.

Fixes nodejs#25720.

PR: nodejs#25755
PR-URL: nodejs#25755
Reviewed-By: João Reis <reis@janeasystems.com>
@misterdjules
Copy link
Author

Landed in e10892c.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants