Skip to content

Conversation

@mscdex
Copy link
Contributor

When no matching benchmark files are found, a more sensible error is thrown now.

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

@mscdexmscdex added the benchmark Issues and PRs related to the benchmark subsystem. label Apr 15, 2017
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'd add "in the current folder", but I don't run this often enough anyway.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

That would be confusing IMHO. The CLI searches all of the benchmark/ subdirectories for benchmarks specified on the command line. It never searches the current working directory.

When no matching benchmark files are found, a more sensible error is shown now.
@mscdexmscdexforce-pushed the benchmark-improve-error-message branch from 43926b1 to 3ca50a7CompareApril 16, 2017 16:08
@mscdex
Copy link
ContributorAuthor

I've tweaked this a bit after I discovered there is already a benchmark count check both in run.js and _cli.js.


if(benchmarks.length===0){
console.error('no benchmarks found');
console.error('No benchmarks found');
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

This matches the casing used in run.js.

jasnell pushed a commit that referenced this pull request Apr 17, 2017
When no matching benchmark files are found, a more sensible error is shown now. PR-URL: #12421 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

Landed in ccdcd91

@jasnelljasnell closed this Apr 17, 2017
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
When no matching benchmark files are found, a more sensible error is shown now. PR-URL: #12421 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
@evanlucasevanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
When no matching benchmark files are found, a more sensible error is shown now. PR-URL: #12421 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
When no matching benchmark files are found, a more sensible error is shown now. PR-URL: #12421 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
@gibfahn
Copy link
Member

Marking as dont-land because this depends on #7094, which is semver-major. LMK if I'm wrong.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

benchmarkIssues and PRs related to the benchmark subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@mscdex@jasnell@gibfahn@thefourtheye@benjamingr@cjihrig@joyeecheung@aqrln@jseijas