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-force-repl #8484

Closed
wants to merge 3 commits into from
Closed

test: fix flaky test-force-repl #8484

wants to merge 3 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 10, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test repl

Description of change

Increase time allowed for startup from 1 second to 5 seconds to avoid
occasional flakiness. While at it, refactor a few minor things such as
var->const and using common.mustCall().

Fixes: #8483

@Trott
test: fix flaky test-force-repl
4dfb44b 
Increase time allowed for startup from 1 second to 5 seconds to avoid
occasional flakiness. While at it, refactor a few minor things such as
var->const and using common.mustCall().

Fixes: nodejs#8483
@Trott Trott added repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests. labels Sep 10, 2016
@Trott
Copy link
Member Author

Trott commented Sep 10, 2016 •
edited
Loading

Stress test on master: https://ci.nodejs.org/job/node-stress-single-test/906/nodes=win10/console

Stress test with this change: https://ci.nodejs.org/job/node-stress-single-test/907/nodes=win10/console

EDIT: Well those are not the results I expected...

targos
targos reviewed Sep 10, 2016
View reviewed changes
test/parallel/test-force-repl.js

cp.stdout.setEncoding('utf8');

cp.stdout.once('data', function(b) {
cp.stdout.once('dasta', common.mustCall(function(b) {
Copy link
Member

Choose a reason for hiding this comment

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

dasta?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha! I added that to make sure the test fails if the callback never fired but then checked it in that way by accident.

@Trott
Copy link
Member Author

Trott commented Sep 10, 2016

Let's try again:

master: https://ci.nodejs.org/job/node-stress-single-test/908/nodes=win10-1p/console

this branch: https://ci.nodejs.org/job/node-stress-single-test/909/nodes=win10-1p/console

addaleax
addaleax reviewed Sep 10, 2016
View reviewed changes
test/parallel/test-force-repl.js

cp.stdout.setEncoding('utf8');

cp.stdout.once('data', function(b) {
cp.stdout.once('data', common.mustCall(function(b) {
clearTimeout(timeoutId);
assert.equal(b, '> ');
Copy link
Member

Choose a reason for hiding this comment

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

If you’re refactoring this anyway you might as well use strictEqual here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

If you’re refactoring this anyway you might as well use strictEqual here :)

Hey, shame on me. Yeah, I'll change equal() -> strictEqual().

@addaleax
Copy link
Member

I’d be curious how well this test does with no timeout at all… is there a specific reason this test has one? git blame says it’s always been there.

@Trott
Copy link
Member Author

Trott commented Sep 10, 2016

The timeout is there to facilitate running it on the command line without the test.py script. A failure will print a message rather than just hanging. I would actually prefer no timeout and let it hang on the command line if it's a failure. That's an infrequent problem and the CI issues caused by timers have been enormous in the past. But I've gotten push back on that, so I tend not to do it unless it's a particularly egregious case. However, if someone else removed timers like this, I'd be all +1 LGTM :shipit:!. :-D

@santigimeno
Copy link
Member

LGTM.
FWIW, I'm +1 to remove the timeout right away.

@jasnell
Copy link
Member

jasnell commented Sep 12, 2016

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Sep 14, 2016
@Trott
test: fix flaky test-force-repl
ad1a9dd 
Increase time allowed for startup from 1 second to 5 seconds to avoid
occasional flakiness. While at it, refactor a few minor things such as
var->const and using common.mustCall().

Fixes: nodejs#8483
PR-URL: nodejs#8484
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member Author

Trott commented Sep 14, 2016

Landed in ad1a9dd

@Trott Trott closed this Sep 14, 2016
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
@Trott
test: fix flaky test-force-repl
719453e 
Increase time allowed for startup from 1 second to 5 seconds to avoid
occasional flakiness. While at it, refactor a few minor things such as
var->const and using common.mustCall().

Fixes: #8483
PR-URL: #8484
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
@Trott
test: fix flaky test-force-repl
7d6ce04 
Increase time allowed for startup from 1 second to 5 seconds to avoid
occasional flakiness. While at it, refactor a few minor things such as
var->const and using common.mustCall().

Fixes: #8483
PR-URL: #8484
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@Trott @Fishrock123
test: fix flaky test-force-repl
ff238c8 
Increase time allowed for startup from 1 second to 5 seconds to avoid
occasional flakiness. While at it, refactor a few minor things such as
var->const and using common.mustCall().

Fixes: #8483
PR-URL: #8484
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
@Trott @rvagg
test: fix flaky test-force-repl
c77f966 
Increase time allowed for startup from 1 second to 5 seconds to avoid
occasional flakiness. While at it, refactor a few minor things such as
var->const and using common.mustCall().

Fixes: #8483
PR-URL: #8484
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
@Trott @MylesBorins
test: fix flaky test-force-repl
69404ec 
Increase time allowed for startup from 1 second to 5 seconds to avoid
occasional flakiness. While at it, refactor a few minor things such as
var->const and using common.mustCall().

Fixes: #8483
PR-URL: #8484
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Closed
@Trott Trott deleted the repl5 branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
repl Issues and PRs related to the REPL 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 parallel/test-force-repl on Windows
6 participants
@Trott @addaleax @santigimeno @jasnell @targos @MylesBorins