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

Add support for catch2 benchmarks #6

Merged
merged 3 commits into from
Jan 16, 2020
Merged

Conversation

bernedom
Copy link
Contributor

@bernedom bernedom commented Jan 12, 2020

Added support for benchmarks produced by the catch2 framework. Unfortunately the output is not very parser-friendly, so the implementation is not so nice.

To get CI to run on my repo I had to manually add a first set of data to the gh-pages, so CI might not run out of the box

@rhysd
Copy link
Member

rhysd commented Jan 13, 2020

Thank you! I'll review this.

BTW would you ignore the CI failure? I think it is a bug of validation script which does not consider the initial state of benchmark file.

CONTRIBUTING.md Show resolved Hide resolved
CONTRIBUTING.md Show resolved Hide resolved
src/extract.ts Outdated Show resolved Hide resolved
src/extract.ts Outdated Show resolved Hide resolved
src/extract.ts Outdated Show resolved Hide resolved
src/extract.ts Outdated Show resolved Hide resolved
@rhysd
Copy link
Member

rhysd commented Jan 13, 2020

TODO(@rhysd)

  • Describe Catch2 support in README
  • Create a new release with minor version up

src/extract.ts Show resolved Hide resolved
Copy link
Member

@rhysd rhysd left a comment

Choose a reason for hiding this comment

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

Thank you for the updates. Almost all looks good except for the condition of the check as I commented.

if (
ret.every(function(r) {
return r.range == '' && r.unit == '';
})
Copy link
Member

@rhysd rhysd Jan 15, 2020

Choose a reason for hiding this comment

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

Since we should verify all elements have valid values here, I think following is correct:

ret.some(function(r) {
    return r.range === '' || r.unit === '';
})

EDIT: Oh, and please use === for equality. == should never be used for JS/TS :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are of course correct. Thanks for the hint about ===, as this is the first time I touched TS/JS such hints are very welcome. The fix is underway.

Copy link
Member

@rhysd rhysd Jan 15, 2020

Choose a reason for hiding this comment

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

In case you don't know the detail, == is very loose since it permits implicit conversion in operands: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comparison_Operators#Equality

Please never mind about you're not familiar with JS/TS. Thank you for diving into a new language for contributing this repository!

@rhysd
Copy link
Member

rhysd commented Jan 15, 2020

I enabled eqeqeq rule of eslint which forces use of === (I apologize it was missing in eslintrc). Would you take a look at all comments from GitHub Action in https://github.com/rhysd/github-action-benchmark/pull/6/files?

@bernedom
Copy link
Contributor Author

Sure, I will check the failing eqeqeq rules later today or tomorrow.

@rhysd
Copy link
Member

rhysd commented Jan 15, 2020

If you installed all dependencies with npm install, running npm run fix would fix them automatically. Please try.

Copy link
Member

@rhysd rhysd left a comment

Choose a reason for hiding this comment

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

OK, looks good. Thank you for adding this.

@rhysd rhysd merged commit 99ae26b into benchmark-action:master Jan 16, 2020
@rhysd rhysd mentioned this pull request Jan 17, 2020
@rhysd
Copy link
Member

rhysd commented Jan 21, 2020

Notification: This was released at v1.7.0.

https://github.com/rhysd/github-action-benchmark/releases/tag/v1.7.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants