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

🏗✅ Make visual diff tests hermetic #21935

Merged

Conversation

danielrozenberg
Copy link
Member

@danielrozenberg danielrozenberg commented Apr 22, 2019

This PR:

  • replaces all* external resources used by visual diff tests with local ones
  • replaces images that I don't know where they're from with pictures from Picsum, which are in turn pictures from Unsplash, which are Public Domain pictures
  • BLOCKS external (non-localhost) network for access for visual diff tests, unless they are defined with the new flag allow_external_network_access set to true
    • Currently only one test is using this flag, and I will work with its author to remove external network dependency if possible
    • Some external resources are required by some tests, such as those coming from cdn.ampproject.org or fonts.googleapis.com. Those are now MOCKED by the test runner (i.e., the test runner returns a local, frozen copy of those external assets)
  • Disables a single test that I could not make work. After submitting this PR I will try to fix it

Progress on #21056

*all that are required to make the tests pass

Copy link
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Nice! How does this look on Travis?

Approving to unblock.

@danielrozenberg
Copy link
Member Author

Literally just sent this PR to review, it didn't even have time to run on Travis 😅

@zombifier
Copy link
Contributor

zombifier commented Apr 22, 2019

Hi, I wrote the GPT.js test which is the only test remaining that requires external access. The test never loaded AMP locally since there is no support for proxying (I could change GPT itself so it loads AMP locally but didn't find the time to do so), and with your submitted changes it would likely break entirely.

Even with that fixed the test relies on loading gpt.js, whose new versions are released very rapidly, and as such would not work for long with a local cached version. In my opinion you can delete the test completely (and don't add allow_external_network_access). In the future I will create more basic AMP creative tests for both AMP and non-AMP pages which wouldn't need external network access at all.
Thanks.

@danielrozenberg
Copy link
Member Author

@zombifier done - I removed the optional flag and disabled (but not completely removed) your test. I'll be happy to work with you to modify this test to be hermetic!

@danielrozenberg
Copy link
Member Author

@rsimha approve the Percy build for me? Can always use an extra pair of eyes instead of blindly approving mine.

Things to note:

  • I'm not sure why some of the images have those tiny pixel shifts, maybe the network cached a different version of them before?
  • I swapped the images for amp-consent and amp-according with images from picsum.photos, which are Public Domain images. I'm just not sure where those other images came from, so I prefer to swap them...

@rsimha
Copy link
Contributor

rsimha commented Apr 22, 2019

Approved diffs. Some images appear to have changed, which seems reasonable given what you're doing in this PR. A good check might be to re-run this and see if the same diffs occur.

@danielrozenberg
Copy link
Member Author

I reran this several times locally (I don't want to jinx this successful execution by rerunning it on Travis… there's still #21882 that should help fix the timeouts) and the results are consistent. Merging this now…

@danielrozenberg danielrozenberg merged commit 98000ef into ampproject:master Apr 23, 2019
@danielrozenberg danielrozenberg deleted the visual-diff-hermetic-network branch April 23, 2019 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants