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.

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

test: new flakiness in test-process-kill-null.js #14141

Closed
refack opened this issue Jul 9, 2017 · 3 comments · Fixed by #14142
Closed

test: new flakiness in test-process-kill-null.js #14141

refack opened this issue Jul 9, 2017 · 3 comments · Fixed by #14142
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. process Issues and PRs related to the process subsystem. regression Issues related to regressions. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.

Comments

@refack
Copy link
Contributor

refack commented Jul 9, 2017

  • Version: master
  • Platform: Windows
  • Subsystem: test,process

44483b6 introduced flakiness to test/parallel/test-process-kill-null.js.
Looking for advice on the best course of action:

  1. Revert & reopen PR.
  2. Fix.
  3. Mark as flaky.

/cc @nodejs/testing @nodejs/platform-windows

@refack refack self-assigned this Jul 9, 2017
@refack refack added flaky-test Issues and PRs related to the tests with unstable failures on the CI. process Issues and PRs related to the process subsystem. regression Issues related to regressions. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform. labels Jul 9, 2017
@Trott
Copy link
Member

Trott commented Jul 9, 2017

When in doubt, I'd say revert & reopen PR.

I imagine if you were confident that you could fix it quickly (or determined to do so), then that's what you'd do and you wouldn't ask. :-D

And I'd reserve "mark as flaky" for situations where we do not know how to fix the test. If reverting fixes the flakiness, I'd say that's preferable. green > yellow

@Trott
Copy link
Member

Trott commented Jul 9, 2017

Oh, I guess another factor is just how flaky. Like, if it's one in a thousand runs, leaving it open while there's an active effort to fix it over a few days is A-OK to me. If it's one in 10, OMG no, revert.

@refack refack mentioned this issue Jul 9, 2017
3 tasks
@refack
Copy link
Contributor Author

refack commented Jul 9, 2017

I guess it's more in the 1 in 10 range so #14142.
Maybe even a real bug...
Now I can sleep on this more peacefully ...

refack added a commit to refack/node that referenced this issue Jul 9, 2017
This reverts commit 44483b6.

PR-URL: nodejs#14142
Fixes: nodejs#14141
Refs: nodejs#14099
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@refack refack removed their assignment Oct 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. process Issues and PRs related to the process subsystem. regression Issues related to regressions. test Issues and PRs related to the tests. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants