test: reduce test-hash-seed run time by half#25522
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or 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
Replace min() function with Math.min(...).
Trott added the flaky-test 
MemberAuthor
Trott commented Jan 15, 2019
(By the way, does anyone know if this test is even valid anymore? I imagine it is, but I don't know where the hash-seed is calculated. It looks like the fixture maybe copies some code from someplace else and therefore might theoretically be out of date? @ofrobots @nodejs/v8 ) |
Uh oh!
There was an error while loading. Please reload this page.
MemberAuthor
Trott commented Jan 15, 2019
Lite CI should be sufficient because pummel tests aren't yet run in CI. Here's a custom run to exercise just this test to confirm that it won't time out anymore (or at least won't always time out) in CI: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/3908/ |
Contributor
ofrobots commented Jan 15, 2019
Yes, the test is still relevant. This ensures that we are building V8 in a way to make sure the V8's internal hash seed is not predictable. See CVE-2017-11499. This is not an important attack vector for browsers, so this kind of stuff is likely going to be configuration flag for V8 going forward. The test ensures that we are setting the flag correct and V8 itself is not regressing. |
ofrobots approved these changes Jan 15, 2019
lpinca approved these changes Jan 15, 2019
Uh oh!
There was an error while loading. Please reload this page.
Trottforce-pushed the refactor-hash-seed branch 2 times, most recently from 9eedc23 to 0834196Compare
refack approved these changes Jan 16, 2019
Uh oh!
There was an error while loading. Please reload this page.
Contributor
refack commented Jan 16, 2019
IIUC this could be tested with node8 < v8.1.4
[nit] Only on multi-core/hyper-threaded machines. For example our Windows 10 CI machines are single core. |
Trottforce-pushed the refactor-hash-seed branch from 0834196 to c1260cfCompareReduce the time it takes to run test/pummel/test-hash-seed by switching from spawnSync() to spawn(). On my computer, this reduces the runtime from about 80 seconds to about 40 seconds. This test is not (yet) run regularly on CI, but when it was run recently, it timed out.
Trottforce-pushed the refactor-hash-seed branch from c1260cf to 6f00801CompareMemberAuthor
Trott commented Jan 16, 2019
Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2260/ Running the test on CI (scheduled): https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/3915/ |
Trott added the author ready 
richardlau approved these changes Jan 16, 2019
Contributor
refack commented Jan 16, 2019
Test still works (after a bit of massage to DEV D:\code\prws>node8.1.3 test\pummel\test-hash-seed.js Seeds: 60421████,60421████ assert.js:60 throw new errors.AssertionError({^ AssertionError [ERR_ASSERTION]: 1=== 2at Countdown.requiredCallback (D:\code\prws\test\pummel\test-hash-seed.js:15:10) at Countdown.<anonymous> (D:\code\prws\test\common\index.js:378:15) at Countdown.dec (D:\code\prws\test\common\countdown.js:21:22) at ChildProcess.subprocess.on (D:\code\prws\test\pummel\test-hash-seed.js:29:15) at emitTwo (events.js:125:13) at ChildProcess.emit (events.js:213:7) at Process.ChildProcess._handle.onexit (internal/child_process.js:197:12) DEV D:\code\prws>node8.1.4 test\pummel\test-hash-seed.js Seeds: 802238064,671715787 |
JungMinu approved these changes Jan 16, 2019
MemberAuthor
Trott commented Jan 18, 2019
Landed in 5fdd554...f2e3a4e |
Trott added a commit to Trott/io.js that referenced this pull request Jan 18, 2019
Replace min() function with Math.min(...). PR-URL: nodejs#25522 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Jan 18, 2019
Reduce the time it takes to run test/pummel/test-hash-seed by switching from spawnSync() to spawn(). On my computer, this reduces the runtime from about 80 seconds to about 40 seconds. This test is not (yet) run regularly on CI, but when it was run recently, it timed out. PR-URL: nodejs#25522 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 23, 2019
Replace min() function with Math.min(...). PR-URL: #25522 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
addaleax pushed a commit that referenced this pull request Jan 23, 2019
Reduce the time it takes to run test/pummel/test-hash-seed by switching from spawnSync() to spawn(). On my computer, this reduces the runtime from about 80 seconds to about 40 seconds. This test is not (yet) run regularly on CI, but when it was run recently, it timed out. PR-URL: #25522 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
Merged
BethGriggs pushed a commit that referenced this pull request Apr 29, 2019
Replace min() function with Math.min(...). PR-URL: #25522 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 29, 2019
Reduce the time it takes to run test/pummel/test-hash-seed by switching from spawnSync() to spawn(). On my computer, this reduces the runtime from about 80 seconds to about 40 seconds. This test is not (yet) run regularly on CI, but when it was run recently, it timed out. PR-URL: #25522 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
Replace min() function with Math.min(...). PR-URL: #25522 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
BethGriggs pushed a commit that referenced this pull request May 10, 2019
Reduce the time it takes to run test/pummel/test-hash-seed by switching from spawnSync() to spawn(). On my computer, this reduces the runtime from about 80 seconds to about 40 seconds. This test is not (yet) run regularly on CI, but when it was run recently, it timed out. PR-URL: #25522 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
Replace min() function with Math.min(...). PR-URL: #25522 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
Reduce the time it takes to run test/pummel/test-hash-seed by switching from spawnSync() to spawn(). On my computer, this reduces the runtime from about 80 seconds to about 40 seconds. This test is not (yet) run regularly on CI, but when it was run recently, it timed out. PR-URL: #25522 Reviewed-By: Ali Ijaz Sheikh <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Minwoo Jung <[email protected]>
This was referenced May 29, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
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.
Reduce the time it takes to run test/pummel/test-hash-seed by switching
from spawnSync() to spawn(). On my computer, this reduces the runtime
from about 80 seconds to about 40 seconds. This test is not (yet) run
regularly on CI, but when it was run recently, it timed out.
While we're in there, replace min() function with Math.min(...). It's in a separate commit because it's unrelated, but I couldn't bring myself to not do it at all.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes