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.

Sign up for GitHub

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

Jump to bottom

Fix flaky sequential/test-domain-abort-on-uncaught and move back to parallel #11826

Closed
gibfahn opened this issue Mar 13, 2017 · 6 comments
Closed

Fix flaky sequential/test-domain-abort-on-uncaught and move back to parallel #11826

gibfahn opened this issue Mar 13, 2017 · 6 comments
Labels
arm Issues and PRs related to the ARM platform. domain Issues and PRs related to the domain subsystem. test Issues and PRs related to the tests.

Comments

@gibfahn
Copy link
Member

gibfahn commented Mar 13, 2017

  • Version: master
  • Platform: arm
  • Subsystem: test

See #11814 (comment)

This test is flaky under load. This can be seen by running it with a command like this:

tools/test.py -j 32 --repeat=32 test/parallel/test-domain-abort-on-uncaught.js

(If 32 isn't enough to cause problems on your setup, try 64 or higher.)

There are at least three solutions possible here:

  • move the test to sequential Done in test: fix flaky test-domain-abort-on-uncaught #11817
  • break the test up into as many as 13 separate test files so that each one is launching one additional node process rather than one test launching 13 of them in parallel
  • rewrite the test to run the 13 processes sequentially rather than in parallel

The second or third option above can be a good first contribution for someone.

How to fix

  • Read through the linked issue and understand the problem.
  • Replicate the failure on your machine.
  • Fix the test (probably using one of the methods suggested above).
  • Move the test back to test/parallel/ (it's currently in test/sequential/).
  • Follow CONTRIBUTING.md and submit a PR with the fix.

If you want to have a go at this comment on this issue to let us know. If you have any questions ask them here.
@gibfahn gibfahn added arm Issues and PRs related to the ARM platform. domain Issues and PRs related to the domain subsystem. good first issue Issues that are suitable for first-time contributors. labels Mar 13, 2017
@gibfahn gibfahn mentioned this issue Mar 13, 2017
Closed
@mscdex mscdex added the test Issues and PRs related to the tests. label Mar 13, 2017
@clarenced
Copy link
Contributor

Hi @gibfahn I would like to fix this issue, I have already done a PR. But it seems the file test-domain-abort-on-uncaught.js is still in test/parallel. Can you confirm that?

@gibfahn
Copy link
Member Author

gibfahn commented Mar 14, 2017 •
edited
Loading

@clarenced yes, #11817 hasn't landed yet (and probably shouldn't if you've already raised a PR). So you can fix the test and leave it in parallel.

EDIT: Let me know the number when you raise it.

cc/ @Trott

@Trott
Copy link
Member

Trott commented Mar 14, 2017

I plan on landing #11817 in another 18 hours or so.

@gibfahn
Copy link
Member Author

gibfahn commented Mar 17, 2017 •
edited
Loading

@clarenced #11817 has landed, which will mean that your branch will need to be rebased on master.

@clarenced
Copy link
Contributor

Hi @gibfahn and @Trott, I created a pull request for this issue #11920

@clarenced clarenced mentioned this issue Mar 18, 2017
Closed
2 tasks
Trott added a commit to Trott/io.js that referenced this issue Jul 30, 2017
@Trott
test: refactor test-domain-abort-on-uncaught
8f0f403 
* use common.mustCall() instead of exit handler
* use execSync instead of exec so test is reliable under load
* move from sequential to parallel

Fixes: nodejs#11826
@Trott Trott mentioned this issue Jul 30, 2017
Closed
2 tasks
@Trott
Copy link
Member

Trott commented Jul 30, 2017

Proposed fix in #14541

@Trott Trott removed the good first issue Issues that are suitable for first-time contributors. label Jul 30, 2017
@Trott Trott closed this as completed in 548cc72 Aug 2, 2017
addaleax pushed a commit that referenced this issue Aug 7, 2017
@Trott @addaleax
test: refactor test-domain-abort-on-uncaught
5094f2c 
* use common.mustCall() instead of exit handler
* use execSync instead of exec so test is reliable under load
* move from sequential to parallel

PR-URL: #14541
Fixes: #11826
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
@Trott @MylesBorins
test: refactor test-domain-abort-on-uncaught
9c59f1c 
* use common.mustCall() instead of exit handler
* use execSync instead of exec so test is reliable under load
* move from sequential to parallel

PR-URL: #14541
Fixes: #11826
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Aug 16, 2017
@Trott @MylesBorins
test: refactor test-domain-abort-on-uncaught
014a9bd 
* use common.mustCall() instead of exit handler
* use execSync instead of exec so test is reliable under load
* move from sequential to parallel

PR-URL: #14541
Fixes: #11826
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Sep 3, 2017
@Trott @MylesBorins
test: refactor test-domain-abort-on-uncaught
f0b3bf3 
* use common.mustCall() instead of exit handler
* use execSync instead of exec so test is reliable under load
* move from sequential to parallel

PR-URL: #14541
Fixes: #11826
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Sep 5, 2017
@Trott @MylesBorins
test: refactor test-domain-abort-on-uncaught
2ce80d9 
* use common.mustCall() instead of exit handler
* use execSync instead of exec so test is reliable under load
* move from sequential to parallel

PR-URL: #14541
Fixes: #11826
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
arm Issues and PRs related to the ARM platform. domain Issues and PRs related to the domain subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

test: refactor test-domain-abort-on-uncaught Trott/io.js
4 participants
@mscdex @clarenced @Trott @gibfahn