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: revise test-policy-integrity #35101

Merged
merged 1 commit into from
Sep 10, 2020
Merged

test: revise test-policy-integrity #35101

merged 1 commit into from
Sep 10, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 8, 2020

  • eliminate unneeded Set deletion/cleanup
  • use number of CPUs as limit for processes spawned rather than
    hard-coding the limit
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 8, 2020
@Trott
Copy link
Member Author

Trott commented Sep 8, 2020

This greatly speeds the test in some environments. Notably, it means that the test no longer times out most of the time on FreeBSD in CI.

Stress test with master showing it failing due to timing out 9 out of 10 times:
https://ci.nodejs.org/job/node-stress-single-test/nodes=freebsd11-x64/171/

Stress test with this PR showing it succeeding every time:
https://ci.nodejs.org/job/node-stress-single-test/nodes=freebsd11-x64/172

On master, the one successful time in CI, it took 216 seconds. Every other time, it timed out after 240 seconds.

With this PR, it takes about 93 seconds to run each time.

@Trott Trott requested a review from bmeck September 8, 2020 13:05
@Trott
Copy link
Member Author

Trott commented Sep 8, 2020

@nodejs/testing

@Trott
Copy link
Member Author

Trott commented Sep 8, 2020

@Trott
Copy link
Member Author

Trott commented Sep 9, 2020

@nodejs/collaborators This could use reviews.

@Trott
Copy link
Member Author

Trott commented Sep 9, 2020

@bmeck I don't think these changes invalidate the test or anything, but I would feel a lot more confident about that if you had a chance to take a look and agreed.

@bmeck
Copy link
Member

bmeck commented Sep 10, 2020

So I actually went about deleting due to memory and pushing the limit up to try and speed things up as I was originally trying to get ARM to be happy, these seem fine.

@puzpuzpuz puzpuzpuz self-requested a review September 10, 2020 06:31
@Trott
Copy link
Member Author

Trott commented Sep 10, 2020

Landed in 7fc1a4a

@Trott Trott merged commit 7fc1a4a into nodejs:master Sep 10, 2020
@Trott Trott deleted the revise-policy branch September 10, 2020 13:08
* eliminate unneeded Set deletion/cleanup
* use number of CPUs as limit for processes spawned rather than
  hard-coding the limit

PR-URL: nodejs#35101
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2020
* eliminate unneeded Set deletion/cleanup
* use number of CPUs as limit for processes spawned rather than
  hard-coding the limit

PR-URL: #35101
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Sep 21, 2020
4 tasks
addaleax pushed a commit that referenced this pull request Sep 22, 2020
* eliminate unneeded Set deletion/cleanup
* use number of CPUs as limit for processes spawned rather than
  hard-coding the limit

PR-URL: #35101
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
@codebytere codebytere mentioned this pull request Sep 28, 2020
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
* eliminate unneeded Set deletion/cleanup
* use number of CPUs as limit for processes spawned rather than
  hard-coding the limit

PR-URL: nodejs#35101
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants