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

Flaky parallel/test-repl-history-navigation #31094

Closed
BridgeAR opened this issue Dec 25, 2019 · 3 comments
Closed

Flaky parallel/test-repl-history-navigation #31094

BridgeAR opened this issue Dec 25, 2019 · 3 comments
Assignees
@BridgeAR
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests.

Comments

@BridgeAR
Copy link
Member

I added this test recently and it actually checks a race condition. Seems like the test itself is not solid enough tough.

20:40:29 not ok 1604 parallel/test-repl-history-navigation
20:40:29   ---
20:40:29   duration_ms: 3.189
20:40:29   severity: fail
20:40:29   exitcode: 1
20:40:29   stack: |-
20:40:29     Failed test # 6
20:40:29     Last outputs: [
20:40:29       �[32m"'> '"�[39m,
20:40:29       �[32m"'€'"�[39m,
20:40:29       �[32m"' // WOW'"�[39m,
20:40:29       �[32m"'> '"�[39m,
20:40:29       �[32m"'s'"�[39m,
20:40:29       �[32m"' // Always visible'"�[39m,
20:40:29       �[32m"'> '"�[39m,
20:40:29       �[32m"'€'"�[39m,
20:40:29       �[32m"' // WOW'"�[39m
20:40:29     ]
20:40:29     /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-repl-history-navigation.js:357
20:40:29                 throw e;
20:40:29                 ^
20:40:29     
20:40:29     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
20:40:29     + actual - expected
20:40:29     
20:40:29     + ' // WOW'
20:40:29     - '> '
20:40:29         at Writable.write [as _write] (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-repl-history-navigation.js:351:20)
20:40:29         at doWrite (_stream_writable.js:453:12)
20:40:29         at writeOrBuffer (_stream_writable.js:435:5)
20:40:29         at Writable.write (_stream_writable.js:326:11)
20:40:29         at Timeout._onTimeout (internal/repl/utils.js:231:19)
20:40:29         at listOnTimeout (internal/timers.js:551:17)
20:40:29         at processTimers (internal/timers.js:492:7) {
20:40:29       generatedMessage: true,
20:40:29       code: 'ERR_ASSERTION',
20:40:29       actual: ' // WOW',
20:40:29       expected: '> ',
20:40:29       operator: 'strictEqual'
20:40:29     }
@BridgeAR BridgeAR added repl Issues and PRs related to the REPL subsystem. test Issues and PRs related to the tests. flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Dec 25, 2019
@BridgeAR BridgeAR self-assigned this Dec 25, 2019
@richardlau richardlau mentioned this issue Jan 8, 2020
Open
@addaleax
Copy link
Member

addaleax commented Feb 6, 2020

This seems easy to reproduce by changing common.platformTimeout(40) to common.platformTimeout(400) in the test.

Is there a chance we could remove the timeouts entirely? There shouldn’t be any in a REPL test, I think.

@BridgeAR
Copy link
Member Author

BridgeAR commented Feb 9, 2020

@addaleax we never specified if a custom completion function may be async or not. Right now async is accepted and if someone types fast the completion could be triggered multiple times. In that case only the very last triggered completion should be accepted. We could pause the stream during the completion instead of using a counter but it's not possible to keep on typing while the completion is running in that case.

@addaleax
Copy link
Member

addaleax commented Feb 9, 2020

@BridgeAR I assumed that completion functions may be async, but I don’t understand why that would need to be modelled though timers in the test?

@BridgeAR BridgeAR mentioned this issue Feb 9, 2020
Closed
4 tasks
codebytere pushed a commit that referenced this issue Feb 17, 2020
@BridgeAR @codebytere
test: fix flaky parallel/test-repl-history-navigation test
6dbe6bd 
Two scenarios should be tested:

1. The completion is triggered and the result is printed before the
   next invocation.
2. The completion is triggered multiple times right after each other
   without waiting for the result. In that case only the last result
   should be printed.

The first scenario did not need a timeout while the latter did not
need a timeout for the second invocation.

PR-URL: #31708
Fixes: #31094
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this issue Apr 25, 2020
@BridgeAR @targos
test: fix flaky parallel/test-repl-history-navigation test
3475eb8 
Two scenarios should be tested:

1. The completion is triggered and the result is printed before the
   next invocation.
2. The completion is triggered multiple times right after each other
   without waiting for the result. In that case only the last result
   should be printed.

The first scenario did not need a timeout while the latter did not
need a timeout for the second invocation.

PR-URL: nodejs#31708
Fixes: nodejs#31094
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Apr 28, 2020
@BridgeAR @targos
test: fix flaky parallel/test-repl-history-navigation test
8a044cb 
Two scenarios should be tested:

1. The completion is triggered and the result is printed before the
   next invocation.
2. The completion is triggered multiple times right after each other
   without waiting for the result. In that case only the last result
   should be printed.

The first scenario did not need a timeout while the latter did not
need a timeout for the second invocation.

PR-URL: #31708
Fixes: #31094
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@snyk-bot snyk-bot mentioned this issue Jun 1, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@BridgeAR BridgeAR

Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. 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 a pull request may close this issue.

test: fix flaky parallel/test-repl-history-navigation test BridgeAR/node
2 participants
@addaleax @BridgeAR