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: add default type in getstringwidth.js #31377

Merged
merged 0 commits into from
Jan 16, 2020
Merged

benchmark: add default type in getstringwidth.js #31377

merged 0 commits into from
Jan 16, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Jan 16, 2020

This fixes a benchmark test that was recently broken by a breaking
change on the master branch.

Fixes: #31372

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

@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Jan 16, 2020
@Trott Trott mentioned this pull request Jan 16, 2020
Closed
@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 16, 2020
@Trott
Copy link
Member Author

Trott commented Jan 16, 2020

I'd like to fast-track this to unbreak nightly CI. Please 👍 here to approve (or comment to block).

@Trott
Copy link
Member Author

Trott commented Jan 16, 2020

@BridgeAR @sam-github @AshCripps

@Trott
Copy link
Member Author

Trott commented Jan 16, 2020

Benchmark tests CI: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/11595/

@mscdex
Copy link
Contributor

mscdex commented Jan 16, 2020

It's probably better to throw instead.

@Trott
Copy link
Member Author

Trott commented Jan 16, 2020

It's probably better to throw instead.

Might be a good enhancement in the future. Before we can do that we'd need to:

  • Change the benchmark tests as described in Investigate flaky test-benchmark-misc  #31372 (comment) so that none of them depend on default options anymore
  • Add this "throw on undefined option" feature to the benchmark test runner (rather than inserting it into each and every benchmark)

@Trott Trott merged commit d2683ed into nodejs:master Jan 16, 2020
@Trott Trott force-pushed the fix-getstringwidth branch from 3c7c340 to d2683ed Compare January 16, 2020 06:00
@Trott
Copy link
Member Author

Trott commented Jan 16, 2020

Landed in d2683ed

@Trott Trott deleted the fix-getstringwidth branch January 16, 2020 06:00
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
@Trott @MylesBorins
benchmark: add default type in getstringwidth.js
6e978f7 
This fixes a benchmark test that was recently broken by a breaking
change on the master branch.

Fixes: #31372

PR-URL: #31377
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@codebytere codebytere mentioned this pull request Jan 16, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 14, 2020
@Trott @codebytere
benchmark: add default type in getstringwidth.js
c7c0dfe 
This fixes a benchmark test that was recently broken by a breaking
change on the master branch.

Fixes: #31372

PR-URL: #31377
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
@Trott @codebytere
benchmark: add default type in getstringwidth.js
267a01f 
This fixes a benchmark test that was recently broken by a breaking
change on the master branch.

Fixes: #31372

PR-URL: #31377
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@richardlau richardlau richardlau approved these changes

@cjihrig cjihrig cjihrig approved these changes

@sam-github sam-github sam-github 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Investigate flaky test-benchmark-misc
7 participants
@Trott @mscdex @sam-github @cjihrig @richardlau @BridgeAR @nodejs-github-bot