Skip to content
Closed
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions .github/workflows/e2e-checks.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
name: E2E Code Quality Checks
run-name: E2E Code Quality Checks

on:
pull_request:
branches: [ main ]
paths:
- 'e2e/**'
push:
branches: [ main ]
paths:
- 'e2e/**'

jobs:
quality:
name: E2E Code Quality
runs-on: ubuntu-latest

steps:
- name: Checkout code
uses: actions/checkout@v4

- name: Set up Node.js
uses: actions/setup-node@v4
with:
node-version: lts/*

- name: Cache Node.js dependencies
id: cache-node
uses: actions/cache@v4
with:
path: e2e/node_modules
key: ${{ runner.os }}-node-${{ hashFiles('package-lock.json') }}

- name: Install dependencies
if: steps.cache-node.outputs.cache-hit != 'true'
run: make e2e-install-ci

- name: Check formatting
run: make e2e-format-check-native
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think we got to discussing the where or how this should run - we can go back to the google docs if necessary. I recently started a section about it there

23 changes: 23 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ __check_defined = \
e2e-clean \
e2e-clean-image \
e2e-clean-report \
e2e-format \
e2e-format-check \
e2e-format-check-native \
e2e-format-native \
e2e-install-ci-native \
e2e-merge-reports \
e2e-setup-ci \
e2e-setup-native \
Expand Down Expand Up @@ -81,6 +86,9 @@ __check_defined = \
# The e2e test image includes the test suite for all apps and therefore isn't specific to each app.
E2E_IMAGE_NAME := $(PROJECT_ROOT)-e2e

# Define Node.js Docker image to use for e2e commands
E2E_NODE_IMAGE := node:22-alpine
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.

Let's just use one image for e2e stuff not a separate one

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Combining your two suggestions above - we can use our existing ./e2e/Dockerfile - but I believe we need to override the ENTRYPOINT - this worked for me with the --entrypoint flag:

e2e-format-check: e2e-build ## Format check without autofix inside Docker
	docker run --rm -v $(PWD)/e2e:/e2e --entrypoint sh $(E2E_IMAGE_NAME) -c "npm run e2e-format:check"

The ./e2e/Dockerfile was written just to run tests (npm run e2e-test) and its using the playwright image - which has a bunch of extra cross-browser testing libraries that aren't necessarily needed for checks. That's why I was thinking of using a separate simple node image to run checks.

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.

I'd be ok getting rid of ENTRYPOINT if that means we can simplify things.

Does it make the checks a lot slower to combine everything? I get the performance benefit but unless there's a significant different I don't really want to overoptimize.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I didn't see any slowdowns when we got rid of ENTRYPOINT

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.

getting rid of ENTRYPOINT isn't going to make things slower, I was referring to your comment:

The ./e2e/Dockerfile was written just to run tests (npm run e2e-test) and its using the playwright image - which has a bunch of extra cross-browser testing libraries that aren't necessarily needed for checks. That's why I was thinking of using a separate simple node image to run checks.

combining into one image means it'll be a bigger image that'll take a bit longer to build


e2e-build: ## Build the e2e Docker image, if not already built, using ./e2e/Dockerfile
docker build -t $(E2E_IMAGE_NAME) -f ./e2e/Dockerfile .

Expand All @@ -95,6 +103,21 @@ e2e-clean-report: ## Remove the local e2e report folders and content
rm -rf ./e2e/blob-report
rm -rf ./e2e/test-results

e2e-format: ## Format code with autofix inside Docker
docker run --rm -v $(PWD)/e2e:/e2e $(E2E_NODE_IMAGE) sh -c "cd /e2e && npm run e2e-format"
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.

Is there a way to get rid of cd /e2e? Would setting WORKDIR /e2e in the Dockerfile help?

Copy link
Copy Markdown
Contributor Author

@rylew1 rylew1 Feb 24, 2025

Choose a reason for hiding this comment

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

Reference comment #875 (comment)


e2e-format-check: ## Format check without autofix inside Docker
docker run --rm -v $(PWD)/e2e:/e2e $(E2E_NODE_IMAGE) sh -c "cd /e2e && npm run e2e-format:check"

e2e-format-check-native: ## Format check without autofix natively
cd e2e && npm run e2e-format:check

e2e-format-native: ## Format code with autofix natively
cd e2e && npm run e2e-format

e2e-install-ci: ## CI install dependencies
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.

confusing that we have both e2e-install-ci and e2e-setup-ci can we combine them

Copy link
Copy Markdown
Contributor Author

@rylew1 rylew1 Feb 24, 2025

Choose a reason for hiding this comment

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

This issue is somewhat related to the comment above - I wanted a separate non-playwright install command for checks so i added e2e-install-ci.
If we wanted to again adapt our current setup - something like this might work:

e2e-setup-ci: ## Setup end-to-end tests for CI
	cd e2e && npm ci
	cd e2e && npm run e2e-setup

But this would be unnecessarily installing playwright for code quality checks - and unnecessarily running an extra npm ci for our create-report GHA job

cd e2e && npm ci

e2e-merge-reports: ## Merge E2E blob reports from multiple shards into an HTML report
cd e2e && npm run e2e-merge-reports

Expand Down
1 change: 1 addition & 0 deletions e2e/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ node_modules/
/blob-report/
/playwright/.cache/
*.png*
.prettier-cache
1 change: 1 addition & 0 deletions e2e/.prettierignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
*-lock.json
7 changes: 7 additions & 0 deletions e2e/.prettierrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"semi": true,
"singleQuote": true,
"tabWidth": 2,
"trailingComma": "es5",
"printWidth": 100
}
18 changes: 17 additions & 1 deletion e2e/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

39 changes: 21 additions & 18 deletions e2e/package.json
Original file line number Diff line number Diff line change
@@ -1,20 +1,23 @@
{
"name": "e2e",
"version": "1.0.0",
"scripts": {
"e2e-merge-reports": "npx playwright merge-reports --reporter html blob-report",
"e2e-setup": "npx playwright install --with-deps",
"e2e-show-report": "npx playwright show-report",
"e2e-test": "./run-e2e-test",
"e2e-test:ui": "./run-e2e-test --ui"
},
"devDependencies": {
"@playwright/test": "^1.49.0",
"@types/node": "^22.10.1"
},
"dependencies": {
"@axe-core/playwright": "^4.10.1",
"dotenv": "^16.4.7",
"playwright": "^1.49.0"
}
"name": "e2e",
"version": "1.0.0",
"scripts": {
"e2e-format": "prettier --cache --cache-location='./.prettier-cache' --write '**/*.{js,json,md,mdx,ts,tsx,scss,yaml,yml}'",
"e2e-format:check": "prettier --cache --cache-location='./.prettier-cache' --check '**/*.{js,json,md,mdx,ts,tsx,scss,yaml,yml}'",
"e2e-merge-reports": "npx playwright merge-reports --reporter html blob-report",
"e2e-setup": "npx playwright install --with-deps",
"e2e-show-report": "npx playwright show-report",
"e2e-test": "./run-e2e-test",
"e2e-test:ui": "./run-e2e-test --ui"
},
"devDependencies": {
"@playwright/test": "^1.49.0",
"@types/node": "^22.10.1",
"prettier": "^3.5.1"
},
"dependencies": {
"@axe-core/playwright": "^4.10.1",
"dotenv": "^16.4.7",
"playwright": "^1.49.0"
}
}
27 changes: 13 additions & 14 deletions e2e/playwright.config.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// Load environment variables from .env file if it exists
import * as dotenv from 'dotenv';

import { defineConfig, devices } from "@playwright/test";
import { defineConfig, devices } from '@playwright/test';

dotenv.config();

Expand All @@ -11,7 +11,7 @@ dotenv.config();
export default defineConfig({
// Timeout for each test in milliseconds
timeout: 20000,
testDir: "./tests", // Ensure this points to the correct test directory
testDir: './tests', // Ensure this points to the correct test directory
// Run tests in files in parallel
fullyParallel: true,
// Fail the build on CI if you accidentally left test.only in the source code.
Expand All @@ -22,19 +22,19 @@ export default defineConfig({
workers: process.env.CI ? 1 : undefined,
// Use 'blob' for CI to allow merging of reports. See https://playwright.dev/docs/test-reporters
reporter: process.env.CI
? [['blob']] :
// Don't open the HTML report since it hangs when running inside a container.
// Use make e2e-show-report for opening the HTML report
[['html', { open: 'never' }]],
? [['blob']]
: // Don't open the HTML report since it hangs when running inside a container.
// Use make e2e-show-report for opening the HTML report
[['html', { open: 'never' }]],
// Shared settings for all the projects below. See https://playwright.dev/docs/api/class-testoptions.
use: {
// Base URL to use in actions like `await page.goto('/')`.
baseURL: process.env.BASE_URL,

// Collect trace when retrying the failed test. See https://playwright.dev/docs/trace-viewer
trace: "on-first-retry",
screenshot: "on",
video: "on-first-retry",
trace: 'on-first-retry',
screenshot: 'on',
video: 'on-first-retry',
},
// Splits tests into chunks for distributed parallel execution
shard: {
Expand All @@ -48,15 +48,14 @@ export default defineConfig({
// Supported browsers: https://playwright.dev/docs/browsers#:~:text=Configure%20Browsers%E2%80%8B,Google%20Chrome%20and%20Microsoft%20Edge.
projects: [
{
name: "chromium",
use: { ...devices["Desktop Chrome"] },
name: 'chromium',
use: { ...devices['Desktop Chrome'] },
},

// Test against mobile viewports.
{
name: "Mobile Chrome",
use: { ...devices["Pixel 7"] },
name: 'Mobile Chrome',
use: { ...devices['Pixel 7'] },
},
],

});
20 changes: 10 additions & 10 deletions e2e/util.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
// Merge a base and derived config
export function deepMerge(obj1, obj2) {
const result = { ...obj1 };
const result = { ...obj1 };

for (let key in obj2) {
if (obj2.hasOwnProperty(key)) {
if (obj2[key] instanceof Object && obj1[key] instanceof Object) {
result[key] = deepMerge(obj1[key], obj2[key]);
} else {
result[key] = obj2[key];
}
for (let key in obj2) {
if (obj2.hasOwnProperty(key)) {
if (obj2[key] instanceof Object && obj1[key] instanceof Object) {
result[key] = deepMerge(obj1[key], obj2[key]);
} else {
result[key] = obj2[key];
}
}

return result;
}

return result;
}
1 change: 0 additions & 1 deletion e2e/{{app_name}}/tests/index.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,4 @@ test.describe('Generic Webpage Tests', () => {
// const element = page.locator('h1');
// await expect(element).toBeVisible();
// });

});