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

benchmark: improve --filter pattern matching #29987

Closed
wants to merge 1 commit into from
Closed

benchmark: improve --filter pattern matching #29987

wants to merge 1 commit into from

Conversation

mmarchini
Copy link
Contributor

@mmarchini mmarchini commented Oct 15, 2019 •
edited
Loading

  • Let users provide more than one pattern separated by :
  • Let users provide a exclude pattern prefixed with !
  • Add tests for --filter
  • Document --filter
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@mmarchini mmarchini added the benchmark Issues and PRs related to the benchmark subsystem. label Oct 15, 2019
@mscdex
Copy link
Contributor

mscdex commented Oct 15, 2019

I would prefer just allowing multiple --filter flags instead. That would match with how existing flags like --set work.

@mmarchini
Copy link
Contributor Author

@mscdex that makes sense, I'll update the PR. Any thoughts on using ! to exclude benchmarks matching the pattern?

@mmarchini
Copy link
Contributor Author

Ugh, I forgot ! doesn't work well inside double quotes (I was using single quotes while testing). Google Test uses -, let me see if I can make it work instead of !.

@mmarchini
Copy link
Contributor Author

PR updated to allow multiple --filter instead of using : as a separator and to use - instead of !. Also fixed a small bug on the argument parsing.

@Trott
Copy link
Member

Trott commented Oct 19, 2019

@nodejs/collaborators This could use some reviews.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26109/

@Trott
Copy link
Member

Trott commented Oct 19, 2019 •
edited
Loading

Benchmark CI: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/9579/ ✔️

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 19, 2019 •
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/26112/ ✔️

lundibundi
lundibundi reviewed Nov 12, 2019
View reviewed changes
test/parallel/test-benchmark-cli.js Outdated Show resolved Hide resolved
test/parallel/test-benchmark-cli.js Outdated Show resolved Hide resolved
benchmark/compare.js Show resolved Hide resolved
benchmark/_cli.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27525/

@mmarchini
Copy link
Contributor Author

@lundibundi done, thank you for your suggestions :)

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27527/

mcollina
mcollina approved these changes Dec 9, 2019
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

joyeecheung
joyeecheung reviewed Dec 9, 2019
View reviewed changes
benchmark/_cli.js Outdated Show resolved Hide resolved
@mmarchini
benchmark: improve --filter pattern matching
2641c3e 
  * Let users provide more than one pattern by repeating the flag
  * Add new flag --exclude to exclude patterns
  * Add tests for --filter
  * Document --filter

This commit also fixes a bug where things like
`compare.js --new --old binary --new binary` was acceptable (now the
script will exit and print the usage message).
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27530/

lundibundi
lundibundi approved these changes Dec 10, 2019
View reviewed changes
Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

LGTM

benchmark/_cli.js Show resolved Hide resolved
benchmark/_cli.js Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27553/

@lundibundi lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 10, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27556/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27559/

@mmarchini
Copy link
Contributor Author

Rebuilding ARM, although it seems to be broken everywhere (I saw the same errors in other PR builds): https://ci.nodejs.org/job/node-test-commit-arm/28083/

@mmarchini mmarchini mentioned this pull request Dec 10, 2019
Closed
@mmarchini
Copy link
Contributor Author

I opened #30890 to track the failures on ARM, since it doesn't seem related to this PR (the same failures are happening in other PRs). Landing.

@mmarchini
Copy link
Contributor Author

mmarchini commented Dec 10, 2019 •
edited
Loading

Landed in a4e7ea8

@mmarchini mmarchini closed this Dec 10, 2019
mmarchini added a commit that referenced this pull request Dec 10, 2019
@mmarchini
benchmark: improve --filter pattern matching
a4e7ea8 
  * Let users provide more than one pattern by repeating the flag
  * Add new flag --exclude to exclude patterns
  * Add tests for --filter
  * Document --filter

This commit also fixes a bug where things like
`compare.js --new --old binary --new binary` was acceptable (now the
script will exit and print the usage message).

PR-URL: #29987
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/27561/

targos pushed a commit that referenced this pull request Dec 11, 2019
@mmarchini @targos
benchmark: improve --filter pattern matching
78eaf50 
  * Let users provide more than one pattern by repeating the flag
  * Add new flag --exclude to exclude patterns
  * Add tests for --filter
  * Document --filter

This commit also fixes a bug where things like
`compare.js --new --old binary --new binary` was acceptable (now the
script will exit and print the usage message).

PR-URL: #29987
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
Merged
targos pushed a commit that referenced this pull request Jan 14, 2020
@mmarchini @targos
benchmark: improve --filter pattern matching
ac5de4f 
  * Let users provide more than one pattern by repeating the flag
  * Add new flag --exclude to exclude patterns
  * Add tests for --filter
  * Document --filter

This commit also fixes a bug where things like
`compare.js --new --old binary --new binary` was acceptable (now the
script will exit and print the usage message).

PR-URL: #29987
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@mmarchini @BethGriggs
benchmark: improve --filter pattern matching
667471e 
  * Let users provide more than one pattern by repeating the flag
  * Add new flag --exclude to exclude patterns
  * Add tests for --filter
  * Document --filter

This commit also fixes a bug where things like
`compare.js --new --old binary --new binary` was acceptable (now the
script will exit and print the usage message).

PR-URL: #29987
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
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

@mscdex mscdex mscdex left review comments

@lundibundi lundibundi lundibundi approved these changes

@Trott Trott Trott approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mmarchini @mscdex @Trott @nodejs-github-bot @mcollina @joyeecheung @lundibundi