Skip to content

test: fix flaky test-force-repl#8484

Closed
Trott wants to merge 3 commits intonodejs:masterfrom
Trott:repl5
Closed

test: fix flaky test-force-repl#8484
Trott wants to merge 3 commits intonodejs:masterfrom
Trott:repl5

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 Outdated
cp.stdout.setEncoding('utf8');

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

@targos targos Sep 10, 2016

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

@Trott Trott Sep 10, 2016

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 Outdated
cp.stdout.once('data', function(b) {
cp.stdout.once('data', common.mustCall(function(b) {
clearTimeout(timeoutId);
assert.equal(b, '> ');
Copy link
Member

@addaleax addaleax Sep 10, 2016

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

@Trott Trott Sep 10, 2016

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

addaleax commented Sep 10, 2016

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

santigimeno commented Sep 11, 2016

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 <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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 <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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