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: simplify http benchmarker regular expression #38206

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

Trott
Copy link
Member

@Trott Trott commented Apr 11, 2021

A non-capturing group inside a capturing group has no effect. Simplify
the regular expression.

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. http Issues or PRs related to the http subsystem. labels Apr 11, 2021
@nodejs-github-bot
Copy link
Collaborator

@@ -173,7 +173,7 @@ class H2LoadBenchmarker {
}

processResults(output) {
const rex = /(\d+(?:\.\d+)) req\/s/;
const rex = /(\d+\.\d+) req\/s/;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the original intent was to handle integer values? I don't know if that's necessary, please disregard if we know that doesn't happen.

Suggested change
const rex = /(\d+\.\d+) req\/s/;
const rex = /(\d+(?:\.\d+)?) req\/s/;

Copy link
Member

@ErickWendel ErickWendel Jan 10, 2022

Choose a reason for hiding this comment

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

I think it shouldn't work with integers. The h2load output is something like "finished in 233.18s, 0.43 req/s, 105B/s" his docs

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 12, 2022
A non-capturing group inside a capturing group has no effect. Simplify
the regular expression.

PR-URL: nodejs#38206
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@Trott
Copy link
Member Author

Trott commented Jan 12, 2022

Landed in 36035e0

@Trott Trott merged commit 36035e0 into nodejs:master Jan 12, 2022
@Trott Trott deleted the non-capture branch January 12, 2022 06:58
targos pushed a commit that referenced this pull request Jan 14, 2022
A non-capturing group inside a capturing group has no effect. Simplify
the regular expression.

PR-URL: #38206
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
thedull pushed a commit to thedull/node that referenced this pull request Jan 18, 2022
A non-capturing group inside a capturing group has no effect. Simplify
the regular expression.

PR-URL: nodejs#38206
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
A non-capturing group inside a capturing group has no effect. Simplify
the regular expression.

PR-URL: nodejs#38206
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
A non-capturing group inside a capturing group has no effect. Simplify
the regular expression.

PR-URL: #38206
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ricky Zhou <0x19951125@gmail.com>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
@panva panva removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants