-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Add visual diff test for story ads system layer #35619
Add visual diff test for story ads system layer #35619
Conversation
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.
Right now the ad is blank, is this intentional or are we not waiting long enough?
Description nits: The idea with the emojis is that they are for the title; see other PRs for example https://github.com/ampproject/amphtml/pulls
This is a bit of a confusing description, it makes it sound like we introduce some sort of css override when really we are just configuring the test. Something like "Add visual diff test for story ads" is more clear and accurate. If you really want to describe what visual diffs do, you could say something like "this helps us ensure future changes are the same visually" but it's not necessarily css properties that are being checked. Also please update the title to indicate this is not a test for the ad badge specifically. These may seem trivial but accurate descriptions can really help if you have to try to find an old PR 6 months from now. |
…he inline javascript that loads the amp-ad badge
✅ Tests
Added visual diff test and css selector override on load complete (story ads system layer, not testing the ad itself).
This helps us ensure future changes are the same visually.
At this moment we cannot test the ad itself, as iframe functionality is flaky in Percy.
For more info and details on how percy handles iframes, please reach out to Daniel Rozenberg.
Fixes #35474