🏗 Add new features to visual diff tests #17028
Conversation
| document.body.appendChild(iframe); | ||
|
|
||
| document.body.style.margin = '0'; // eslint-disable-line amphtml-internal/no-style-property-setting | ||
| iframe.style.border = '0'; // eslint-disable-line amphtml-internal/no-style-property-setting |
There was a problem hiding this comment.
Instead of using eslint-disable-line, you could add this file to .eslintignore, since I don't think it makes sense to lint it.
In addition, I think there's a check somewhere that requires the AMP copyright at the top of each JS file, that you can skip for this file.
test/visual-diff/visual-tests
Outdated
| * // AMP page, and enable JavaScript execution on the Percy side. | ||
| * // The code being executed *must* be inlined inside a <script> tag, not | ||
| * // be an externally loaded script via a <script src="..."> tag. | ||
| * // The code *WILL BE EXECUTED TWICE*! Once locally, before the DOM is |
There was a problem hiding this comment.
In the past, double execution of JS code was the reason behind bugs like percy/percy-capybara#51. I assume something has changed since then?
There was a problem hiding this comment.
By removing the AMP library from the page before uploading it to percy, we can avoid a lot of unexpected behaviour. Of course, the authors of those inlined scripts should still be aware of the possibility that things would happen unexpectedly, hence why I suggest using a guard here. I'd like to see how people use this feature before we refine it (insert generic quote about premature optimization)
danielrozenberg
force-pushed
the
visual-diff-features
branch
from
cea89fa to
c746e58
Compare
Codecov Report
@@ Coverage Diff @@
## master #17028 +/- ##
==========================================
+ Coverage 77.9% 77.92% +0.01%
==========================================
Files 562 562
Lines 41131 41131
==========================================
+ Hits 32043 32051 +8
+ Misses 9088 9080 -8
Continue to review full report at Codecov.
|
|
After discussing this with @newmuis offline, renamed |
|
@danielrozenberg Is it reasonable to allow multiple explicit viewport sizes? Like: {
"viewport": [
{ "width": 320, "height": 480 },
{ "width": 1920, "height": 1080 }
]
} |
|
@newmuis I can definitely look into that. Since I freeze the page and wrap it in an iframe, this would mean a full reload of the page in the new viewport size, so it's essentially like adding a new test |
|
@danielrozenberg whatever you prefer -- I can also just add a new test of the same page with the same params except for the |
|
Looking at what you did in #17110, I think it makes sense to keep them separate, especially since the way |
In the
visual-testsconfig file, add two new features forwebpages:"viewport": { "width": 123, "height": 456 }- set this test to have an exact viewport size. On Percy, only that explicit viewport size will be set. Wraps the document in an iframe after it's "settled" (all CSS classes are verified) because Percy doesn't understand explicit viewport sizes"enable_dirty_javascript": true- allows running "dirty" (non-AMP) JS inlined in the page. This will execute that code twice, once locally when preparing the page to render and once again on Percy.See added documentation in the PR's text for further explanation.
I tested this with @zhouyx's PR #16829, modified to scroll the page via code enabled by
enable_dirty_javascriptand constrained withviewport. Looks pretty good! https://percy.io/ampproject/danielrozenberg/builds/881708(the only issue remaining for that PR to work with these changes is that the ad doesn't show up when running in
--headlessmode, which we should investigate later)