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: compare local and latest builds of a component #978

Merged
merged 1 commit into from
Oct 23, 2020
Merged

Conversation

Westbrook
Copy link
Contributor

Description

Update to the latest version of tachometer so that we can easily compare local builds to the most recent version available on NPM.

  • ensure all benchmarks actually run in the environment
  • prep CLI to properly configure the use of tachometer
  • add/expand some test
  • bump test subjects from 10 => 100 for more clear differences in perf scores

Related Issue

refs #137

Motivation and Context

We want to start tweaking the internals of our elements for performance and feature completeness and it is useful to have baseline comparisons to know that we're moving in a good or at least acceptable direction.

How Has This Been Tested?

Manually on local against some future looking performance changes.

Screenshots (if appropriate):

image
Current output where remote is the performance of the code available on the latest channel on NPM and <none> is that of the code available locally.

Types of changes

  • Tooling

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.

Copy link
Contributor

@davidyxu davidyxu left a comment

Choose a reason for hiding this comment

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

seems good to me, although I wasn't able to complete the benchmark on field-group, not sure if thats related to the updates here:

yarn test:bench -p field-group

gives me this message:

Failed 1/3 times to get measurement(s) field-group:basic-test in chrome from http://127.0.0.1:8081/test/benchmark/bench-runner.html?bench=basic-test&package=field-group. Retrying.

in the browser i see this error:

Uncaught (in promise) TypeError: Failed to resolve module specifier "@spectrum-web-components/field-group/sp-field-group.js". Relative references must start with either "/", "./", or "../".

@Westbrook
Copy link
Contributor Author

@davidyxu Thanks for diving so deep on this! 🙏 @spectrum-web-components/field-group has yet to be released to NPM, so it won't be able to be tested in this way. 😓 It is useful that you brought it us as it was generated from files that didn't have the correct pathing for test/benchmark/helpers.js, so I've corrected that here and in the generator so we shouldn't have that issue in the future.

In collaboration with Alex from the LRWeb team, we're gathering some changes together to try to reduce the broader effects of a couple of breaking changes from Spectrum CSS before our next round of releases. That should hopefully come in the next week or two 🤞 and then @spectrum-web-components/field-group et al will be available for testing in this way.

`&package=${packageName}`
);
if (!config.benchmarks) return;
config.benchmarks.push({
Copy link
Contributor

Choose a reason for hiding this comment

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

In regards to the unpublished npm package issue I ran into with field-group, it would be a nice-to-have in the future to skip the remote test if its not available.

a couple options might be to either to use some standardized proess with the package version to determine if its been published or to actually check for latest in npm (although this might be a bit more heavy)

yarn info @spectrum-web-components/field-group@latest

speaking of which, it might also make sense to limit the benchmark tests to the same major version instead of against latest, in case theres been an interface change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! I've added the following message when a package is still on the "default" (read here as "unpublished) version:

⚠️  It looks like '${packageName}' has yet to be published to NPM. Skipping!

I've also added the -c/--compare flag which allows for the use of a specific version of @spectrum-web-components/bundle to be used as the "remote" source against which to test. This is done so that packages that require child components both public or private are able to rely of releases of all the comparable version at a time.

Copy link
Contributor

@davidyxu davidyxu left a comment

Choose a reason for hiding this comment

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

left some thoughts on the field-group stuff, but otherwise lgtm.

@Westbrook Westbrook merged commit 784d750 into next Oct 23, 2020
@Westbrook Westbrook deleted the westbrook/perf branch October 23, 2020 16:25
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