readline: use Date.now() and move test to parallel - #18563
Conversation
Sorry, something went wrong.
|
@maclover7 I'm aware. I'm like 99.999% positive that change wasn't strictly correct. Timer.now() updates internal uv time so it shouldn't fix the bug on its own. Indeed, running those tests in parallel proves that the bug exists with both Timer.now() and Date.now(). Hence the actual fix to those tests in this PR. (Also, strictly speaking Timer.now() could return 0 early in the process' life so it's not really ideal for using in readline which wants a truthy time.) |
Sorry, something went wrong.
There was a problem hiding this comment.
On second thought, now is a costant, so isn't this always 0?
Sorry, something went wrong.
There was a problem hiding this comment.
It's constant within the scope of the function but the case it's accounting for is when _normalWrite is called twice in a row, once with \n and then with \r.
Sorry, something went wrong.
There was a problem hiding this comment.
Gotcha.
Sorry, something went wrong.
There was a problem hiding this comment.
That said, this did make me rethink caching the value like I did. It doesn't make any sense and it's an unnecessary call in many situations. I'll fix that up.
Sorry, something went wrong.
There was a problem hiding this comment.
Also it's definitely easier to grok now.
Sorry, something went wrong.
The readline module wants a truthy time while using Timer.now() doesn't necessarily guarantee that early on in the process' life. It also doesn't actually resolve the timing issues experienced in an earlier issue. Instead, this PR fixes the related tests and moves them back to parallel. Refs: nodejs#14674
|
Ok, this is no longer WIP as the stress test CI is clean: https://ci.nodejs.org/job/node-stress-single-test/1774/nodes=osx1010/console |
Sorry, something went wrong.
The readline module wants a truthy time while using Timer.now() doesn't necessarily guarantee that early on in the process' life. It also doesn't actually resolve the timing issues experienced in an earlier issue. Instead, this PR fixes the related tests and moves them back to parallel. Refs: nodejs#14674 PR-URL: nodejs#18563 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
The readline module wants a truthy time while using Timer.now() doesn't necessarily guarantee that early on in the process' life. It also doesn't actually resolve the timing issues experienced in an earlier issue. Instead, this PR fixes the related tests and moves them back to parallel. Refs: #14674 PR-URL: #18563 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
The readline module wants a truthy time while using Timer.now() doesn't necessarily guarantee that early on in the process' life. It also doesn't actually resolve the timing issues experienced in an earlier issue. Instead, this PR fixes the related tests and moves them back to parallel. Refs: #14674 PR-URL: #18563 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
The readline module wants a truthy time while using Timer.now() doesn't necessarily guarantee that early on in the process' life. It also doesn't actually resolve the timing issues experienced in an earlier issue. Instead, this PR fixes the related tests and moves them back to parallel. Refs: #14674 PR-URL: #18563 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
The readline module wants a truthy time while using Timer.now() doesn't necessarily guarantee that early on in the process' life. It also doesn't actually resolve the timing issues experienced in an earlier issue. Instead, this PR fixes the related tests and moves them back to parallel. Refs: #14674 PR-URL: #18563 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
The readline module wants a truthy time while using Timer.now() doesn't necessarily guarantee that early on in the process' life. It also doesn't actually resolve the timing issues experienced in an earlier issue. Instead, this PR fixes the related tests and moves them back to parallel. Refs: nodejs#14674 PR-URL: nodejs#18563 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
The readline module wants a truthy time while using Timer.now() doesn't necessarily guarantee that early on in the process' life. It also doesn't actually resolve the timing issues experienced in #14681. Instead, this PR fixes the related tests and moves them back to parallel.
This needs a stress test & CI (hence the WIP), but I can't seem to run one from a different repository anymore as used to be possible...
CI:
https://ci.nodejs.org/job/node-test-pull-request/12927/ (green)
https://ci.nodejs.org/job/node-test-pull-request/12930/ (green)
https://ci.nodejs.org/job/node-test-pull-request/12931/ (green)
https://ci.nodejs.org/job/node-test-pull-request/12932/
https://ci.nodejs.org/job/node-test-pull-request/12933/
Stress test CI:
https://ci.nodejs.org/job/node-stress-single-test/1774/ (OS X)
Checklist
Affected core subsystem(s)
readline