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

benchmark: add benchmarks for the test_runner #48931

Merged
merged 9 commits into from
Jul 29, 2023

Conversation

rluvaton
Copy link
Member

@rluvaton rluvaton commented Jul 26, 2023

I'm trying to benchmark tests creation and execution

this is needed as we need to evaluate how a change in the test runner affects the performance:

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. test_runner labels Jul 26, 2023
});
}

await Promise.all(promises);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use finished on the reporter or this?

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mostly didn't understand what we are benchmarking. Can you leave some descriptive comments on what we are benchmarking? PS: The file names does not help either. (For example: plain-tests-as-fast-as-can)

@anonrig
Copy link
Member

anonrig commented Jul 26, 2023

cc @nodejs/performance @MoLow @cjihrig

@MoLow
Copy link
Member

MoLow commented Jul 26, 2023

I mostly didn't understand what we are benchmarking. Can you leave some descriptive comments on what we are benchmarking? PS: The file names does not help either. (For example: plain-tests-as-fast-as-can)

we want to compare how changes in test runner code affect its performance, but I agree naming can be improved

@rluvaton in regarding to #47945 (comment) - @cjihrig and myself were talking about what we want to benchmark - we probably also want to run tests with --test (probably one case should run a single file with --test and another should run multiple files (perhaps all of the test-runner fixture folder))
can you add those benchmarks to this pr? if not, Il do it in a follow-up PR

@rluvaton
Copy link
Member Author

[...] and another should run multiple files (perhaps all of the test-runner fixture folder)

I don't think running the test-runner fixture folder is a good idea as it changes, while I think benchmarks are best when they are static

  • we probably also want to run tests with --test

I avoided that on purpose to reduce the scope of the benchmark, adding the --test as well will also benchmark the process and read files and more

@MoLow
Copy link
Member

MoLow commented Jul 26, 2023

[...] and another should run multiple files (perhaps all of the test-runner fixture folder)

I don't think running the test-runner fixture folder is a good idea as it changes, while I think benchmarks are best when they are static

it is ok since the benchmarks are compared before and after a change we want to understand the impact of. no baseline or benchmark is saved according to @cjihrig

  • we probably also want to run tests with --test

I avoided that on purpose to reduce the scope of the benchmark, adding the --test as well will also benchmark the process and read files and more

that is a good point, but I think that should only mean we should rely on this specific benchmark only if it is significantly different, not that we shouldn't test it at all

@rluvaton
Copy link
Member Author

can you add those benchmarks to this pr? if not, Il do it in a follow-up PR

I prefer if you add those as you also need to add only and also I faced some problems with adding the --test flag, if it's really important to you I can put some time to fix those...

Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@debadree25 debadree25 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 29, 2023
@debadree25 debadree25 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 29, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 29, 2023
@nodejs-github-bot nodejs-github-bot merged commit f458e5b into nodejs:main Jul 29, 2023
27 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in f458e5b

@rluvaton rluvaton deleted the add-test-runner-benchmarks branch July 29, 2023 18:09
pluris pushed a commit to pluris/node that referenced this pull request Aug 6, 2023
PR-URL: nodejs#48931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
pluris pushed a commit to pluris/node that referenced this pull request Aug 7, 2023
PR-URL: nodejs#48931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
PR-URL: #48931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Aug 15, 2023
PR-URL: nodejs#48931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
PR-URL: #48931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
targos pushed a commit that referenced this pull request Nov 27, 2023
PR-URL: #48931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#48931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#48931
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. test_runner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants