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 Performance on CI #170

Merged
merged 12 commits into from
Aug 8, 2021
Merged

Test Performance on CI #170

merged 12 commits into from
Aug 8, 2021

Conversation

JRJurman
Copy link
Member

@JRJurman JRJurman commented Aug 6, 2021

Summary

This PR adds the configuration to run the performance tests on CircleCI, makes them a little more consistent, and pushes the performance tests (and build badges!) back to the repo.

  • When working on a non-master branch, this creates a new branch with the -artifacts suffix.
    • These branches never have to be merged (we will have to clean them up later, but that's fine for now)
  • When working on master (after a PR has been merged), this creates a new commit straight on master
    • This will include the final performance result and badge result, regardless of whatever was committed in the PR

Performance / Size

Performance Results - performance-test-on-ci...performance-test-on-ci-artifacts

Resources

Checklist

  • PR Summary
  • Tests
  • Version Bump

@JRJurman JRJurman force-pushed the performance-test-on-ci branch 2 times, most recently from 306119e to 809793b Compare August 6, 2021 23:04
@JRJurman JRJurman changed the title [WIP] test performance on ci Test Performance on CI Aug 6, 2021
@@ -10,7 +10,7 @@ const start = require('./start')
* @package
* @public
* @description
* Tram-One is a light View Framework that comes with all the dependencies you need to start developing on the web.
* Tram-One is a Modern View Framework that has advance features like hooks, observables, and JSX-like template components, all in plain vanilla javascript.
Copy link
Member Author

Choose a reason for hiding this comment

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

needed something to test that the badge sizes would be updated, this worked, and this needed to be updated anyways.

Comment on lines +109 to +111
expect(stats['0050'].averageTime / 5).not.toBeGreaterThan(stats['0010'].averageTime + buffer)
expect(stats['0500'].averageTime / 5).not.toBeGreaterThan(stats['0100'].averageTime + buffer)
expect(stats['5000'].averageTime / 5).not.toBeGreaterThan(stats['1000'].averageTime + buffer)
Copy link
Member Author

Choose a reason for hiding this comment

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

using averageTime instead of medianTime, since that should be the least error prone (with the outliers removed)

// number of times to run the test
const NUMBER_OF_RUNS = 50
// the number of tests to toss (slowest and fastest)
const BUFFER = 10
Copy link
Member Author

@JRJurman JRJurman Aug 7, 2021

Choose a reason for hiding this comment

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

including a buffer now, which is the number of slowest and fastest runs that we'll toss when doing calculations. In reality, this means that we are only getting stats for 30 runs (50 runs, - 10 slowest, - 10 fastest), but these should be more consistent overall.

Comment on lines +50 to +51
- store_artifacts:
path: performance-tests/__snapshots__
Copy link
Member Author

Choose a reason for hiding this comment

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

while not totally required, this does give us a historical view within a branch (since when we make our -artifacts branch, we always just force-push over the commit)

Comment on lines +65 to +72
if [[ -z "${CIRCLE_PULL_REQUEST}" ]]
then
echo "Master Build, checkout to master"
git checkout
else
echo "PR Build, checkout to new branch"
git checkout -b ${CIRCLE_BRANCH}-artifacts
fi
Copy link
Member Author

Choose a reason for hiding this comment

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

Some configurations only commit when on the master branch, but since we want information on PRs as well, this is allows us to account for both 👍

echo "PR Build, checkout to new branch"
git checkout -b ${CIRCLE_BRANCH}-artifacts
fi
git commit --allow-empty -am "Update Snapshots & Badges [skip ci]"
Copy link
Member Author

Choose a reason for hiding this comment

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

[skip ci] is important here, this actually prevents CircleCI from infinitely triggering builds.

git checkout -b ${CIRCLE_BRANCH}-artifacts
fi
git commit --allow-empty -am "Update Snapshots & Badges [skip ci]"
git push -fu origin HEAD
Copy link
Member Author

Choose a reason for hiding this comment

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

we force push the artifacts onto the branch, this is required for the Pull Request use case, less so for master, but I don't foresee that being an actual issue.

Copy link
Contributor

@chtinahow chtinahow left a comment

Choose a reason for hiding this comment

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

👍

@JRJurman JRJurman mentioned this pull request Aug 7, 2021
@JRJurman JRJurman merged commit 1c6d04c into master Aug 8, 2021
@JRJurman JRJurman deleted the performance-test-on-ci branch August 8, 2021 14:32
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