-
Notifications
You must be signed in to change notification settings - Fork 4
Add more fixture files with Lighthouse data #22
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #22 +/- ##
============================================
+ Coverage 61.25% 63.09% +1.84%
+ Complexity 340 338 -2
============================================
Files 50 50
Lines 831 794 -37
============================================
- Hits 509 501 -8
+ Misses 322 293 -29
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
- name: Check if there are changes | ||
id: changes | ||
run: echo ::set-output name=changed::$([[ -z $(git status --porcelain) ]] && echo "0" || echo "1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to improve this specific step, otherwise there will always be changes, as the timestamp alone will already cause this to be triggered.
What about using jq
to fetch the lighthouseVersion
key, and only updating if that key has changed? It would mean storing the version somewhere, either in the name (which would require a smarter way of adding/removing the files) or as the contents of a separate file (which makes it easy to compare with a git diff
against that singe file).
bin/generate-fixture
Outdated
cd ../ | ||
|
||
# Generate the fixture file for the amp-page. | ||
./bin/px psi --json --key $PSI_API_KEY $publicUrl/amp-page/ > ./tests/fixtures/psi_local-wp_amp-page.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify the check for changes, we could extract the lighthouseVersion
here and store it in a psi_local-wp_amp-page.version
file. Then the workflow can just git diff
that file to decide whether a pull request needs to be changed.
Updated the |
@ediamin That looks pretty good so far. How many audits are we triggering so far with the current site setup? |
@schlessera I've added a testing plugin which will simulate bad audit scores for the following,
in addition to these, I've found that overall performance stays under 60 in multiple tests. |
Fixes #6
Currently we have only one single fixture file for the
https:\\amp-wp.org
page. The goal of this PR is to add more fixture files so we can use them with--url
flag.This PR introduces two new binaries,
generate-fixture
: This will spin up a WP site to test against PageSpeed Insights API. It'll create/update a fixture filepsi_local-wp_amp-page.json
file intests/fixtures
directory.stop-server
: It cleans up/destroys everything that setup bygenerate-fixture
. This is helpful when we work in our local.Prerequisite:
To make the action workable in GitHub, we need to setup two secrets:
PSI_API_KEY
: The PageSpeed Insights API key,NGROK_AUTH_TOKEN
: ngrok auth token that can be found in ngrok dashboard.To work in local
.env
file that can be generated from the.env.example
file.To-dos:
tests/fixtures
folder, using the URL and the versions of both PSI API and the embedded Lighthouse audit.ref: #6 (comment)