search

ampproject / amphtml

The AMP web component framework.
https://amp.dev
Apache License 2.0
14.89k stars 3.89k forks source link

Add visual diff tests for amp-story #16460

Closed newmuis closed 5 years ago

newmuis commented 6 years ago

The initial attempt in #11682 ran into some issues, but now that the experiment is removed from 1.0, I believe we should be able to add these tests with no problem.

newmuis commented 6 years ago

A few more issues:

  1. Percy doesn't re-render the document for different sizes, it renders at one size and then re-uses a snapshot of the DOM with a different viewport size to generate the other screenshots. This is problematic because we actually use JavaScript to change the DOM on resize.
  2. The height of the viewport is automatically determined by Percy, loosely based on the size of the document. I've tried setting a fixed height on the <html> tag, the <body>, and the <amp-story>, to no avail; the viewport always remains too tall. Perhaps they have a min-height?
  3. The loading_complete_css, loading_incomplete_css, and forbidden_css selectors do not work to target elements that are contained within shadow DOM (such as <amp-story>'s system layer.
  4. Additionally, if the tests time out waiting for loading_complete_css to be in the DOM, but it is never found in the DOM, the test will continue rather than failing.

It's possible for us to write code in the runtime to combat some of these, but it feels wasteful for users to download code that exists to make the visual diff tests work.

/cc @danielrozenberg @rsimha

newmuis commented 6 years ago

3 and 4 can be mitigated by writing custom loading CSS classes in the test HTML files themselves, hooking into the events triggered by amp-story. 2 we might be able to live with for now; at the very least, we can change the height of the story and just leave blank space at the bottom of the screenshot. 1 is by far the biggest problem because it means we cannot test both desktop and mobile.

rsimha commented 6 years ago

The height and width of the viewport in which the local rendering step is done are currently hard coded, and used across all visual tests.

https://github.com/ampproject/amphtml/blob/dfa9a478b7f58a67c0a3f80a4f28fd7c225c4b3a/build-system/tasks/visual-diff.js#L37-L38

I'd imagine they can be set programmatically before snapshotting each page. Assuming this is possible, instead of doing the client-side rendering once and server-side rendering in three widths, we could consider doing client-side rendering once per viewport size, and server-side rendering once with the same size. This will generate the same number of snapshots, but the build could take longer on the client side. If the delta isn't too much, we can allow individual tests to specify the widths they're interested in testing so the full run doesn't take as long.

@danielrozenberg I think it's worth investigating a) whether it's possible to dynamically change the viewport size between tests, and if so, b) how much longer it would take to run the tests as described above.

newmuis commented 6 years ago

For the height, I think my issue is with the height of the screenshot, not necessarily the height of the viewport. The screenshots aren't 100000px, so I'm not sure where the height comes from.

rsimha commented 6 years ago

10000 is the maximum viewport height. It's used so that snapshots capture the entire page. Smaller pages generate smaller snapshots.

newmuis commented 6 years ago

That's the issue I'm running into: even if I make the page "smaller", there appears to be a minimum height that is always used. As such, I'm unable to make the page sufficiently small (in height) for my use case.

I just don't know where the minimum is coming from ☹️

newmuis commented 6 years ago

Between @danielrozenberg's fixes in #17028 and disabling amp-story's use of shadow DOM while the tests are running, we're most of the way there. One other issue is that it appears that CSS grid is not working in the tests. For example, from https://github.com/ampproject/amphtml/pull/17028:

Percy's screenshot (incorrect): screenshot of elements being laid out incorrectly, along the vertical axis

Browser screenshot (correct): screenshot of elements being laid out correctly, along the horizontal axis

Weirdly, I tested this locally in Firefox ESR 52 (on Mac OS), and it works fine.

newmuis commented 6 years ago

Reopened because these tests are very flaky.

My initial suspicion is that while waiting for the .i-amphtml-story-loaded class to be added to the DOM is sufficient for the actual content of the page, other UI chrome like the share pill and the desktop background may or may not be ready at that time.

So, my next step would be to try adding the CSS classes for all the other visible UI chrome to the loading_complete_css list, and running the tests many times to ensure that there are no flakes.

erwinmombay commented 6 years ago

This is a high priority issue but it hasn't been updated in awhile. @newmuis Do you have any updates?

ampprojectbot commented 6 years ago

This is a high priority issue but it hasn't been updated in awhile. @newmuis Do you have any updates?

ampprojectbot commented 5 years ago

This is a high priority issue but it hasn't been updated in awhile. @newmuis Do you have any updates?

ampprojectbot commented 5 years ago

This is a high priority issue but it hasn't been updated in awhile. @newmuis Do you have any updates?

newmuis commented 5 years ago

Closing this in favor of keeping all skipped tests tracked in #11639