Skip to content

Stabilize Mobile Visual Snapshots#2936

Draft
google-labs-jules[bot] wants to merge 10 commits into
mainfrom
feat/stabilize-mobile-snapshots-5203909596431698698
Draft

Stabilize Mobile Visual Snapshots#2936
google-labs-jules[bot] wants to merge 10 commits into
mainfrom
feat/stabilize-mobile-snapshots-5203909596431698698

Conversation

@google-labs-jules

Copy link
Copy Markdown
Contributor

This PR stabilizes the Playwright visual regression suite to prevent false-positive failures across different environments (local vs. CI).

Key improvements:

  • Standardized Environments: Added dedicated Mobile Chrome and Mobile Safari projects to playwright.config.ts, ensuring consistent emulation parameters.
  • Automated Snapshot Management: Migrated multiple test files from manual page.screenshot() to Playwright's native expect(page).toHaveScreenshot(). This allows Playwright to handle environment-specific snapshot naming (e.g., -linux.png) automatically.
  • Enhanced Stability Helpers:
    • Updated disableAnimations to suppress View Transitions and force immediate scrolling.
    • Added a global document.fonts.ready check to the page fixture to ensure snapshots are captured only after web fonts have settled.
  • Test Cleanup: Removed manual viewport and device overrides in individual test files to ensure they inherit the standardized project settings.

Fixes #2900


PR created automatically by Jules for task 5203909596431698698 started by @arii

- Standardize mobile projects in playwright.config.ts (Pixel 5, iPhone 12)
- Refactor mobile tests to use toHaveScreenshot assertions
- Remove manual device emulations in favor of project configuration
- Expand disableAnimations to cover View Transitions and scroll behavior
- Ensure fonts are fully loaded globally before taking snapshots
@google-labs-jules

Copy link
Copy Markdown
Contributor Author

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@arii

arii commented Jun 25, 2026

Copy link
Copy Markdown
Owner

🤖 AI Technical Audit

ANTI-AI-SLOP

The refactoring of visual regression tests is generally high-quality and aligns with Playwright best practices. However, the verify_ux_consistency.spec.ts test remains an anti-pattern. Mixing multiple test scenarios (homepage/detail page, mobile/desktop) into a single monolithic test case makes failures difficult to debug and prevents parallelization. Furthermore, the use of isMobile checks inside the test body to toggle screenshot paths negates the benefit of Playwright's project-based configuration. Tests should rely on the project setup, not procedural logic.

FINAL RECOMMENDATION

approved with suggestions

Review automatically published via RepoAuditor.

@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

🚀 Deployment Details (Last updated: Jun 26, 2026, 3:06 PM PST)

🚀 Pushed to gh-pages; publish in progress

- Add 'Mobile Chrome' and 'Mobile Safari' projects to playwright.config.ts
- Migrate tests to use expect(page).toHaveScreenshot() for deterministic comparison
- Remove manual viewport and device emulations in favor of project settings
- Add view-transition-name: none and scroll-behavior: auto to disableAnimations
- Await document.fonts.ready in page fixture to prevent typography drift
- Relocate and rename snapshots to match new project naming conventions
- Standardize mobile projects (Mobile Chrome, Mobile Safari)
- Migrate all visual tests to use the custom visual fixture (fixtures/visual.ts)
- Implement global font-loading and animation suppression in the fixture
- Replace manual screenshot paths with toHaveScreenshot assertions
- Relocate snapshots to spec-specific directories with project-specific naming
- Skip mobile tests in desktop projects to prevent dimension mismatches
- Use lowercase project names (mobile-chrome, mobile-safari)
- Use hyphenated snapshot names to match Playwright discovery
- Migrate all visual tests to custom fixture with font-ready guards
- Relocate and rename snapshots for project-based comparison
- Skip mobile tests in desktop project runs

@arii arii left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Issue audit result

Recommendation: Completed, close

Reason:
The PR implements standard visual test approaches, replacing manual page.screenshot() with expect(page).toHaveScreenshot() across mobile tests. It updates playwright.config.ts to standard 'chromium', 'mobile-chrome', and 'mobile-safari' projects. It introduces test.skip(({ isMobile }) => !isMobile, ...) where appropriate and disables CSS animations globally to reduce flakiness. Snapshot file names now match the required standard. All CI steps including visual tests succeeded.

Implementation evidence:

  • Files checked: playwright.config.ts, tests/affiliate-mobile.spec.ts, tests/blog-post-mobile.spec.ts, tests/fixtures/visual.ts, tests/research-mobile.spec.ts, tests/search_mobile.spec.ts, tests/utils/playwright-helpers.ts, tests/verify_ux_consistency.spec.ts, tests/visual-affiliate.spec.ts
  • PRs checked: #2936
  • Routes checked: N/A
  • Tests or validation: CI tests successfully passed on this PR, including Build & E2E checking snapshots.

Remaining work:
None.

@arii

arii commented Jun 25, 2026

Copy link
Copy Markdown
Owner

@jules-fix-ci

@github-actions

Copy link
Copy Markdown
Contributor

🤖 Jules is on it!

Initialized autonomous repair session (sessions/4812018917954590741) for branch feat/stabilize-mobile-snapshots-5203909596431698698.

@arii

arii commented Jun 25, 2026

Copy link
Copy Markdown
Owner

🤖 AI Technical Audit

ANTI-AI-SLOP

The refactoring is generally sound; however, verify_ux_consistency.spec.ts remains a monolithic test file that violates the SRP (Single Responsibility Principle). While consolidating snapshots is good, the test contains excessive conditional branching (if (isMobile)) that makes local debugging difficult and hides the true state of the application across different device profiles. Please break these into separate test files or use project-specific tagging.

FINAL RECOMMENDATION

Approved with Minor Changes

DEFINITION OF DONE

  1. Refactor verify_ux_consistency.spec.ts to remove the isMobile conditional branches. Use test.describe or split into homepage.spec.ts and guide.spec.ts utilizing project-based configuration.
  2. Verify tests pass for all three projects (chromium, mobile-chrome, mobile-safari).
  3. Run audit for anti-patterns to ensure no lingering page.screenshot calls remain in the codebase.
  4. Update snapshots if necessary.

Review automatically published via RepoAuditor.

google-labs-jules Bot and others added 3 commits June 25, 2026 16:06
…t configs (#2952)

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: Ariel Anders <anders.ariel@gmail.com>
@arii

arii commented Jun 25, 2026

Copy link
Copy Markdown
Owner

🤖 AI Technical Audit

ANTI-AI-SLOP

  • The PR is well-structured and addresses the core issues of flakiness in visual regression tests.
  • Cleanup of redundant test.use({ viewport: ... }) is appreciated.
  • snapshotPathTemplate is correctly implemented to prevent collision and aid maintenance.
  • The manual font waiting logic in the visual.ts fixture is a robust addition to avoid layout shift-related noise.

FINAL RECOMMENDATION

Approved with Minor Changes

DEFINITION OF DONE

  • Clean up orphaned snapshot files. Many renames left behind leftover files in the old directory structure that should be purged from the repository.
  • Add a note in playwright.config.ts or a README update explaining the new snapshot directory structure for future contributors.
  • Verify all snapshots are actually being generated in the correct folders (run tests locally to confirm output alignment).

Review automatically published via RepoAuditor.

@arii arii left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

PR Review

Summary: This PR aims to stabilize mobile visual snapshots by adopting native Playwright snapshot features (toHaveScreenshot) instead of manual file path string manipulations. It removes hardcoded viewports in favor of leveraging Playwright project configurations and adds some fixes for view-transition animations.

Findings:

  • CI Failure: The Build & E2E check failed. Looking at the diff, this PR introduces -linux.png snapshots (e.g., tests/verify_ux_consistency.spec.ts-snapshots/detail-page-v2-chromium-linux.png). This directly violates the repository context ("Playwright visual regression snapshots are structured using a custom snapshotPathTemplate... Default Playwright OS suffixes (like -linux.png) are obsolete and should be purged").
  • Flawed Snapshot Naming: The PR is actively adding OS-suffixed snapshots which likely broke the CI environment that doesn't expect them, and conflicts with the stated goal of using snapshotPathTemplate.
  • Refactoring: The shift to expect(page).toHaveScreenshot(name) is correct and an improvement over page.screenshot({ path: ... }).

Recommendation: Not Approved. The PR introduces obsolete -linux.png suffixed snapshots which violates the repository's Playwright configuration constraints and fails the CI. Please remove the OS suffixes from the snapshot names to rely on the project-level directory grouping.

@arii arii left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

ANTI-AI-SLOP

The Build & E2E check failed. This PR introduces -linux.png snapshots which directly violates the repository context ("Default Playwright OS suffixes (like -linux.png) are obsolete and should be purged").

FINDINGS

This PR aims to stabilize mobile visual snapshots by adopting native Playwright snapshot features (toHaveScreenshot). However, the PR introduces obsolete -linux.png suffixed snapshots which violates the repository's Playwright configuration constraints and fails the CI. Please remove the OS suffixes from the snapshot names to rely on the project-level directory grouping.

FINAL RECOMMENDATION

Not Approved

@arii arii left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

ANTI-AI-SLOP

The Build & E2E check failed. This PR introduces -linux.png snapshots which directly violates the repository context ("Default Playwright OS suffixes (like -linux.png) are obsolete and should be purged").

FINDINGS

This PR aims to stabilize mobile visual snapshots by adopting native Playwright snapshot features (toHaveScreenshot). However, the PR introduces obsolete -linux.png suffixed snapshots which violates the repository's Playwright configuration constraints and fails the CI. Please remove the OS suffixes from the snapshot names to rely on the project-level directory grouping.

FINAL RECOMMENDATION

Not Approved

@arii arii left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

ANTI-AI-SLOP

The Build & E2E check failed. This PR introduces -linux.png snapshots which directly violates the repository context ("Default Playwright OS suffixes (like -linux.png) are obsolete and should be purged").

FINDINGS

This PR aims to stabilize mobile visual snapshots by adopting native Playwright snapshot features (toHaveScreenshot). However, the PR introduces obsolete -linux.png suffixed snapshots which violates the repository's Playwright configuration constraints and fails the CI. Please remove the OS suffixes from the snapshot names to rely on the project-level directory grouping.

FINAL RECOMMENDATION

Not Approved

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate why mobile visual snapshots prompt unexpected updates when no changes exist

1 participant