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

test: reduce run time for string_decoder benchmark #16118

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 10, 2017

test-benchmark-string_decoder was timing out in CI. Reduce the run time
by sending appropriate options such that each benchmark file only runs
one combination of options.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test benchmark string_decoder

test-benchmark-string_decoder was timing out in CI. Reduce the run time
by sending appropriate options such that each benchmark file only runs
one combination of options.
@Trott Trott 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. string_decoder Issues and PRs related to the string_decoder subsystem. test Issues and PRs related to the tests. labels Oct 10, 2017
@Trott
Copy link
Member Author

Trott commented Oct 10, 2017

Pinging @nodejs/testing for reviews. Would like to expedite landing so things like this from https://ci.nodejs.org/job/node-test-commit-smartos/12035/nodes=smartos15-64/console stop happening:

not ok 206 parallel/test-benchmark-string_decoder
  ---
  duration_ms: 120.743
  severity: fail
  stack: |-
    timeout

@Trott
Copy link
Member Author

Trott commented Oct 10, 2017

@Trott
Copy link
Member Author

Trott commented Oct 10, 2017

@refack
Copy link
Contributor

refack commented Oct 10, 2017

For reference, new CI:
https://ci.nodejs.org/job/node-test-commit-smartos/12037/nodes=smartos15-64/console

ok 148 parallel/test-benchmark-string_decoder
  ---
  duration_ms: 5.28
  ...

@BridgeAR
Copy link
Member

@Trott do we in general really want to have the benchmark coverage? I know we had a few ones that had issues but in general this is slowing down the CI and local testing a lot.

@Trott
Copy link
Member Author

Trott commented Oct 10, 2017

@Trott do we in general really want to have the benchmark coverage? I know we had a few ones that had issues but in general this is slowing down the CI and local testing a lot.

@BridgeAR I'm in favor of it, but it's our collective decision, so if lots of others feel differently, then we can put them in pummel or something like that.

@Trott
Copy link
Member Author

Trott commented Oct 10, 2017

By the way, this test (in parallel) now takes 5 seconds when it was timing out more than 2 minutes, so this fix will go a long way towards reducing the time these benchmark tests are taking. :-D

@Trott
Copy link
Member Author

Trott commented Oct 10, 2017

Change seems trivial, easy to revert, low risk, and high value in the "turn CI not-red" department, so I'm going to land now....

Trott added a commit to Trott/io.js that referenced this pull request Oct 10, 2017
test-benchmark-string_decoder was timing out in CI. Reduce the run time
by sending appropriate options such that each benchmark file only runs
one combination of options.

PR-URL: nodejs#16118
Reviewed-By: Refael Ackermann <refack@gmail.com>
@Trott
Copy link
Member Author

Trott commented Oct 10, 2017

Landed in fa265a2

@Trott Trott closed this Oct 10, 2017
addaleax pushed a commit to addaleax/ayo that referenced this pull request Oct 12, 2017
test-benchmark-string_decoder was timing out in CI. Reduce the run time
by sending appropriate options such that each benchmark file only runs
one combination of options.

PR-URL: nodejs/node#16118
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos pushed a commit that referenced this pull request Oct 18, 2017
test-benchmark-string_decoder was timing out in CI. Reduce the run time
by sending appropriate options such that each benchmark file only runs
one combination of options.

PR-URL: #16118
Reviewed-By: Refael Ackermann <refack@gmail.com>
@Trott Trott deleted the fix-benchmark-string-decoder branch January 13, 2022 22:46
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. flaky-test Issues and PRs related to the tests with unstable failures on the CI. string_decoder Issues and PRs related to the string_decoder subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants