[Bug]: `toHaveScreenshot` fails in Github workflow

[AntonioVentilii]

Version

^1.52.0

Steps to reproduce

A simple Github workflow that runs some E2E tests, inside of which there is

expect(...). toHaveScreenshot()

Complete CI here: https://github.com/dfinity/oisy-wallet/actions/runs/15471499824/job/43556862674

Expected behavior

I expect it to create a screenshot at the first try, without errors.

Actual behavior

It fails with error:

TypeError: The "data" argument must be of type string or an instance of Buffer, TypedArray, or DataView. Received undefined
    at Object.writeFileSync (node:fs:2435:5)
    at writeFileSync (/home/runner/work/oisy-wallet/oisy-wallet/node_modules/playwright/lib/matchers/toMatchSnapshot.js:333:21)
    at writeFiles (/home/runner/work/oisy-wallet/oisy-wallet/node_modules/playwright/lib/matchers/toMatchSnapshot.js:314:5)
    at Object.toHaveScreenshot (/home/runner/work/oisy-wallet/oisy-wallet/node_modules/playwright/lib/matchers/toMatchSnapshot.js:327:12)
    at HomepageLoggedIn.Homepage.takeScreenshotWithRetry (file:///home/runner/work/oisy-wallet/oisy-wallet/e2e/utils/pages/homepage.page.ts:497:5)
    at HomepageLoggedIn.takeScreenshot (file:///home/runner/work/oisy-wallet/oisy-wallet/e2e/utils/pages/homepage.page.ts:543:4)
    at file:///home/runner/work/oisy-wallet/oisy-wallet/e2e/homepage.spec.ts:22:2
    at /home/runner/work/oisy-wallet/oisy-wallet/node_modules/playwright/lib/worker/workerMain.js:304:9
    at /home/runner/work/oisy-wallet/oisy-wallet/node_modules/playwright/lib/worker/testInfo.js:277:11
    at TimeoutManager.withRunnable (/home/runner/work/oisy-wallet/oisy-wallet/node_modules/playwright/lib/worker/timeoutManager.js:67:14)
    at TestInfoImpl._runWithTimeout (/home/runner/work/oisy-wallet/oisy-wallet/node_modules/playwright/lib/worker/testInfo.js:275:7)
    at /home/runner/work/oisy-wallet/oisy-wallet/node_modules/playwright/lib/worker/workerMain.js:302:7
    at WorkerMain._runTest (/home/runner/work/oisy-wallet/oisy-wallet/node_modules/playwright/lib/worker/workerMain.js:277:5)
    at WorkerMain.runTestGroup (/home/runner/work/oisy-wallet/oisy-wallet/node_modules/playwright/lib/worker/workerMain.js:193:11)
    at process.<anonymous> (/home/runner/work/oisy-wallet/oisy-wallet/node_modules/playwright/lib/common/process.js:70:22) {
...

Additional context

• It is completely random: some tests raises it, some others no. it is not test dependent (already checked).
• No issue running it locally.
• The element being snapshot is not undefined (already checked).
• The element being snapshot is either Page or Locator.
• Workaround: if we retry it does not raise an error (at the 3rd try or something similar).

Environment

It happens only in CI Github action with ubuntu-24.04

[agg23]

It looks like there could be a real issue here, but we need a minimal repro that we can run to see the issue directly. Your codebase has too much indirection to tell what's going on, and the stack traces seem to be inaccurate. I couldn't figure out how takeScreenshotWithRetry is defined (I'm assuming it's injected based off of takeScreenshot).

[AntonioVentilii]

@agg23 thank you for the answer! I wasn't able to create a reproducible build just for a lack of time, sorry: it requires a new webpage, E2E tests, Github actions, and so on... basically my repo, but cleaned of a lot of noise.

In my opinion, the problem is there: if you look at the stack I pasted, the error is raised when it tries to write the file of the screenshot. I mean, the element to be screenshot exists, everything work, but the writeFileSync function fails.

at Object.writeFileSync (node:fs:2435:5)
at writeFileSync (/home/runner/work/oisy-wallet/oisy-wallet/node_modules/playwright/lib/matchers/toMatchSnapshot.js:333:21)

I don't know if it useful, but the screenshot already exists, it just needs to overwrite it.

What are the cases when this could fail? No permission on the existing file (because locally it works)?

[agg23]

I agree with you that it's our writeFileSync call, but without more information it's difficult to tell what is different about it. That's why I was looking for the takeScreenshotWithRetry function, to see if it was doing anything weird.

[AntonioVentilii]

Ah! Here it is

private takeScreenshotWithRetry = async (element: Page | Locator) => {
		let attempts = 0;
		const maxAttempts = 5;

		while (attempts < maxAttempts) {
			try {
				await expect(element).toHaveScreenshot();

				return;
			} catch (err: unknown) {
				console.warn(`Attempt ${attempts + 1} failed:`, err);
				attempts++;
				await this.#page.waitForTimeout(5000);
			}
		}
	};

[AntonioVentilii]

just an update: it really seems that the issue is when the screenshot already exists. If i remove the existing screenshot file, it works correctly. But I don't understand why, since i am using the flag --update-snapshots=changed

npm run e2e:ci -- --shard=${{ matrix.shardIndex }}/${{ matrix.shardTotal }}

where

"e2e:ci": "npm run playwright:install && playwright test --update-snapshots=changed --reporter=html,list"

and these are my playwright configs

import { notEmptyString } from '@dfinity/utils';
import { defineConfig, devices } from '@playwright/test';
import dotenv, { type DotenvPopulateInput } from 'dotenv';
import { join } from 'node:path';
import { readCanisterIds } from './env.utils';

dotenv.config({
	path: join(process.cwd(), '.env.e2e')
});

dotenv.populate(
	process.env as DotenvPopulateInput,
	readCanisterIds({
		filePath: join(process.cwd(), 'canister_e2e_ids.json'),
		prefix: 'E2E_'
	})
);

const DEV = (process.env.NODE_ENV ?? 'production') === 'development';

const MATRIX_OS = process.env.MATRIX_OS ?? '';
const isMac = notEmptyString(MATRIX_OS)
	? MATRIX_OS.includes('macos')
	: process.platform === 'darwin';

const appleProjects = [
	{
		name: 'Safari',
		use: devices['Desktop Safari']
	},
	{
		name: 'Google Chrome',
		use: devices['Desktop Chrome']
	},
	{
		name: 'iPhone SE',
		use: {
			...devices['iPhone SE'],
			screen: { width: 375, height: 667 },
			viewport: { width: 375, height: 667 }
		}
	},
	{
		name: 'iPad Pro 11',
		use: {
			...devices['iPad Pro 11'],
			screen: { width: 633, height: 1194 },
			viewport: { width: 633, height: 1194 }
		}
	}
];

const nonAppleProjects = [
	{
		name: 'Google Chrome',
		use: devices['Desktop Chrome']
	},
	{
		name: 'Firefox',
		use: devices['Desktop Firefox']
	},
	{
		name: 'Pixel 7',
		use: {
			...devices['Pixel 7'],
			screen: { width: 412, height: 915 },
			viewport: { width: 412, height: 915 }
		}
	}
];

const TIMEOUT = 5 * 60 * 1000;

export default defineConfig({
	retries: 3,
	timeout: TIMEOUT,
	workers: 5,
	expect: {
		toHaveScreenshot: {
			threshold: 0.3,
			// disable any animations caught by playwright for better screenshots and less flaky tests
			animations: 'disabled',
			// hide caret for cleaner snapshots
			caret: 'hide',
			// apply masks to hide flaky elements
			stylePath: 'e2e/styles/masks.css'
		}
	},
	webServer: {
		command: DEV ? 'npm run dev' : 'npm run build && npm run preview',
		reuseExistingServer: true,
		port: DEV ? 5173 : 4173,
		timeout: TIMEOUT
	},
	testDir: 'e2e',
	testMatch: ['**/*.e2e.ts', '**/*.spec.ts'],
	snapshotDir: 'e2e/snapshots',
	use: {
		testIdAttribute: 'data-tid',
		trace: 'on',
		actionTimeout: TIMEOUT,
		navigationTimeout: TIMEOUT,
		...(DEV && { headless: false })
	},
	projects: isMac ? appleProjects : nonAppleProjects
});

[AntonioVentilii]

@dgozman hi! I see that you tagged it with v1.54 but supposedly the next version would be v1.53.

Are you planning on skipping @agg23 's fix for the next release, or do you skip the version semantically?

[dgozman]

@AntonioVentilii The fix is not ready, so it will not make it to v1.53 release in time. Hopefully, next release will have the proper fix.

[AntonioVentilii]

@dgozman Thank you for the insight! Looking forward to it!

In the meanwhile, is there something we can do on our side? Maybe revert it to an older version? Or changing the version of some sub-dependency? What do you suggest? Asking because we are a bit of an impasse now

[dgozman]

@AntonioVentilii I am not really sure. The actual reason is still unknown, but it is probably related to the timing. Perhaps try adding some expect().toBeVisible() call before the failing toHaveScreenshot()? Unfortunately, I cannot reproduce, so you should experiment on your own.

[AntonioVentilii]

@agg23 @dgozman I don't know if useful: I wrapped .toHaveScreenshot() in a try-catch statement, ignoring the errors. The E2E tests proceed smoothly and the screenshots are created/updated. However, some of them (very randomly) are partially created:

[agg23]

@AntonioVentilii it would be very helpful if you could provide a minimal repro. While the crash is obvious (thus my PR), we really need to understand why this is happening in the first place before we check in a fix. Ideally it would be a single test and config file in a ready to go GitHub repo, so I can check it out and run it to see the failing scenario.

[agg23]

Closing due to lack of concrete repro. Please feel free to open a new issue if you find some minimal reproduction of this issue.