google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 293 forks source link

Enhance VRT Infrastructure - Constrain to Viewport (With Option to Override) & Allow Specified Scenario Runs Only #9764

Open 10upsimon opened 1 week ago

10upsimon commented 1 week ago

Feature Description

Enhancement 01

While working on #9375 it was discovered that elements that should be fixed to the bottom of the viewport (or other such element) - via position: fixed; bottom: 0; style implementations - are appearing as floating in the middle of nowhere in the actual VRT test bitmaps. An example of this can be seen below:

image.png

Upon assessing the viewport definitions at tests/backstop/viewports.js, specifically taking the small viewport into account...

    {
        label: 'small',
        width: 420,
        height: 580,
    },

... It was discovered that the apparent floating element was indeed ending at the 580px vertical mark, indicating that - according to the defined viewport size - it was indeed correctly positioned.

Further investigation revealed that none of the VRT test images were adhering to the viewport constraints. This is due to the default selector in Backstop being document. Therefore, the entire document as being captured.

The selectors property can be applied to scenarios to specify which selector you'd like Backstop to "capture". An obvious fix for this seemed to be the application of selectors: [ 'viewport ] to each scenario.

In theory, this would have been an acceptable approach, however some stories are more style-guide in nature, and require the full canvas of of the story to be captured during testing, and our existing implementation of Backstop and/or Puppeteer does not appear to respect this selector. Examples stories include:

Digging deeper, it was clear that two key features are needing to be introduced to solve this issue:

  1. The ability to constrain the story to the viewport by default
  2. The ability to "break out" of this constraint for stories that require it

In order to achieve this, a few key aspects will need to be introduced, namely:

  1. Introduction of an (optional) story parameter: constrainToViewport with a default value of true
  2. Parsing of this property at a scenario level in tests/backstop/scenarios.js for both modern and legacy story traversals
  3. Based on the above, conditionally setting the value of the selectors property for each scenario to one of the following:
    • selectors: [ '#document ], for stories that should be constrained to the viewport dimensions (default), more details on this further below
    • selectors: [ 'document' ] for stories that should not be constrained to the viewport dimensions
  4. An additional decorator within .storybook/preview.js that conditionally renders a new div wrapper with ID document for viewport constraint stories. This additional element would have CSS that constrains it's dimensions to the viewport, and prevents overflow of content. This is required in order to fix the default behaviour of Backstop not adhering to the [ 'viewport' ] selector. This element should purposefully be named #document so that all existing VRT images that do continue to pass are named identical to the default [ 'document' ] selector, thus limiting the failures. If this was not the case, ALL existing VRT tests would fail.

Following the above, a story who wishes to break out of the viewport constraint would simply pass the applicable parameter:

...
decorators: [
        ( Story, { args } ) => {
                     ...
        },
    ],
        {
            parameters: {
                constrainToViewport: false,
            }
        }

Thus allowing the fill canvas to be captured as prior.

Not only will this transform the VRT captures to be true to our viewport definitions, it'll also reduce the size of the VRT reference folder and the test payload itself. Although this may not have an execution-time based impact, it's still less energy, a less wasteful operation overall.

Enhancement 02

While exploring possible solutions related to the above, it was becoming increasingly clear that having to run the entire VRT suite over and over was becoming a) time consuming and b) more prone to error should system resources become unstable (resulting in false failures, timeouts etc).

Part of this VRT enhancement sees the ability to specify a single or multiple scenario labels that should be run as part of the VRT test. This is a simple yet effective enhancement, and will greatly reduce developer overhead when needing to run and re-run VRT tests locally when working on a single or small handful of component updates.

Given the fact that story scenarios are assigned to a single constant, this can easily be achieve by:

This would allow developers to run single or multiple story scenarios for purposes of VRT via a command like or similar to the following:

npm run test:visualtest -- --labels="KeyMetrics/MetricSelectionPanel"

This would be a welcome enhancement for local development.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

aaemnnosttv commented 1 week ago

@10upsimon to summarize our recent discussions