WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.46k stars 4.18k forks source link

Add visual diffs #28477

Open ockham opened 3 years ago

ockham commented 3 years ago

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. https://github.com/WordPress/gutenberg/issues/28242). Those regressions were addressed by a number of follow-up PRs (https://github.com/WordPress/gutenberg/pull/28114, https://github.com/WordPress/gutenberg/pull/28287, https://github.com/WordPress/gutenberg/pull/28350, https://github.com/WordPress/gutenberg/pull/28361, https://github.com/WordPress/gutenberg/pull/28364, https://github.com/WordPress/gutenberg/pull/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 https://github.com/WordPress/gutenberg/pull/28404#issuecomment-764904883.

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:

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)

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.

mtias commented 3 years ago

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

johngodley commented 3 years ago

I looked at using Jest Image Snapshot for the Layout Grid block (https://github.com/Automattic/block-experiments/pull/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 commented 3 years ago

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

Some prior work:

A slightly related issue for the standalone tests: https://github.com/WordPress/gutenberg/issues/26184

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

ockham commented 3 years ago

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 commented 3 years ago

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

ockham commented 3 years ago

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.