-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: remove flaky test functionality #812
Conversation
+1 Any idea what sort of tests will fail after this? |
I don't believe any will, we didn't merge in the list of flaky tests and went ahead and fixed those tests anyway https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/171/ |
Hmmm, some strange errors are cropping up on that CI run. Tested with & without the patch on OS X 10.10 and everything seems fine. |
@Fishrock123 The add ons probably aren't build before run. Possibly separate test-addons from |
yes, I put |
LGTM |
I'm all for removing the flaky stuff too. |
+1 and LGTM |
Re-running the CI on this since some failures seemed unrelated: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/174/ |
Not sure if relevant, but this cropped up on
|
@Fishrock123 already filed as #790 and dupe @ #809 |
Reverts nodejs/node-v0.x-archive#8689 PR-URL: #812 Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
landed via 20f8e7f |
test something PR-URL: nodejs/node#812
Reverts nodejs/node-v0.x-archive#8689
I'm proposing that we remove this embarrassing functionality. If we need have special cases for tests then we should have that explicitly documented and implemented in the tests themselves.