Swis 364/banner screenshot tests by roseb89 · Pull Request #617 · NYPL/nypl-library-card-app

roseb89 · 2026-03-20T21:48:29Z

Description

Added screenshot tests for the hero component banner for English and Urdu
Added a script to the package.json to use docker to create screenshots
Added Dockerfile.test
Added docker-compose-test.yml

Tickets:

SWIS-364

Motivation and Context

How Has This Been Tested?

Open Docker
Open a terminal and run npm run dev
Open another terminal and run npm run test:visual -- playwright/tests/visual-tests/banner.spec.ts

Command to run tests npm run test:visual -- playwright/tests/visual-tests/banner.spec.ts
Command to update screenshots pm run test:visual:update -- tests/banner.spec.ts --update-snapshots

Checklist:

I have updated the documentation accordingly.
All new and existing tests passed.

…hots

vercel · 2026-03-20T21:48:35Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
nypl-library-card-app	Ready	Preview, Comment	Mar 24, 2026 0:54am

Copilot

Pull request overview

Adds Playwright visual (screenshot) coverage for the landing page “banner” in English and Urdu, and wires up snapshot storage + a Docker-based script to generate/update those snapshots.

Changes:

Add a new Playwright visual test spec for the landing page banner (English + Urdu).
Commit baseline screenshots for Chromium/Firefox/WebKit.
Update Playwright config to customize snapshot paths and set a global screenshot diff tolerance; add a Docker script to generate snapshots.

Reviewed changes

Copilot reviewed 2 out of 9 changed files in this pull request and generated 4 comments.

File	Description
playwright/tests/visual-tests/banner.spec.ts	Introduces visual assertions for the landing page banner in English and Urdu.
playwright.config.ts	Configures global `toHaveScreenshot` diff tolerance and a custom snapshot output path template.
package.json	Adds a Docker-based script intended to generate/update banner visual snapshots.
playwright/screenshots/**	Adds baseline PNG snapshots for all 3 browser projects.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

playwright/tests/visual-tests/banner.spec.ts

package.json

roseb89 · 2026-03-20T22:19:57Z

Playwright test failed because of the pixel differences between my local and github actions. I will save the github actions screenshots and commit them locally for the tests to pass.

`Error: expect(locator).toHaveScreenshot(expected) failed

Locator: getByRole('heading', { name: 'Apply for a Library Card Online', level: 1 })
Expected an image 710px by 64px, received 758px by 126px. 54540 pixels (ratio 0.58 of all image pixels) are different.
`

russellcullen · 2026-03-20T22:53:16Z

Playwright test failed because of the pixel differences between my local and github actions. I will save the github actions screenshots and commit them locally for the tests to pass.

Im not sure that's the best approach, I think there should be a way to create and run screenshot tests so everyone can run these locally and they run in CI fine. Otherwise, it would be quite annoying to have to push code to github in order to test it.

Copilot

Pull request overview

Copilot reviewed 3 out of 10 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

playwright/tests/visual-tests/banner.spec.ts

package.json

… parameter

Copilot

Pull request overview

Copilot reviewed 3 out of 10 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-23T20:28:37Z

playwright/tests/visual-tests/banner.spec.ts

+    await expect(landingPage.heroBanner).toHaveScreenshot(
+      "landing-page-banner-english.png",
+      { maxDiffPixelRatio: 0.05 }
+    );


maxDiffPixelRatio is configured globally (playwright.config.ts) but these assertions override it with a different value (0.05). Having two tolerances makes the visual baseline harder to reason about and 5% can mask real regressions. Consider relying on the global default and only overriding per-test when there’s a documented reason (or align the per-test value with the global setting).

Copilot · 2026-03-23T20:28:37Z

playwright.config.ts

+
+    /* Standardize viewport and pixel density across local and CI environments */
+    viewport: { width: 1280, height: 720 },
+    deviceScaleFactor: 1,


The added viewport and deviceScaleFactor in the root use block are likely being overridden by the per-project use: { ...devices[...] } settings (those device presets include their own viewport/scale). As a result, this may not actually standardize rendering for screenshots across environments. Consider either (1) setting viewport/deviceScaleFactor explicitly inside each project’s use, or (2) not spreading devices[...] presets and instead set only browserName + the standardized viewport/scale in one place.

Suggested change

/* Standardize viewport and pixel density across local and CI environments */

viewport: { width: 1280, height: 720 },

deviceScaleFactor: 1,

Copilot

Pull request overview

Copilot reviewed 6 out of 13 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-24T11:55:08Z

docker-compose.test.yml

+  playwright:
+    build:
+      context: .
+      dockerfile: Dockerfile.test


The compose file references Dockerfile.test, but this PR adds DockerFile.test (capital F). On case-sensitive filesystems (most Linux/CI), Docker won’t find the file and the build will fail. Rename the file to Dockerfile.test (or update docker-compose.test.yml to match) so the path is consistent.

Suggested change

dockerfile: Dockerfile.test

dockerfile: DockerFile.test

Copilot · 2026-03-24T11:55:08Z

playwright.config.ts

+
+    /* Standardize viewport and pixel density across local and CI environments */
+    viewport: { width: 1280, height: 720 },
+    deviceScaleFactor: 1,


Viewport and deviceScaleFactor are set globally here and also repeated inside each project’s use block. This duplication increases maintenance and can cause confusion about which values are authoritative. Consider keeping these settings in only one place (global use if shared by all projects; per-project only if they differ).

Suggested change

/* Standardize viewport and pixel density across local and CI environments */

viewport: { width: 1280, height: 720 },

deviceScaleFactor: 1,

You told me to do it this way. I had it right before lol

I believe copilot is suggesting that we should only set viewport in a single spot. Right now it's currently configured here globally for all projects and then overriden down below for each project/browser configuration.

It doesn't appear that Copilot is actually suggesting one method over the other -- just that we should only set viewport once.

Copilot · 2026-03-24T11:55:09Z

playwright/pageobjects/landing.page.ts

    this.mainHeading = page.getByRole("heading", {
-      name: "Apply for a Library Card Online",
+      name: /Apply for a Library Card Online|للائبریری کارڈ کے لیے آن لائن اپلائی کریں/,
      level: 1,
    });


The page object’s mainHeading locator is now coupled to specific localized strings. This makes the page object less reusable and can create false positives if copy changes. A more maintainable approach is to locate the H1 structurally (e.g., role+level only) and assert the exact text per test (English vs Urdu) in the spec.

Copilot · 2026-03-24T11:55:09Z

run-tests.sh

+#!/bin/bash
+
+# 1. Extract Playwright version from package.json
+PW_VERSION=$(node -p "require('./package.json').devDependencies['@playwright/test'].replace(/[^0-9.]/g, '')")


This script would be more reliable if it failed fast and cleaned up resources: add set -euo pipefail and consider using a trap to run docker-compose ... down on exit to avoid leaving containers/networks around after failures. Also, docker-compose is deprecated in many environments in favor of docker compose (Compose v2), so switching improves compatibility.

Copilot · 2026-03-24T11:55:09Z

run-tests.sh

+# 1. Extract Playwright version from package.json
+PW_VERSION=$(node -p "require('./package.json').devDependencies['@playwright/test'].replace(/[^0-9.]/g, '')")
+
+echo "🚀 Detected Playwright v$PW_VERSION. Starting Docker environment..."
+
+# 2. Run the tests using the matching Docker image
+# We use 'up --build' to ensure any code changes are reflected
+PW_VERSION=$PW_VERSION docker-compose -f docker-compose.test.yml up --build --exit-code-from playwright


This script would be more reliable if it failed fast and cleaned up resources: add set -euo pipefail and consider using a trap to run docker-compose ... down on exit to avoid leaving containers/networks around after failures. Also, docker-compose is deprecated in many environments in favor of docker compose (Compose v2), so switching improves compatibility.

Suggested change

# 1. Extract Playwright version from package.json

PW_VERSION=$(node -p "require('./package.json').devDependencies['@playwright/test'].replace(/[^0-9.]/g, '')")

echo "🚀 Detected Playwright v$PW_VERSION. Starting Docker environment..."

# 2. Run the tests using the matching Docker image

# We use 'up --build' to ensure any code changes are reflected

PW_VERSION=$PW_VERSION docker-compose -f docker-compose.test.yml up --build --exit-code-from playwright

set -euo pipefail

# 1. Extract Playwright version from package.json

PW_VERSION=$(node -p "require('./package.json').devDependencies['@playwright/test'].replace(/[^0-9.]/g, '')")

echo "🚀 Detected Playwright v$PW_VERSION. Starting Docker environment..."

# Ensure Docker resources are cleaned up on exit, even on failure

trap 'docker compose -f docker-compose.test.yml down || true' EXIT

# 2. Run the tests using the matching Docker image

# We use 'up --build' to ensure any code changes are reflected

PW_VERSION=$PW_VERSION docker compose -f docker-compose.test.yml up --build --exit-code-from playwright

roseb89 added 2 commits March 20, 2026 16:32

banner screenshot tests

85f79bc

added banner screenshot tests and docker script to save/store screens…

eaf56ee

…hots

roseb89 requested a review from Copilot March 20, 2026 21:48

Copilot started reviewing on behalf of roseb89 March 20, 2026 21:48 View session

roseb89 requested review from Toxiapo, clarissarichard, milliesavalia and russellcullen March 20, 2026 21:49

Copilot AI reviewed Mar 20, 2026

View reviewed changes

playwright/tests/visual-tests/banner.spec.ts Show resolved Hide resolved

playwright/tests/visual-tests/banner.spec.ts Show resolved Hide resolved

playwright/tests/visual-tests/banner.spec.ts Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

roseb89 marked this pull request as draft March 20, 2026 21:55

roseb89 marked this pull request as ready for review March 20, 2026 21:57

roseb89 marked this pull request as draft March 20, 2026 22:01

russellcullen reviewed Mar 20, 2026

View reviewed changes

package.json Outdated Show resolved Hide resolved

roseb89 added 3 commits March 23, 2026 14:18

update screenshot test with standard viewport for all browsers

6db7815

update landing page mainHeading nand heroBanner

65159b9

updated screenshot tests with the standard viewport

1eb594d

vercel bot deployed to Preview March 23, 2026 19:31 View deployment

roseb89 requested a review from Copilot March 23, 2026 19:45

Copilot started reviewing on behalf of roseb89 March 23, 2026 19:45 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

playwright/tests/visual-tests/banner.spec.ts Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

roseb89 added 3 commits March 23, 2026 16:19

delete old screenshots, add networkirdle, and update script and query…

94f22c4

… parameter

delete old screenshots and upate script

012dc7d

update screenshot

219ac87

vercel bot deployed to Preview March 23, 2026 20:23 View deployment

roseb89 requested a review from Copilot March 23, 2026 20:23

Copilot started reviewing on behalf of roseb89 March 23, 2026 20:23 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

roseb89 added 3 commits March 24, 2026 07:37

docker compose test, run-test.sh, updated banner test and package.json

8a21ca5

deleted old screenshots

71a4c41

new screenshots

f6f5906

roseb89 requested a review from Copilot March 24, 2026 11:51

Copilot started reviewing on behalf of roseb89 March 24, 2026 11:52 View session

vercel bot deployed to Preview March 24, 2026 11:52 View deployment

Copilot AI reviewed Mar 24, 2026

View reviewed changes

roseb89 added 3 commits March 24, 2026 08:37

update playwright config and playwright-tests.yml, rename Dockerfile

a287e82

delete old screenshots

0dc8f28

update package.json and add new screenshots

deac787

vercel bot deployed to Preview March 24, 2026 12:54 View deployment

Conversation

roseb89 commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Checklist:

Uh oh!

vercel bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roseb89 commented Mar 20, 2026

Uh oh!

russellcullen commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

roseb89 Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

russellcullen Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

roseb89 commented Mar 20, 2026 •

edited

Loading

vercel bot commented Mar 20, 2026 •

edited

Loading

russellcullen commented Mar 20, 2026 •

edited

Loading

russellcullen Mar 24, 2026 •

edited

Loading