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.

Sign up for GitHub

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

Jump to bottom

test: fix flaky test-resolve-async #17957

Closed
wants to merge 3 commits into from
Closed

test: fix flaky test-resolve-async #17957

wants to merge 3 commits into from

Conversation

apapirovski
Copy link
Member

@apapirovski apapirovski commented Jan 3, 2018 •
edited
Loading

As I understand it, this test doesn't require the setTimeout and can do just fine with setImmediate. Looks like timeout might be needed after all, let's see if slightly higher setting will do.

This should fix the flakiness seen in https://ci.nodejs.org/job/node-test-commit-linux/15333/nodes=alpine37-container-x64/tapResults/

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

test

@apapirovski apapirovski requested a review from addaleax January 3, 2018 01:37
@nodejs-github-bot nodejs-github-bot added addons Issues and PRs related to native addons. test Issues and PRs related to the tests. labels Jan 3, 2018
@apapirovski
Copy link
Member Author

apapirovski commented Jan 3, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/12392/ (green)
Stress test CI: https://ci.nodejs.org/job/node-stress-single-test/1602/ (green)

@apapirovski apapirovski force-pushed the fix-test-addons-resolve-async branch from ada4921 to 766322d Compare January 3, 2018 02:33
@Trott
Copy link
Member

Trott commented Jan 3, 2018 •
edited
Loading

FWIW, if the setTimeout() is needed, then the common.mustCall() is superfluous.

@apapirovski
Copy link
Member Author

apapirovski commented Jan 3, 2018 •
edited
Loading

@Trott good point, I think this test actually doesn't need the common.mustCall in either location. The one within the then call is made obsolete by the requirement that the called variable must be set to true and is checked within setTimeout.

(I think the reason setTimeout is used to check is to avoid promise microtasks being triggered from JS before the check happens, but maybe I misunderstood this test. Part of the reason I requested a review from @addaleax who created this originally.)

Trott
Trott reviewed Jan 3, 2018
View reviewed changes
test/addons/callback-scope/test-resolve-async.js Outdated
testResolveAsync().then(common.mustCall(() => {
called = true;
}));
testResolveAsync().then(() => (called = true));
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for the parentheses here instead of the seemingly-more-idiomatic curly braces?

Trott
Trott reviewed Jan 3, 2018
View reviewed changes
test/addons/callback-scope/test-resolve-async.js Outdated

setTimeout(common.mustCall(() => { assert(called); }),
common.platformTimeout(20));
setTimeout(() => assert(called), common.platformTimeout(50));
Copy link
Member

Choose a reason for hiding this comment

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

My usual comment: I'd prefer the assert(called) be wrapped in curly braces to make it clear that there is no intention of returning a value here. I suspect I'm on the losing end of this particular unimportant style debate. :-P

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't have a strong preference so I've gone ahead and changed it.

@apapirovski apapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 3, 2018
mhdawson
mhdawson approved these changes Jan 3, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR BridgeAR added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 5, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 5, 2018
@apapirovski @BridgeAR
test: fix flaky test-resolve-async
d72c78b 
PR-URL: nodejs#17957
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR
Copy link
Member

BridgeAR commented Jan 5, 2018 •
edited
Loading

Landed in d72c78b

CI https://ci.nodejs.org/job/node-test-commit-light/97/

@BridgeAR BridgeAR closed this Jan 5, 2018
@apapirovski apapirovski deleted the fix-test-addons-resolve-async branch January 5, 2018 02:23
MylesBorins pushed a commit that referenced this pull request Jan 8, 2018
@apapirovski @MylesBorins
test: fix flaky test-resolve-async
fcdb0af 
PR-URL: #17957
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@apapirovski @MylesBorins
test: fix flaky test-resolve-async
3d9ff31 
PR-URL: #17957
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 9, 2018
@apapirovski @MylesBorins
test: fix flaky test-resolve-async
3cc9882 
PR-URL: #17957
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
Merged
@TimothyGu TimothyGu removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 13, 2018
MylesBorins pushed a commit that referenced this pull request Jan 24, 2018
@apapirovski @MylesBorins
test: fix flaky test-resolve-async
d472704 
PR-URL: #17957
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@mhdawson mhdawson mhdawson approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@hiroppy hiroppy hiroppy approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
addons Issues and PRs related to native addons. fast-track PRs that do not need to wait for 48 hours to land. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.