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.

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

test: reduce test-hash-seed run time by half #25522

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 15, 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.

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), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Replace min() function with Math.min(...).
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 15, 2019
@Trott Trott added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Jan 15, 2019
@Trott Trott mentioned this pull request Jan 15, 2019
2 tasks
@Trott
Copy link
Member Author

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 )

@Trott
Copy link
Member Author

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/

@ofrobots
Copy link
Contributor

@Trott

(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?

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.

test/pummel/test-hash-seed.js Show resolved Hide resolved
@Trott Trott force-pushed the refactor-hash-seed branch 2 times, most recently from 9eedc23 to 0834196 Compare January 15, 2019 23:49
test/pummel/test-hash-seed.js Outdated Show resolved Hide resolved
@refack
Copy link
Contributor

refack commented Jan 16, 2019

By the way, does anyone know if this test is even valid anymore?

IIUC this could be tested with node8 < v8.1.4

reduce test-hash-seed run time by half

[nit] Only on multi-core/hyper-threaded machines. For example our Windows 10 CI machines are single core.

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.
@Trott
Copy link
Member Author

Trott commented Jan 16, 2019

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 16, 2019
@refack
Copy link
Contributor

refack commented Jan 16, 2019

Test still works (after a bit of massage to common/index.js)

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 === 2
    at 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

@Trott
Copy link
Member Author

Trott commented Jan 18, 2019

Landed in 5fdd554...f2e3a4e

@Trott Trott closed this Jan 18, 2019
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 <ofrobots@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
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 <ofrobots@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
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 <ofrobots@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
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 <ofrobots@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
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 <ofrobots@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
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 <ofrobots@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
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 <ofrobots@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
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 <ofrobots@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
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 <ofrobots@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
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 <ofrobots@google.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
@Trott Trott deleted the refactor-hash-seed branch January 13, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. flaky-test Issues and PRs related to the tests with unstable failures on the CI. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants