This repository has been archived by the owner on Apr 22, 2023. It is now read-only.
test: relax timing constraints for child process #25291
Closed
Conversation
This file contains 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
With additional load in the system, the child process which runs sleep command takes more time to run - typically slightly above 1 second, but above 2 seconds under stress. While the intent of the test is to test the functionality of spawnSync and the child process in general, in effect it is testing the system command sleep, and further, it's responsiveness. Since from the name the purpose of the test seems to be unrelated to the sleep behaviour, I believe a more meaningful assertion would be to see the time taken is more than 1 second.
|
Can you change the first line of the commit comment to be 50 chars or less |
|
I counted and got 48 actually, can you please check? |
|
my mistake, I guess I can't count.... |
|
lgtm |
mhdawson
pushed a commit
that referenced
this pull request
May 15, 2015
With additional load in the system, the child process which runs sleep command takes more time to run - typically slightly above 1 second, but above 2 seconds under stress. While the intent of the test is to test the functionality of spawnSync and the child process in general, in effect it is testing the system command sleep, and further, it's responsiveness. Since from the name the purpose of the test seems to be unrelated to the sleep behaviour, I believe a more meaningful assertion would be to see the time taken is more than 1 second. Reviewed-By: Michael Dawson <[email protected]> PR-URL: #25291
|
Landed as ebbb356 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In windows we were seeing failures in the Node test:
test-child-process-spawnsync.js
assert.js:86
throw new assert.AssertionError({
ˆ
AssertionError: sleep should not take longer or less than 1 second
at Object. (test\simple\test-child-process-spawnsync.js:44:8)
at Module._compile (module.js:460:26)
at Object.Module._extensions..js (module.js:478:10)
at Module.load (module.js:355:32)
at Function.Module._load (module.js:310:12)
at Function.Module.runMain (module.js:501:10)
at startup (node.js:129:16) # at node.js:814:3
This is easily reproducible by adding additional load to
the system while the test runs.
We see that the additional load can cause the sleep
to wakeup after 2 seconds or more instead of the usual/expected
1 second.
While the intent of the test is to test
the functionality of spawnSync and the child process
in general, in effect it is testing the system command sleep,
and further, it's responsiveness.
Since from the name the purpose of the test seems to be
unrelated to the sleep behavior, I believe a more meaningful
assertion would be to see the time taken is more than 1 second.
And hence the pull request.