GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — test: revise test-policy-integrity by Trott · Pull Request #35101 · nodejs/node

Conversation

@Trott

@Trott Trott commented Sep 8, 2020

Copy link
Copy Markdown
Member
  • 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to Node.js core tests and test infrastructure. label Sep 8, 2020
@Trott

Trott commented Sep 8, 2020

Copy link
Copy Markdown
Member Author

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.
aduh95 and puzpuzpuz reacted with rocket emoji
All reactions
  • 🚀 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott
Trott requested a review from bmeck September 8, 2020 13:05
@Trott

Trott commented Sep 8, 2020

Copy link
Copy Markdown
Member Author

@nodejs/testing
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Sep 8, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Sep 9, 2020

Copy link
Copy Markdown
Member Author

@nodejs/collaborators This could use reviews.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Sep 9, 2020

Copy link
Copy Markdown
Member Author

@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bmeck

bmeck commented Sep 10, 2020

Copy link
Copy Markdown
Member

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.
Trott reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bmeck
bmeck approved these changes Sep 10, 2020
View reviewed changes
@puzpuzpuz
puzpuzpuz self-requested a review September 10, 2020 06:31
puzpuzpuz
puzpuzpuz approved these changes Sep 10, 2020
View reviewed changes
@Trott
Trott force-pushed the revise-policy branch from a584db1 to 7fc1a4a Compare September 10, 2020 13:08
@Trott

Trott commented Sep 10, 2020

Copy link
Copy Markdown
Member Author

Landed in 7fc1a4a
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
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 <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Sep 21, 2020
V14.12.0 proposal #35285
Merged
4 tasks done
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 <[email protected]>
Reviewed-By: Andrey Pechkurov <[email protected]>
@codebytere codebytere mentioned this pull request Sep 28, 2020
v12.19.0 proposal #35401
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 <[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

Reviewers

2 more reviewers
@bmeck bmeck bmeck approved these changes
@puzpuzpuz puzpuzpuz puzpuzpuz approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

test Issues and PRs related to Node.js core tests and test infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@Trott @bmeck @puzpuzpuz @nodejs-github-bot
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.