-
Notifications
You must be signed in to change notification settings - Fork 286
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
Fix flaky VRT tests #9101
Comments
FWIW the instability around the |
I've added a couple of ideas here to keep these stories consistent. I noticed the AC/descriptions suggested the second failure was in 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. |
@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 site-kit-wp/.storybook/main.js Lines 37 to 38 in 56feb1f
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. |
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 |
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.
Yes, let's get an issue opened to do that and start a conversation about what should be done there. Thanks! IB ✅ |
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:
Additionally, the test
Components/SelectionPanel/Default
has been seen failing in this test run:Visual diff:
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Modules/Analytics4/Widgets/DashboardOverallPageMetricsWidgetGA4/LoadedEntityURL
Components/SelectionPanel/Default
Implementation Brief
Modules/Analytics4/Widgets/DashboardOverallPageMetricsWidgetGA4/LoadedEntityURL
assets/js/modules/analytics-4/components/dashboard/DashboardOverallPageMetricsWidgetGA4.stories.js
, to the following:const currentEntityURL = 'https://www.example.com/example-page-3/';
.Why? This URL creates a faker seed in the mock report data generation code that returns an engagement rate of 0.27% which is a smaller value that does not trigger the resize code.
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
The text was updated successfully, but these errors were encountered: