[chore] override page.goto to run started logic (#3197)

CONTRIBUTING.md

@@ -51,9 +51,9 @@ You can run the tests for only a single package by first moving to that director
 
 You must rebuild each time before running the tests if you've made code changes.
 
-To run a single integration test, provide the `FILTER` env var with the test name. E.g. `FILTER="includes paths" pnpm test:integration`. You can also open up the file and change `test` to `test.only`.
+To run a single integration test or otherwise control the running of the tests locally see [the Playwright CLI docs](https://playwright.dev/docs/test-cli). Note that you will need to run these commands from the test project directory such as `packages/kit/test/apps/basics`.
 
-You can run the test server with `cd packages/kit/test/apps/basics; pnpm run dev` to hit it with your browser.
+You can run the test server with `cd packages/kit/test/apps/basics; pnpm run dev` to hit it with your browser. The Playwright Inspector offers similar functionality.
 
 You may need to install some dependencies first, e.g. with `npx playwright install-deps` (which only works on Ubuntu).
 

packages/kit/test/apps/amp/playwright.config.js

@@ -1 +1,11 @@
-export { config as default } from '../../utils.js';
+import { config } from '../../utils.js';
+
+// remove any projects with javaScriptEnabled
+const projects = config.projects || [];
+for (let i = projects.length - 1; i >= 0; i--) {
+	if (projects[i]?.use?.javaScriptEnabled) {
+		projects.splice(i, 1);
+	}
+}
+
+export default config;

packages/kit/test/apps/basics/test/test.js

@@ -89,10 +89,8 @@ test.describe.parallel('a11y', () => {
 });
 
 test.describe('Scrolling', () => {
-	// skip these tests if
-	// a) JS is disabled, since we're testing client-side behaviour, or
-	// b) we're in CI, because for unknown reasons the tests are flaky as hell there
-	test.skip(({ javaScriptEnabled }) => !javaScriptEnabled || !!process.env.CI);
+	// skip these tests if JS is disabled, since we're testing client-side behaviour
+	test.skip(({ javaScriptEnabled }) => !javaScriptEnabled);
 
 	test('url-supplied anchor works on direct page load', async ({ page, in_view }) => {
 		await page.goto('/anchor/anchor#go-to-element');
@@ -145,11 +143,9 @@ test.describe('Scrolling', () => {
 
 	test('url-supplied anchor is ignored with onMount() scrolling on direct page load', async ({
 		page,
-		in_view,
-		started
+		in_view
 	}) => {
 		await page.goto('/anchor-with-manual-scroll/anchor#go-to-element');
-		await started();
 		expect(await in_view('#abcde')).toBe(true);
 	});
 
@@ -1219,11 +1215,10 @@ test.describe.parallel('Redirects', () => {
 		);
 	});
 
-	test('redirect-on-load', async ({ baseURL, page, javaScriptEnabled, started }) => {
+	test('redirect-on-load', async ({ baseURL, page, javaScriptEnabled }) => {
 		await page.goto('/redirect-on-load');
 
 		if (javaScriptEnabled) {
-			await started();
 			await page.waitForTimeout(50); // TODO investigate why this test is flaky
 			expect(page.url()).toBe(`${baseURL}/redirect-on-load/redirected`);
 			expect(await page.textContent('h1')).toBe('Hazaa!');
@@ -1239,8 +1234,7 @@ test.describe.parallel('Routing', () => {
 		page,
 		clicknav,
 		app,
-		javaScriptEnabled,
-		started
+		javaScriptEnabled
 	}) => {
 		await page.goto('/routing/slashes');
 
@@ -1250,7 +1244,6 @@ test.describe.parallel('Routing', () => {
 
 		if (javaScriptEnabled) {
 			await page.goto(`${baseURL}/routing/slashes`);
-			await started();
 			await app.goto('/routing/');
 			expect(page.url()).toBe(`${baseURL}/routing`);
 			expect(await page.textContent('h1')).toBe('Great success!');
@@ -1262,8 +1255,7 @@ test.describe.parallel('Routing', () => {
 		page,
 		clicknav,
 		app,
-		javaScriptEnabled,
-		started
+		javaScriptEnabled
 	}) => {
 		await page.goto('/routing/slashes');
 
@@ -1273,7 +1265,6 @@ test.describe.parallel('Routing', () => {
 
 		if (javaScriptEnabled) {
 			await page.goto(`${baseURL}/routing/slashes`);
-			await started();
 			await app.goto('/routing/?');
 			expect(page.url()).toBe(`${baseURL}/routing`);
 			expect(await page.textContent('h1')).toBe('Great success!');
@@ -1285,8 +1276,7 @@ test.describe.parallel('Routing', () => {
 		page,
 		clicknav,
 		app,
-		javaScriptEnabled,
-		started
+		javaScriptEnabled
 	}) => {
 		await page.goto('/routing/slashes');
 
@@ -1296,7 +1286,6 @@ test.describe.parallel('Routing', () => {
 
 		if (javaScriptEnabled) {
 			await page.goto(`${baseURL}/routing/slashes`);
-			await started();
 			await app.goto('/routing/?foo=bar');
 			expect(page.url()).toBe(`${baseURL}/routing?foo=bar`);
 			expect(await page.textContent('h1')).toBe('Great success!');
@@ -1334,13 +1323,11 @@ test.describe.parallel('Routing', () => {
 		app,
 		page,
 		clicknav,
-		javaScriptEnabled,
-		started
+		javaScriptEnabled
 	}) => {
 		if (javaScriptEnabled) {
 			await page.goto('/routing');
 
-			await started();
 			await app.prefetchRoutes(['/routing/a']).catch((e) => {
 				// from error handler tests; ignore
 				if (!e.message.includes('Crashing now')) throw e;
@@ -1357,25 +1344,17 @@ test.describe.parallel('Routing', () => {
 		}
 	});
 
-	test('navigates programmatically', async ({ page, app, javaScriptEnabled, started }) => {
+	test('navigates programmatically', async ({ page, app, javaScriptEnabled }) => {
 		if (javaScriptEnabled) {
 			await page.goto('/routing/a');
-			await started();
 			await app.goto('/routing/b');
 			expect(await page.textContent('h1')).toBe('b');
 		}
 	});
 
-	test('prefetches programmatically', async ({
-		baseURL,
-		page,
-		app,
-		javaScriptEnabled,
-		started
-	}) => {
+	test('prefetches programmatically', async ({ baseURL, page, app, javaScriptEnabled }) => {
 		if (javaScriptEnabled) {
 			await page.goto('/routing/a');
-			await started();
 
 			/** @type {string[]} */
 			let requests = [];
@@ -1641,13 +1620,11 @@ test.describe.parallel('Shadow DOM', () => {
 		app,
 		page,
 		clicknav,
-		javaScriptEnabled,
-		started
+		javaScriptEnabled
 	}) => {
 		await page.goto('/routing/shadow-dom');
 
 		if (javaScriptEnabled) {
-			await started();
 			await app.prefetchRoutes(['/routing/a']).catch((e) => {
 				// from error handler tests; ignore
 				if (!e.message.includes('Crashing now')) throw e;

packages/kit/test/apps/options/test/test.js

@@ -4,9 +4,8 @@ import { test } from '../../../utils.js';
 /** @typedef {import('@playwright/test').Response} Response */
 
 test.describe.parallel('base path', () => {
-	test('serves /', async ({ page, started, javaScriptEnabled }) => {
+	test('serves /', async ({ page, javaScriptEnabled }) => {
 		await page.goto('/path-base/');
-		await started();
 
 		expect(await page.textContent('h1')).toBe('I am in the template');
 		expect(await page.textContent('h2')).toBe("We're on index.svelte");

packages/kit/test/utils.d.ts

@@ -21,7 +21,6 @@ export const test: TestType<
 			clicknav: (selector: string) => Promise<void>;
 			in_view: (selector: string) => Promise<boolean>;
 			read_errors: (href: string) => string;
-			started: () => Promise<void>;
 		},
 	PlaywrightWorkerArgs & PlaywrightWorkerOptions
 >;

packages/kit/test/utils.js

@@ -61,11 +61,9 @@ export const test = base.extend({
 	},
 
 	// @ts-expect-error
-	clicknav: async ({ page, javaScriptEnabled, started }, use) => {
+	clicknav: async ({ page, javaScriptEnabled }, use) => {
 		/** @param {string} selector */
 		async function clicknav(selector) {
-			await started();
-
 			if (javaScriptEnabled) {
 				await page.evaluate(() => {
 					window.navigated = new Promise((fulfil, reject) => {
@@ -113,22 +111,7 @@ export const test = base.extend({
 		use(in_view);
 	},
 
-	// @ts-expect-error
-	// eslint-disable-next-line
-	read_errors: ({}, use) => {
-		/** @param {string} path */
-		function read_errors(path) {
-			const errors =
-				fs.existsSync('test/errors.json') &&
-				JSON.parse(fs.readFileSync('test/errors.json', 'utf8'));
-			return errors[path];
-		}
-
-		use(read_errors);
-	},
-
-	// @ts-expect-error
-	started: async ({ page, javaScriptEnabled }, use) => {
+	page: async ({ page, javaScriptEnabled }, use) => {
 		if (javaScriptEnabled) {
 			page.addInitScript({
 				content: `
@@ -145,11 +128,34 @@ export const test = base.extend({
 			});
 		}
 
-		use(async () => {
-			if (javaScriptEnabled) {
-				await page.waitForFunction(() => window.started);
-			}
-		});
+		const goto = page.goto;
+		page.goto =
+			/**
+			 * @param {string} url
+			 * @param {object}	opts
+			 */
+			async function (url, opts) {
+				const res = await goto.call(page, url, opts);
+				if (javaScriptEnabled) {
+					await page.waitForFunction(() => window.started);
+				}
+				return res;
+			};
+		await use(page);
+	},
+
+	// @ts-expect-error
+	// eslint-disable-next-line
+	read_errors: ({}, use) => {
+		/** @param {string} path */
+		function read_errors(path) {
+			const errors =
+				fs.existsSync('test/errors.json') &&
+				JSON.parse(fs.readFileSync('test/errors.json', 'utf8'));
+			return errors[path];
+		}
+
+		use(read_errors);
 	}
 });
 
@@ -163,7 +169,7 @@ export const config = {
 		timeout: 15000 // AMP validator needs a long time to get moving
 	},
 	workers: 8,
-	retries: 3,
+	retries: process.env.CI ? 5 : 0,
 	projects: [
 		{
 			name: `${process.env.DEV ? 'dev' : 'build'}+js`,
@@ -179,7 +185,8 @@ export const config = {
 		}
 	],
 	use: {
-		screenshot: 'only-on-failure'
+		screenshot: 'only-on-failure',
+		trace: 'retain-on-failure'
 	}
 };