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

Add visual diffs #28477

Open
ockham opened this issue Jan 25, 2021 · 6 comments · May be fixed by #28554
Open

Add visual diffs #28477

ockham opened this issue Jan 25, 2021 · 6 comments · May be fixed by #28554
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@ockham
Copy link
Contributor

ockham commented Jan 25, 2021

A recent refactor of the Cover block (to use an <img /> element rather than a CSS background, in order to enable srcsets) caused a number of visual regressions (e.g. #28242). Those regressions were addressed by a number of follow-up PRs (#28114, #28287, #28350, #28361, #28364, #28404).

That sequence of PRs (including some back-and-forth of alleged fixes and reverts) demonstrates that it's notoriously hard to get styling right across a matrix of possible configurations, such as different themes, frontend vs editor, and different block alignments. An attempt at a somewhat organized testing strategy (listing configurations that turned out especially fragile) was made at #28404 (comment).

In order to guard against similar regressions in the future, we should add some automated testing, as manual testing of these different configurations is tedious, and it's easy to miss something.

Unfortunately, it seems like we don't have an established mechanism for these:

  • Our snapshot testing focuses on block markup, i.e. their content and attributes -- not on their styling.
  • We cannot simply compare CSS classes or styling, since they might change (as in the example given above), whereas the resulting visuals might look the same.

For these reasons, I wonder if it's time to introduce a framework for visual diff testing. It could work on different levels (e.g. block level, or page level), and akin to our existing snapshot testing (store "expected" images in git, compare to those), or compare to the visual output of master.

For the issue documented above, it seems like per-block testing would be the most promising. (Puppeteer seems to support taking screenshots at DOM element level, so we're propably not very constrained on the implementation side.)

We could for starters try to implement taking a screenshot of the Cover block (with some sample background and content)

  • across different themes
  • both in the editor and the frontend

on each new PR, and compare them to the corresponding screenshots generated from the master branch.

These notes should serve as the benchmark: our automated tests should find all the problematic cases listed there (if all the relevant fixes are reverted, of course).

A library like Resemble.js (where it's possible to customize the level of "accuracy", and to highlight the differences visually) might come in handy.

Automattic's Calypso repo might have some prior art.

@ockham ockham added [Type] Task Issues or PRs that have been broken down into an individual action to take [Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. labels Jan 25, 2021
@mtias
Copy link
Member

mtias commented Jan 27, 2021

I remember @johngodley looked briefly at visual diffing for block validation. Perhaps some of that work / research could be useful.

@johngodley
Copy link
Contributor

I looked at using Jest Image Snapshot for the Layout Grid block (Automattic/block-experiments#129), which similarly has many different variations that makes manual testing difficult.

We haven't used it enough to know if the benefits outweigh the negatives so far, but it has spotted some minor changes. Also, it's currently being run manually by the developer - including it in a CI setup may have additional complications.

@gziolo
Copy link
Member

gziolo commented Jan 27, 2021

You should align with what @tellthemachines is doing for the WordPress core: WordPress/wordpress-develop#209.

Some prior work:

A slightly related issue for the standalone tests: #26184

You generally want to run visual testing on smaller parts of the UI to isolate changes introduced.

@ockham
Copy link
Contributor Author

ockham commented Jan 27, 2021

Thanks all! A combination of puppeteer (which we're using already for e2e tests) plus jest-image-snapshot seems rather promising.

You generally want to run visual testing on smaller parts of the UI to isolate changes introduced.

@gziolo Yeah, doing it at block level is definitely an option! As an experiment, we could initially just try it for the Cover block, as that's the block that prompted me to file this issue (and we have kind of a test baseline).

@mtias
Copy link
Member

mtias commented Jan 27, 2021

Side note, but as the patterns directory progresses, I'd love to be running these tests against the full directory eventually. (On demand.)

@ockham
Copy link
Contributor Author

ockham commented Jan 27, 2021

For starters, we might add something as simple as

it('renders correctly', async () => {
  const page = await browser.newPage();
  /* ... */
  const element = document.querySelector(
    /* cover block selector */
  );
  const image = await element.screenshot( { path: 'cover-block.png' } );

  expect(image).toMatchImageSnapshot();
});

to packages/e2e-tests/specs/editor/blocks/cover.test.js.
(Pseudo-code based on examples at https://github.com/americanexpress/jest-image-snapshot and https://pocketadmin.tech/en/puppeteer-screenshot-page-or-element/)

Compared to external services such as Percy.io or Chromatic, this should have the benefit of a lightweight "in-house" solution that ties nicely into our existing tooling.

@ockham ockham linked a pull request Jan 28, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Automated Testing Testing infrastructure changes impacting the execution of end-to-end (E2E) and/or unit tests. [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants