Skip to content

Conversation

@Trott
Copy link
Member

Remove unnecessary 1-second delay from test. Confirmed that test (with changes to eliminate ES6-isms in common.js) still fails in v1.8.1 (which has the bug this test was written for) and passes in v2.3.4 (which contains the fix).

R=@sam-github

@TrottTrott added cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests. labels Jan 11, 2016
@Trott
Copy link
MemberAuthor

@Trott
Copy link
MemberAuthor

Booooo! Blowing up on SmartOS. Still think this is workable. Will revise. Closing for now. Might re-open.

@TrottTrott closed this Jan 11, 2016
@TrottTrott reopened this Jan 11, 2016
@Trott
Copy link
MemberAuthor

OK, pushed a modification, and this is looking much better. https://ci.nodejs.org/job/node-test-commit/1684/

@Trott
Copy link
MemberAuthor

As a side note, this test is flaky, at least on FreeBSD.

(One of the motivations for this is that one additional second per test run starts to add up when you're doing 10K test runs.)

I have a subsequent modification to tack on to this one that will hopefully fix the flakiness. Stress test currently running at https://ci.nodejs.org/job/node-stress-single-test/333/nodes=freebsd102-64/console. Assuming it works out, I can lump it in with this PR or keep it as a separate PR. Doesn't really matter either way to me.

@Trott
Copy link
MemberAuthor

Stress test for the additional change is looking great. Going to cherry-pick it into this PR...

@Trott
Copy link
MemberAuthor

Cherry-picked. New CI: https://ci.nodejs.org/job/node-test-pull-request/1190/

@Trott
Copy link
MemberAuthor

R=@jbergstroem (This is one of the tests that failed in one of the CI runs for the make-it-all-parallel PR you have open.)

@jbergstroem
Copy link
Member

Cool. Looks green indeed. Lets wait for the last arm bot to finish (just got it back online) - but LGTM.

@jasnell
Copy link
Member

LGTM if CI is green

@Trott
Copy link
MemberAuthor

Hmmm...the stress test failed, albeit with a bit less frequency than the previous version of the test. So, it's better, but it's still not there yet. More work to be done, alas.

@Trott
Copy link
MemberAuthor

OK, revised to get rid of flakiness (again).

@jbergstroem
Copy link
Member

Afaik there are no known infra issues. That's too many fails to just have one bot act up as well.

@Trott
Copy link
MemberAuthor

@jbergstroem Yup. Looks like it's a result of #4575. Looks like it got merged even though there were test failures. At least, that's what I'm seeing. Maybe I'm missing something. (I've certainly been known to be wrong on my what-went-wrong diagnosis in these situations.)

@Trott
Copy link
MemberAuthor

Stress test came out green. Since I made some changes, can I get a reaffirmation of the LGTMs? @jasnell@jbergstroem

(Yes, I'll make sure to squash the four commits down to one before landing.)

@jbergstroem
Copy link
Member

LGTM

@jasnell
Copy link
Member

Still LGTM

Trott added a commit to Trott/io.js that referenced this pull request Jan 13, 2016
In test-cluster-worker-wait-server-close, remove unneeded 1-second delay and refactor to eliminate flakiness on FreeBSD. PR-URL: nodejs#4616 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
MemberAuthor

Landed in e98bcfa

@TrottTrott closed this Jan 13, 2016
rvagg pushed a commit that referenced this pull request Jan 14, 2016
In test-cluster-worker-wait-server-close, remove unneeded 1-second delay and refactor to eliminate flakiness on FreeBSD. PR-URL: #4616 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 28, 2016
In test-cluster-worker-wait-server-close, remove unneeded 1-second delay and refactor to eliminate flakiness on FreeBSD. PR-URL: #4616 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 11, 2016
In test-cluster-worker-wait-server-close, remove unneeded 1-second delay and refactor to eliminate flakiness on FreeBSD. PR-URL: #4616 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 11, 2016
In test-cluster-worker-wait-server-close, remove unneeded 1-second delay and refactor to eliminate flakiness on FreeBSD. PR-URL: nodejs#4616 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Feb 11, 2016
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Feb 15, 2016
In test-cluster-worker-wait-server-close, remove unneeded 1-second delay and refactor to eliminate flakiness on FreeBSD. PR-URL: nodejs#4616 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
In test-cluster-worker-wait-server-close, remove unneeded 1-second delay and refactor to eliminate flakiness on FreeBSD. PR-URL: nodejs#4616 Reviewed-By: Johan Bergström <[email protected]> Reviewed-By: James M Snell <[email protected]>
@TrottTrott deleted the 1-second branch October 15, 2021 13:26
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clusterIssues and PRs related to the cluster subsystem.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Trott@jbergstroem@jasnell@MylesBorins