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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

Pummel CI: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/16330/ ✅

@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
@Trott
test: revise test-policy-integrity
7fc1a4a 
* 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 <bradley.meck@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2020
@Trott @ruyadorno
test: revise test-policy-integrity
379c5ce 
* 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 <bradley.meck@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Sep 21, 2020
Merged
4 tasks
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@Trott @addaleax
test: revise test-policy-integrity
43c0174 
* 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 <bradley.meck@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@Trott @joesepi
test: revise test-policy-integrity
e859c80 
* 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 <bradley.meck@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@puzpuzpuz puzpuzpuz puzpuzpuz approved these changes

@bmeck bmeck bmeck approved these changes

Assignees
No one assigned
Labels
test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Trott @bmeck @puzpuzpuz @nodejs-github-bot