FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: fix test-cluster-worker-init.js flakyness by imyller · Pull Request #8703 · nodejs/node · GitHub

/ node Public

test: fix test-cluster-worker-init.js flakyness - #8703

Closed
imyller wants to merge 1 commit into
nodejs:masterfrom
imyller:p1-stress
ConversationCommits (1)ChecksFiles changed
Closed

test: fix test-cluster-worker-init.js flakyness#8703
imyller wants to merge 1 commit into
nodejs:masterfrom
imyller:p1-stress

Conversation

imyller commented Sep 21, 2016 •
edited
Loading

Copy link
Copy Markdown
Member
Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test, cluster

Description of change

Update test to match current test guidelines and use common.mustCall
instead of unref'd timer.

Fixes: #8700

nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 21, 2016
imyller added the cluster Issues and PRs related to the cluster subsystem. label Sep 21, 2016

imyller commented Sep 21, 2016

Copy link
Copy Markdown
Member Author

/cc @nodejs/testing

imyller commented Sep 21, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

CI stress test with pi1:
100 runs: https://ci.nodejs.org/job/node-stress-single-test-pi1-fanned/17/ (passed)
999 runs: https://ci.nodejs.org/job/node-stress-single-test-pi1-fanned/19/ (passed, but aborted after 328 runs for unrelated reasons)

cjihrig requested changes Sep 21, 2016
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm very +1 for removing timers from tests.

Comment thread test/parallel/test-cluster-worker-init.js Outdated

cjihrig Sep 21, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this is the only common.mustCall() you need.

imyller Sep 21, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'd prefer to keep the other common.mustCall()'s because they are actually mandatory for passing the test too. At least it gives more granularity to error msg if test fails if we have them.

imyller Sep 21, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Also, earlier we had no clear picture what phase actually failed. I wanted to improve that situation.

cjihrig Sep 21, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I understand. But I also think that introduces a slippery slope where literally every callback in every test is inside of a common.mustCall() (since it shouldn't be in the test if it isn't necessary).

EDIT: With the exception of common.fail cases.

imyller Sep 21, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sure, I'll modify and I understand your concern.

My personal opinion still remains that in cases where verifying sequence of events is important, common.mustCall can be present in the middle-steps too. I categorized this test to be one of those cases.

Comment thread test/parallel/test-cluster-worker-init.js Outdated

cjihrig Sep 21, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please change this to assert.strictEqual() to prevent surprises.

imyller Sep 21, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sure thing. It will be in the format

assert.strictEqual(message, true, 'did not receive expected message');

because worker sends true as message value.

cjihrig Sep 21, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Right. I just wanted to rule out any other truthy value somehow showing up. Thanks for making the changes.

imyller commented Sep 21, 2016

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/4200/

test: fix test-cluster-worker-init.js flakyness
0dbbdc0 
Update test to match current test guidelines and use common.mustCall
instead of unref'd timer.

Fixes: nodejs#8700

imyller commented Sep 21, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

@cjihrig Please re-review at your convenience. The changes you requested are in.

cjihrig approved these changes Sep 21, 2016
View reviewed changes

cjihrig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The change to arrow functions seems pointless, but I won't fight it. LGTM

imyller commented Sep 21, 2016

Copy link
Copy Markdown
Member Author

New CI after requested modifications: https://ci.nodejs.org/job/node-test-pull-request/4202/

Trott commented Sep 22, 2016

Copy link
Copy Markdown
Member

LGTM

imyller commented Sep 22, 2016

Copy link
Copy Markdown
Member Author

@rvagg had to boot CI Pi's (for unrelated reasons) so stress test aborted after 328 runs. Total we have 428 successfull stress test runs.

Do you feel that we need to complete full 999 cycle in the stress test CI for this one?

Trott commented Sep 22, 2016

Copy link
Copy Markdown
Member

Do you feel that we need to complete full 999 cycle in the stress test CI for this one?

Not in this case. I think it's fine.

Trott commented Sep 22, 2016

Copy link
Copy Markdown
Member

Stress test against master: https://ci.nodejs.org/job/node-stress-single-test-pi1-binary/15/label=pi1-raspbian-wheezy/console

Failed 10 out of 100 runs with current master. So 328 successful runs with 0 failures using the code change in this PR seems pretty convincing.

santigimeno approved these changes Sep 22, 2016
View reviewed changes

santigimeno left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

imyller commented Sep 22, 2016

Copy link
Copy Markdown
Member Author

@Trott Just asking: do we apply 48 hour window to this PR or does arm-fanned CI flakyness require more urgent landing? LGTMs are there and CI is green.

Trott commented Sep 22, 2016

Copy link
Copy Markdown
Member

I'm inclined to wait. There's still plenty of other armv6 failures to sort out, so waiting another 24 hours on this one isn't going to hurt anything. However, if anyone on @nodejs/testing feels differently, I will defer to them on it.

imyller self-assigned this Sep 23, 2016

imyller commented Sep 23, 2016 •
edited
Loading

Copy link
Copy Markdown
Member Author

Landing:

  • Three LGTMs
  • No objections
  • Requested changes have been made
  • CI test passed (only the known CI failures)

imyller commented Sep 23, 2016

Copy link
Copy Markdown
Member Author

landed in 66369d0

imyller closed this Sep 23, 2016
imyller removed their assignment Sep 23, 2016
imyller added a commit that referenced this pull request Sep 23, 2016
test: fix test-cluster-worker-init.js flakyness
66369d0 
Update test to match current test guidelines and use common.mustCall
instead of unref'd timer.

PR-URL: #8703
Fixes: #8700
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
jasnell pushed a commit that referenced this pull request Sep 29, 2016
test: fix test-cluster-worker-init.js flakyness
9a6b779 
Update test to match current test guidelines and use common.mustCall
instead of unref'd timer.

PR-URL: #8703
Fixes: #8700
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
test: fix test-cluster-worker-init.js flakyness
5540e3d 
Update test to match current test guidelines and use common.mustCall
instead of unref'd timer.

PR-URL: #8703
Fixes: #8700
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 18, 2016
test: fix test-cluster-worker-init.js flakyness
cec5e36 
Update test to match current test guidelines and use common.mustCall
instead of unref'd timer.

PR-URL: #8703
Fixes: #8700
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
MylesBorins mentioned this pull request Nov 22, 2016
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

santigimeno santigimeno approved these changes
cjihrig cjihrig approved these changes

Assignees

No one assigned

Labels

cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Investigate flaky test-cluster-worker-init on Raspberry Pi 1

6 participants

Back | FazBrowse Home | New Git URL