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 — benchmark: remove dependency on unshipped tools by AdamMajer · Pull Request #51146 · nodejs/node

Conversation

@AdamMajer

@AdamMajer AdamMajer commented Dec 13, 2023

Copy link
Copy Markdown
Contributor

tools/node_modules is removed from the tarball so it should not be used as part of unit tests or benchmarks.

Fixes: #51145

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 benchmark Issues and PRs related to Node.js benchmarks and benchmarking infrastructure. label Dec 13, 2023
@AdamMajer

AdamMajer commented Dec 13, 2023

Copy link
Copy Markdown
Contributor Author

An alternative to using npx is to reduce the number of executables run here to 2
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AdamMajer
AdamMajer force-pushed the cli-benchmark-fix branch from 800a804 to 493be23 Compare December 13, 2023 13:49
@AdamMajer AdamMajer changed the title test: remove dependency on unshipped tools benchmark: remove dependency on unshipped tools Dec 13, 2023
lpinca
lpinca approved these changes Dec 13, 2023
View reviewed changes
@anonrig

anonrig commented Dec 14, 2023

Copy link
Copy Markdown
Member

cc @nodejs/build
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joyeecheung
joyeecheung reviewed Dec 14, 2023
View reviewed changes
Comment thread benchmark/misc/startup-cli-version.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AdamMajer
benchmark: remove dependency on unshipped tools
35403d7 
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: nodejs#51145
Refs: nodejs#50684
@AdamMajer
AdamMajer force-pushed the cli-benchmark-fix branch from 493be23 to 35403d7 Compare December 15, 2023 14:04
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 25, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Dec 25, 2023
@nodejs-github-bot

nodejs-github-bot commented Dec 25, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56523/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Dec 25, 2023

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56528/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell approved these changes Dec 25, 2023
View reviewed changes
@jasnell

jasnell commented Dec 25, 2023

Copy link
Copy Markdown
Member

Landed in 345f15e
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Dec 25, 2023
jasnell pushed a commit that referenced this pull request Dec 25, 2023
@AdamMajer @jasnell
benchmark: remove dependency on unshipped tools
345f15e 
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: #51145
Refs: #50684
PR-URL: #51146
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
@AdamMajer @RafaelGSS
benchmark: remove dependency on unshipped tools
4dcc511 
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: #51145
Refs: #50684
PR-URL: #51146
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
v21.6.0 proposal #51342
Merged
@AdamMajer AdamMajer mentioned this pull request Feb 13, 2024
benchmark: move non-present deps down the list #51746
Merged
AdamMajer added a commit to AdamMajer/node that referenced this pull request Feb 14, 2024
@AdamMajer
benchmark: move non-present deps down the list
91bba09 
In previous version of this fix, I've simply added a check if the tested
tool is available or not. Unfortuntelly, this fails when only the first
tool is to be run as part of the test-benchmark-misc, and it doesn't
exist.

benchmark/test-benchmark-misc
...
AssertionError [ERR_ASSERTION]: benchmark file not running exactly one
configuration in test:
...
misc/startup-cli-version.js
...

One solution is to check if the cli tool is actually available before
using it in a benchmark

Refs: nodejs#51146
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@AdamMajer @richardlau
benchmark: remove dependency on unshipped tools
4be33b5 
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: #51145
Refs: #50684
PR-URL: #51146
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
v20.12.0 proposal #52212
Merged
aduh95 pushed a commit that referenced this pull request May 11, 2024
@AdamMajer
benchmark: filter non-present deps from start-cli-version
b1515c7 
PR-URL: #51746
Refs: #51146
Refs: #50684
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
targos pushed a commit that referenced this pull request May 11, 2024
@AdamMajer @targos
benchmark: filter non-present deps from start-cli-version
2c7d7ca 
PR-URL: #51746
Refs: #51146
Refs: #50684
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Jun 17, 2024
@AdamMajer @marco-ippolito
benchmark: filter non-present deps from start-cli-version
184cfe5 
PR-URL: #51746
Refs: #51146
Refs: #50684
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
soophoo pushed a commit to soophoo/node that referenced this pull request Jun 20, 2024
@AdamMajer @soophoo
benchmark: filter non-present deps from start-cli-version
3910b40 
PR-URL: nodejs#51746
Refs: nodejs#51146
Refs: nodejs#50684
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheung joyeecheung joyeecheung left review comments
@jasnell jasnell jasnell approved these changes
@lpinca lpinca lpinca approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

benchmark Issues and PRs related to Node.js benchmarks and benchmarking infrastructure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

tests: tools/node_modules is not present in tarball

Uh oh!

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

6 participants

@AdamMajer @anonrig @nodejs-github-bot @jasnell @lpinca @joyeecheung
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.