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 Tachometer Benchmarking #103

Merged
merged 9 commits into from
Aug 15, 2019
Merged

Conversation

Westbrook
Copy link
Contributor

@Westbrook Westbrook commented Aug 6, 2019

Description

Adds basic tachometer benchmarking to the various components

Future Work

  • Add comparison across releases to CI
  • Ensure element cross use case coverage

Motivation and Context

We want to know, maintain, and compare the performance of our components over the course on various additions and this will help clarify a specific numeric metric to describe the performance thereof.

How Has This Been Tested?

npm run build:tests && npm run test:bench makes the tests run non-comparatively across all of the components.

Screenshots (if appropriate):

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@Westbrook Westbrook force-pushed the westbrook/CRISP-1165/tachometer branch from 1721089 to 1ca5b04 Compare August 9, 2019 17:05
@Westbrook Westbrook marked this pull request as ready for review August 15, 2019 18:27
@Westbrook Westbrook requested a review from msdewey August 15, 2019 18:28
test/.tsbuildinfo Outdated Show resolved Hide resolved
Copy link
Contributor

@benjamind benjamind left a comment

Choose a reason for hiding this comment

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

Looks good, haven't run it yet, will do so shortly...

$ node test/benchmark/cli
Run all benchmarks for specific components:
$ node test/benchmark/cli -p button ripple
$ node test/benchmark/cli -p button -p ripple
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this referring to a ripple component we're probably never going to have?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be that a I copped much of this file (the whole file...) from a project that has such a component. Updating to a component me might have int eh future.

test/tsconfig.json Show resolved Hide resolved
max="100"
label="Opacity"
id="opacity-slider"
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't think you could have self closing custom elements?

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 catching all my copy/paste errors, yay! Updating the stories that this snippet came from. Looking into something like https://medium.com/storybookjs/component-story-format-66f4c32366df for the mono-repo to reduce our dependency on internally repeated code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats pretty cool, would be great to be able to have a 'benchmark' story that defines the tachometer fixture.

test/benchmark/sidenav/test-basic.ts Show resolved Hide resolved
test/benchmark/icon/test-basic.ts Outdated Show resolved Hide resolved
@Westbrook Westbrook mentioned this pull request Aug 15, 2019
benjamind
benjamind previously approved these changes Aug 15, 2019
Copy link
Contributor

@benjamind benjamind left a comment

Choose a reason for hiding this comment

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

Discussed offline to resolve tsconfig issues, assuming there's a change incoming for the updated config that resulted from that conversation. I'm going to approve now, but lets get someone else to run this since I was not able to run this under WSL at this time due to not being able to set the chrome binary path, this is being tracked by the tachometer team here: google/tachometer#70

@Westbrook
Copy link
Contributor Author

Thanks @benjamind, I also added the npm run build:tests to the readme, for system that can actually run the test...

@Westbrook Westbrook merged commit a92c21d into master Aug 15, 2019
@Westbrook Westbrook deleted the westbrook/CRISP-1165/tachometer branch August 15, 2019 21:39
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