Skip to content

benchmark: remove dependency on unshipped tools - #51146

Closed
AdamMajer wants to merge 1 commit into
nodejs:mainfrom
AdamMajer:cli-benchmark-fix
Closed

benchmark: remove dependency on unshipped tools#51146
AdamMajer wants to merge 1 commit into
nodejs:mainfrom
AdamMajer:cli-benchmark-fix

Conversation

@AdamMajer

@AdamMajerAdamMajer 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

@nodejs-github-botnodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Dec 13, 2023
@AdamMajer

AdamMajer commented Dec 13, 2023

Copy link
Copy Markdown
ContributorAuthor

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

@AdamMajerAdamMajer changed the title test: remove dependency on unshipped toolsbenchmark: remove dependency on unshipped toolsDec 13, 2023
@anonrig

anonrig commented Dec 14, 2023

Copy link
Copy Markdown
Member

cc @nodejs/build

joyeecheung
joyeecheung reviewed Dec 14, 2023
View reviewed changes
Comment threadbenchmark/misc/startup-cli-version.js Outdated
@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
@lpincalpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 25, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. 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/

@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/

@jasnell

jasnell commented Dec 25, 2023

Copy link
Copy Markdown
Member

Landed in 345f15e

@jasnelljasnell 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 <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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 <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Jan 2, 2024
Merged
@AdamMajerAdamMajer mentioned this pull request Feb 13, 2024
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 <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlaurichardlau mentioned this pull request Mar 25, 2024
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 <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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 <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
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 <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
eliphazbouye pushed a commit to eliphazbouye/node that referenced this pull request Jun 20, 2024
@AdamMajer@eliphazbouye
benchmark: filter non-present deps from start-cli-version
3910b40
PR-URL: nodejs#51746
Refs: nodejs#51146
Refs: nodejs#50684
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joyeecheungjoyeecheungjoyeecheung left review comments
@jasnelljasnelljasnell approved these changes
@lpincalpincalpinca approved these changes

Assignees

No one assigned

Labels

benchmarkIssues and PRs related to the benchmark subsystem.

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

6 participants

@AdamMajer@anonrig@nodejs-github-bot@jasnell@lpinca@joyeecheung