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

test: add arg to narrow http benchmark test #26101

Merged
merged 1 commit into from
Feb 15, 2019
Merged

test: add arg to narrow http benchmark test #26101

merged 1 commit into from
Feb 15, 2019

Conversation

refack
Copy link
Contributor

@refack refack commented Feb 14, 2019 •
edited
Loading

Recent changes (da0dc51) added a new benchmark (benchmark/http/incoming_headers.js) which takes a new arg (headerDuplicates).
This PR passes a value for that arg so that during the sanity test this benchmark will run only once.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2603/pipeline

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Feb 14, 2019
@refack
Copy link
Contributor Author

refack commented Feb 14, 2019

Special CI job: https://ci.nodejs.org/job/node-test-commit-custom-suites/872/

@refack refack added benchmark Issues and PRs related to the benchmark subsystem. flaky-test Issues and PRs related to the tests with unstable failures on the CI. labels Feb 14, 2019
@refack
Copy link
Contributor Author

refack commented Feb 14, 2019

An alternative could be to rename the new arg to n ¯\(ツ)/¯

@refack refack added the fast-track PRs that do not need to wait for 48 hours to land. label Feb 14, 2019
@refack
Copy link
Contributor Author

refack commented Feb 14, 2019

Please 👍 to fast-track since ATM the daily master job is failing

starkwang
starkwang approved these changes Feb 14, 2019
View reviewed changes
Copy link
Contributor

@starkwang starkwang left a comment

Choose a reason for hiding this comment

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

LGTM

@richardlau richardlau mentioned this pull request Feb 14, 2019
Closed
2 tasks
Trott
Trott reviewed Feb 14, 2019
View reviewed changes
test/benchmark/test-benchmark-http.js
@@ -29,7 +29,8 @@ runBenchmark('http',
'n=1',
'res=normal',
'type=asc',
'value=X-Powered-By'
'value=X-Powered-By',
'headerDuplicates=1',
Copy link
Member

Choose a reason for hiding this comment

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

Can you relocate it higher in the list to keep the list in alphabetical order?

Copy link
Member

Choose a reason for hiding this comment

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

Can you relocate it higher in the list to keep the list in alphabetical order?

Argh, never mind, it's already not alphabetical, just mostly alphabetical. Anyway, LGTM, but nit: Would prefer to alphabetize the list.

(Also, agree that n is probably better. The headers aren't duplicates. They're similarly-named.)

@refack
test: add arg to narrow http benchmark test
fe58bef 
PR-URL: nodejs#26101
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@refack refack force-pushed the only-one-bench-http-incoming_headers branch from 182ec22 to fe58bef Compare February 15, 2019 00:10
@refack refack merged commit fe58bef into nodejs:master Feb 15, 2019
@refack
Copy link
Contributor Author

refack commented Feb 15, 2019 •
edited
Loading

Fast-tracked with 3 upvotes
image

@Trott I'll make a follow up clean PR.

@refack refack deleted the only-one-bench-http-incoming_headers branch February 15, 2019 00:14
@refack refack mentioned this pull request Feb 15, 2019
Merged
4 tasks
@refack
Copy link
Contributor Author

refack commented Feb 15, 2019

Refactoring PR #26119

targos pushed a commit that referenced this pull request Feb 15, 2019
@refack @targos
test: add arg to narrow http benchmark test
b0220f8 
PR-URL: #26101
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
refack added a commit to refack/node that referenced this pull request Feb 18, 2019
@refack
benchmark,test: refactoring
c7b6a22 
PR-URL: nodejs#26119
Refs: nodejs#26101
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Feb 19, 2019
@refack @addaleax
benchmark,test: refactoring
b50651f 
PR-URL: #26119
Refs: #26101
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Feb 26, 2019
Merged
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@refack @rvagg
test: add arg to narrow http benchmark test
dd60cd6 
PR-URL: #26101
Reviewed-By: Weijia Wang <starkwang@126.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@refack @rvagg
benchmark,test: refactoring
f4955fd 
PR-URL: #26119
Refs: #26101
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@starkwang starkwang starkwang approved these changes

Assignees
No one assigned
Labels
benchmark Issues and PRs related to the benchmark subsystem. fast-track PRs that do not need to wait for 48 hours to land. flaky-test Issues and PRs related to the tests with unstable failures on the CI. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@refack @nodejs-github-bot @Trott @addaleax @cjihrig @starkwang