Add visual diff test for amp-user-notification #20635
Conversation
|
You need to resolve the conflict for Travis→Percy to run |
There was a problem hiding this comment.
Approval for test itself; I'll leave it to @torch2424 to approve the AMP page and Percy build
test/visual-diff/visual-tests
Outdated
| @@ -600,5 +600,14 @@ | |||
| ], | |||
| "interactive_tests": "examples/visual-tests/amp-story/amp-story-page-attachment-desktop.js" | |||
| }, | |||
| { | |||
| "url": "examples/visual-tests/amp-user-notification/amp-user-notification-blocking.amp.html", | |||
There was a problem hiding this comment.
I think that it might be worth changing the file name to amp-user-notification.amp.html, since this is testing both blocking and unblocking, and you might want to check other things as well in later tests. Your choice of course :)
There was a problem hiding this comment.
Was going to have two different html files. Completely agreed amp-user-notification.amp.html is a better name given there's only one file.
test/visual-diff/visual-tests
Outdated
| "loading_complete_css": [ | ||
| "amp-user-notification.amp-active", | ||
| ], | ||
| "interactive_tests": "examples/visual-tests/amp-user-notification/dismiss-amp-user-notification.js" |
There was a problem hiding this comment.
Rename this file to amp-user-notification-blocking.js (or amp-user-notification.js, if you accept the previous comment)
There was a problem hiding this comment.
renamed to amp-user-notification.js
| <link rel="canonical" href="amps.html" > | ||
| <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1"> | ||
| <link href='https://fonts.googleapis.com/css?family=Georgia|Open+Sans|Roboto' rel='stylesheet' type='text/css'> | ||
| <style amp-custom> |
There was a problem hiding this comment.
Do we need the CSS for this? 🤔 😄
There was a problem hiding this comment.
lol, no I don't need the css. remove unrelated css. I still want to keep the amp-user-notification related css for easier visual diff comparison.
| </div> | ||
| </div> | ||
| <div class="article-body" itemprop="articleBody"> | ||
| <p> |
There was a problem hiding this comment.
Do we need this much sample text? 🤔 😄
There was a problem hiding this comment.
lol, no we don't. cleaned up a bit. However I still want to keep enough content to make the page scrollable.
| module.exports = { | ||
| 'dismiss user notification': async (page, name) => { | ||
| await page.tap('amp-user-notification>button'); | ||
| await page.waitFor(500); |
There was a problem hiding this comment.
Could the veryfyCssElements thing work here? 😄
There was a problem hiding this comment.
I tried that before. Unfortunately there isn't a good css class to wait for given we need to wait for the ad iframe to load.
|
@torch2424 This is good for another round of review. Thank you |
| "url": "examples/visual-tests/amp-user-notification/amp-user-notification.amp.html", | ||
| "name": "amp-user-notification", | ||
| "viewport": {"width": 400, "height": 600}, | ||
| "loading_complete_css": [ |
There was a problem hiding this comment.
@danielrozenberg Do you think it would be better practice to have test functionality all in the JS, or a mix between config and their JS.
I think we could move this into the JS file, and use the verifyCss... function right? I feel like that would be slightly cleaner 😄
There was a problem hiding this comment.
They serve a different purpose - the JS code doesn't start running until after the loading_complete_css is verified; that means you should use loading_complete_css to mark when the page is ready for interactions, and move forward from there
There was a problem hiding this comment.
Ahh. Sounds good! Thank you for the explanation! 😄
* add visual diff test for amp-user-notification * address comments
* add visual diff test for amp-user-notification * address comments
No description provided.