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.23k stars 279 forks source link

Fix flaky VRT tests #9101

Open techanvil opened 1 month ago

techanvil commented 1 month ago

Feature Description

The VRT test Modules/Analytics4/Widgets/DashboardOverallPageMetricsWidgetGA4/LoadedEntityURL is sporadically failing in CI, as can be seen in some recent VRT workflow failures:

Visual diff: image

Additionally, the test Components/SelectionPanel/Default has been seen failing in this test run:

Visual diff:

image


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

Acceptance criteria

Implementation Brief

Modules/Analytics4/Widgets/DashboardOverallPageMetricsWidgetGA4/LoadedEntityURL

Components/SelectionPanel/Default

This appears to be the difference in the focussed or unfoccuses state of the first checkbox.

Test Coverage

QA Brief

Changelog entry

aaemnnosttv commented 2 weeks ago

FWIW the instability around the LoadedEntityURL (the only current unstable scenario) seems to be due to the changes introduced in #9029 and the responsive font-sizing.

benbowler commented 1 week ago

I've added a couple of ideas here to keep these stories consistent. I noticed the AC/descriptions suggested the second failure was in SetupMainPAX but the linked VRT run failed in Components/SelectionPanel/Default so I updated this in the ticket and addressed this in the IB.

I've added a larger estimate as without executing on the ticket I cannot confirm this has the desired effect on multiple GitHub actions runs. I could reduce the estimate if we aim to bounce this back to IB/create a new ticket if the suggested implementation fails to make these tests consistent or other instabilities appear in other tests that we want to address.

aaemnnosttv commented 6 days ago
  • Update assets/js/components/DataBlockGroup.js, add this new block to prevent the rescaling loop to run in the storybook context, add a clear comment to explain this addition:

@benbowler It's important to differentiate VRT from Storybook since these are separate, it's just that VRT uses Storybook. We do have the ability to conditionally alter the way Storybook is run in a VRT context however. See https://github.com/google/site-kit-wp/blob/56feb1f6ca227cc9fc6da3511a96fb2f4bf9d3b0/.storybook/main.js#L37-L38

This is done to add additional CSS to disable animations for the same reason.

With that said, I'd prefer we don't change the behavior of the component to work differently in VRT as that undermines the purpose of the VRT (we may as well just remove that scenario). Animations can't be tested by VRT however, and the above essentially just freezes that from causing issues during screenshotting. As an external/environmental change, it's not a contradiction to the previous statement.

I think part of the problem is that the responsive adaptation only happens in response to resize or mount, when the text content (metrics) can change in between. There's a number of issues with the implementation which should probably be addressed in a separate issue, but I think the simplest fix here is probably to fix the data to use values that won't result in downscaling. That way we can keep the scenario while avoiding changes to production code.

benbowler commented 3 days ago

Hey @aaemnnosttv makes sense, I've updated the IB, finding a URL that creates a faker seed that returns 0.27% for the engagement rate which prevents resizing at 420px.

Do you want me to create the ticket to refactor the DataBlockGroup component?

aaemnnosttv commented 8 hours ago

Thanks @benbowler – while this a little bit hacky, it seems like a fine solution here so long as we document that very clearly as to why that specific URL is important.

Do you want me to create the ticket to refactor the DataBlockGroup component?

Yes, let's get an issue opened to do that and start a conversation about what should be done there.

Thanks!

IB ✅