Add a visual-diff test for amp-by-example #9233
Conversation
|
Sorry if this has already been discussed but have you considered using a variant of |
|
@choumx, I just took two consecutive visual snapshots of It was Malte's idea to start with |
|
@erwinmombay, this is ready for review. Could you take a look? I'll watch the first few runs once this is pushed to master. |
|
@choumx, could you take a look please? |
build-system/pr-check.js
Outdated
| runVisualDiffTests: function() { | ||
| // This must only be run for push builds, since Travis hides the encrypted | ||
| // environment variables required by Percy during pull request builds. | ||
| execOrDie(`${gulp} visual-diff`); |
There was a problem hiding this comment.
Looks like this will cause the build to fail if visual-diff returns status 1, which can happen if Percy is misconfigured or a runtime error is thrown. We probably want a "or-don't-die" variant for now.
There was a problem hiding this comment.
FYI, the actual percy invocation in the visual-diff.js task was already wrapped in an "or-don't-die", but I see no harm in making sure pr-check.js doesn't terminate on failure either. I've got a bug assigned to me to refactor all these exec* functions into one library.
There was a problem hiding this comment.
Yea it's cause I noticed that extractPercyKeys has a few process.exit(1) calls.
| 'Must specify a webpage to diff via --webpage')); | ||
| process.exit(1); | ||
| // Default to the amp-by-example page for test runs. | ||
| // TODO(rsimha): Refactor this to support multiple webpages. |
There was a problem hiding this comment.
add pr issue number for todo
This PR adds a visual-diff test for the amp-by-example page using the Percy service. It visually compares the page as rendered by the AMP runtime at HEAD with a previously selected known good snapshot of the same page.
Note that visual diff tests will only be run for pushes to master, and will be skipped during regular pull requests. This is due to two reasons: 1) Travis does not make secure environment variables available during PRs (required by Percy) 2) Our existing Percy quota will be used more judiciously by running visual tests only after a PR is pushed to master
Fixes issues #8, #8340, #9069