Skip to content

test(vrt): force-disable animations during VRT#2156

Merged
spike-rabbit merged 2 commits into
mainfrom
test/vrt/stable-animations
Jun 19, 2026
Merged

test(vrt): force-disable animations during VRT#2156
spike-rabbit merged 2 commits into
mainfrom
test/vrt/stable-animations

Conversation

@dr-itz

@dr-itz dr-itz commented Jun 16, 2026

Copy link
Copy Markdown
Member

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates several visual regression tests by removing custom pixel difference thresholds and updating the corresponding PNG snapshots. It also introduces animation stabilization logic in the test helpers when updating snapshots. The reviewer recommends applying this animation stabilization logic to all visual regression test runs, rather than only when updating snapshots, to prevent test flakiness due to discrepancies between snapshot generation and verification.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread playwright/support/test-helpers.ts Outdated
Comment thread playwright/support/test-helpers.ts Outdated
@dr-itz dr-itz force-pushed the test/vrt/stable-animations branch from 9fd0256 to acf1fcc Compare June 16, 2026 12:22
@dr-itz dr-itz changed the title test(vrt): make sure VRT snapshots are stable regarding animations test(vrt): force-disable animations during VRT Jun 16, 2026
@dr-itz dr-itz marked this pull request as ready for review June 17, 2026 10:51
@dr-itz dr-itz requested review from a team as code owners June 17, 2026 10:51
@dr-itz dr-itz requested a review from spliffone June 17, 2026 10:51

@spike-rabbit spike-rabbit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about just having a forceDisableAnimationsFlag?
This is only needed in rare cases.
Add least the overall job run seemed to be 30s slowser than before

@dr-itz

dr-itz commented Jun 17, 2026

Copy link
Copy Markdown
Member Author

Overall 30s slower isn't realistic, that from what, one run on unpredictable CI?

Some timings, locally on M1 Max with 4 workers and 5 vCPUs in the VM:

force disable all:   649 passed (8.3m)
explicit:            649 passed (7.8m)
combined:            649 passed (8.1m)
  • force disable all is this MR as-is
  • explicit is your suggestion
  • combined is realising that we disable animations anyway for a11y tests, so move the disable/re-enable a level higher and wait for animations to have finished before VRT

So force-disable is 30s slower without shards.

Combined 0.3 minutes slower, that 18s (that's like 6s per job) while making everything more reliable, i.e. what Playwright should do in the first place. So I would argue that is the best approach. And CI timings vary anyway so reliability over a few saved seconds.

@spike-rabbit

Copy link
Copy Markdown
Member

you are right, then lets do the combined one.

@dr-itz dr-itz force-pushed the test/vrt/stable-animations branch from acf1fcc to 9abfd56 Compare June 18, 2026 19:44
@dr-itz

dr-itz commented Jun 18, 2026

Copy link
Copy Markdown
Member Author

Done. Slowest e2e job this run was 5m36s, so not bad at all.

@spike-rabbit spike-rabbit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@spike-rabbit spike-rabbit added this pull request to the merge queue Jun 19, 2026
Merged via the queue into main with commit 0998594 Jun 19, 2026
18 checks passed
@spike-rabbit spike-rabbit deleted the test/vrt/stable-animations branch June 19, 2026 06:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants