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: remove race condition in http flood test #4793

Closed
wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 21, 2016

Timer race results in some flakiness on slower devices in CI.

Timer race results in some flakiness on slower devices in CI.
@Trott Trott added http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. labels Jan 21, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Jan 21, 2016

LGTM if it works. Even if it doesn't work, nice to get those booleans out of there.

@jasnell
Copy link
Member

jasnell commented Jan 21, 2016

LGTM

@Trott
Copy link
Member Author

Trott commented Jan 21, 2016

@Trott
Copy link
Member Author

Trott commented Jan 21, 2016

Will also need to back out the ES6-isms and confirm that it fails on Node 0.10.20 and passes on Node 0.10.21. Should probably add a comment about that.

@Trott
Copy link
Member Author

Trott commented Jan 24, 2016

Alas, the test passes in 0.10.20, albeit after a very, very, very long time. Time to tweak the test some more...

@Trott
Copy link
Member Author

Trott commented Jan 24, 2016

Checking that drain only fires once seems to make it fail reliably in 0.10.20 and pass in 0.10.21.

Running new version through CI: https://ci.nodejs.org/job/node-test-pull-request/1358/

PTAL @jasnell @cjihrig

@Trott
Copy link
Member Author

Trott commented Jan 24, 2016

Raspberry Pi failure on CI, so that didn't do it either.

@Trott
Copy link
Member Author

Trott commented Jan 24, 2016

Putting the socket timeout back to 200ms and re-running gives a pass: https://ci.nodejs.org/job/node-test-commit/1897/

Would like to run a stress test on Raspberry Pi to get a larger data set, though.

@Trott
Copy link
Member Author

Trott commented Jan 26, 2016

I think this is ready to go, but since it's changed so much since the LGTM's, I'd like to just make sure it still gets 'em. @cjihrig @jasnell ?

@cjihrig
Copy link
Contributor

cjihrig commented Jan 27, 2016

Yep, LGTM if it works.

@Trott
Copy link
Member Author

Trott commented Jan 27, 2016

Since the build team (Johan, I think) got Jenkins all snappy tonight, I'll try another CI run and HOPE FOR GREEN: https://ci.nodejs.org/job/node-test-pull-request/1390/

@Trott
Copy link
Member Author

Trott commented Jan 27, 2016

Here we go: https://ci.nodejs.org/job/node-test-pull-request/1395/

Green! \o/

Trott added a commit to Trott/io.js that referenced this pull request Jan 27, 2016
Timer race results in some flakiness on slower devices in CI. Remove
unneeded setTimeout() and replace booleans with common.mustCall().

PR-URL: nodejs#4793
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jan 27, 2016

Landed in cee14f5

@Trott Trott closed this Jan 27, 2016
rvagg pushed a commit that referenced this pull request Jan 28, 2016
Timer race results in some flakiness on slower devices in CI. Remove
unneeded setTimeout() and replace booleans with common.mustCall().

PR-URL: #4793
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 8, 2016
Timer race results in some flakiness on slower devices in CI. Remove
unneeded setTimeout() and replace booleans with common.mustCall().

PR-URL: #4793
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
Timer race results in some flakiness on slower devices in CI. Remove
unneeded setTimeout() and replace booleans with common.mustCall().

PR-URL: #4793
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Timer race results in some flakiness on slower devices in CI. Remove
unneeded setTimeout() and replace booleans with common.mustCall().

PR-URL: #4793
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Timer race results in some flakiness on slower devices in CI. Remove
unneeded setTimeout() and replace booleans with common.mustCall().

PR-URL: #4793
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Timer race results in some flakiness on slower devices in CI. Remove
unneeded setTimeout() and replace booleans with common.mustCall().

PR-URL: nodejs#4793
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott Trott deleted the no-really branch January 13, 2022 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants