Skip to content

Commit 7c024be

Browse files
cwillisfcwillisf
committed
fix: address PR feedback and aggregate test reporting
- Use `url.pathToFileURL` for cross-platform `file://` URLs - Capture `err.stack` instead of `err.message` in `pageerror` handler - Aggregate test results into one PR comment instead of per-package - Trim test job permissions since reporting moved to a separate job - Fix test reporting path filter
1 parent d84a710 commit 7c024be

4 files changed

Lines changed: 45 additions & 78 deletions

File tree

‎.github/actions/test-package/action.yml‎

Lines changed: 0 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -28,25 +28,6 @@ runs:
2828
run: |
2929
mkdir -p test-results
3030
npm run test
31-
- name: Publish test reports
32-
if: ${{ !cancelled() }}
33-
uses: EnricoMi/publish-unit-test-result-action@34d7c956a59aed1bfebf31df77b8de55db9bbaaf # v2
34-
with:
35-
# Exclude Playwright's HTML report and per-test artifact
36-
# directories. Without these exclusions the action would try to
37-
# parse HTML, CSS, JS, PNG, JSON, etc. as test result files.
38-
#
39-
# We exclude by directory rather than by file extension because
40-
# the action supports JSON test results (Dart, Mocha) and
41-
# Playwright also writes .json files (e.g. .last-run.json) —
42-
# extension-based exclusion would either break JSON test report
43-
# support or let Playwright metadata through to the parser.
44-
files: |
45-
./packages/${{ inputs.package_name }}/test-results/**/*
46-
!./packages/${{ inputs.package_name }}/test-results/playwright-html/**
47-
!./packages/${{ inputs.package_name }}/test-results/playwright-artifacts/**
48-
check_name: "Test report for ${{ inputs.package_name }}"
49-
test_file_prefix: "+packages/${{ inputs.package_name }}/"
5031
- name: Store Test Results
5132
if: ${{ !cancelled() }}
5233
uses: actions/upload-artifact@330a01c490aca151604b8cf639adc76d48f6c5d4 # v5

‎.github/workflows/test-packages.yml‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,7 @@ jobs:
1212
test:
1313
runs-on: ubuntu-latest
1414
permissions:
15-
checks: write
16-
pull-requests: write
1715
contents: read
18-
issues: read
1916
strategy:
2017
fail-fast: false
2118
matrix:
@@ -41,8 +38,31 @@ jobs:
4138
runs-on: ubuntu-latest
4239
needs: test
4340
if: ${{ !cancelled() }}
41+
permissions:
42+
checks: write
43+
pull-requests: write
44+
contents: read
45+
issues: read
4446
steps:
45-
- run: |
47+
- uses: actions/download-artifact@018cc2cf5baa6db3ef3c5f8a56943fffe632ef53 # v6
48+
with:
49+
# Download every per-package test-output artifact. Each lands
50+
# in its own subdirectory (e.g. scratch-gui-test-output/),
51+
# which avoids filename collisions between packages that both
52+
# produce junit.xml.
53+
pattern: '*-test-output'
54+
path: test-results
55+
- name: Publish test reports
56+
uses: EnricoMi/publish-unit-test-result-action@34d7c956a59aed1bfebf31df77b8de55db9bbaaf # v2
57+
with:
58+
# Every test reporter in this repo produces JUnit XML.
59+
# Restricting to *.xml avoids the action trying to parse
60+
# directories and non-test artifacts (Playwright HTML
61+
# reports, traces, screenshots, etc.) as test result files.
62+
files: test-results/**/*.xml
63+
check_name: "Test Results"
64+
- name: Check test outcome
65+
run: |
4666
case "${{ needs.test.result }}" in
4767
success)
4868
echo "Tests passed successfully."

‎packages/scratch-gui/playwright.config.js‎

Lines changed: 13 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,23 @@
11
// @ts-check
22
const path = require('path');
3+
const {pathToFileURL} = require('url');
34
const {defineConfig, devices} = require('@playwright/test');
45

56
/**
67
* Playwright configuration for scratch-gui browser tests.
78
*
8-
* These tests run in both Chromium and Firefox so cross-browser issues
9-
* (e.g., differences in how each browser interprets pointer event spec
10-
* compatibility-mouse-event suppression) are caught in CI rather than
11-
* being noticed by users on staging.
9+
* Tests run in both Chromium and Firefox so cross-browser issues
10+
* (e.g., pointer event compatibility differences) are caught in CI
11+
* rather than first noticed on staging.
1212
*
13-
* Tests load the built playground from `build/` via a `file://` URL,
14-
* mirroring the existing Selenium-based integration tests in
15-
* `test/integration/`. CI builds scratch-gui before running the test
16-
* job, so the build artifact is always present when this config runs.
17-
* Locally, run `npm run build` in scratch-gui first.
18-
*
19-
* Test files reference pages relative to the build directory via
20-
* Playwright's `baseURL`, e.g. `await page.goto('index.html')`.
13+
* Tests reference pages relative to the build directory via baseURL,
14+
* e.g. `await page.goto('index.html')`. Locally, run `npm run build`
15+
* first.
2116
*/
2217
module.exports = defineConfig({
2318
testDir: './test/playwright',
2419

25-
// Tests in this suite are isolated and stateless, so they can
26-
// safely run in parallel.
20+
// Tests in this suite are (must be) isolated and stateless, so they can safely run in parallel.
2721
fullyParallel: true,
2822

2923
// Fail the build if a developer accidentally commits `test.only`.
@@ -32,16 +26,8 @@ module.exports = defineConfig({
3226
// CI gets a single retry for flake tolerance; locally, fail fast.
3327
retries: process.env.CI ? 1 : 0,
3428

35-
// Keep all Playwright output under the same `test-results/`
36-
// directory the rest of scratch-gui's test pipeline writes to,
37-
// so the CI publish-unit-test-result-action and upload-artifact
38-
// steps pick everything up with their existing globs.
39-
//
40-
// The HTML report has to live in its own subdirectory because
41-
// the HTML reporter clears its output folder on each run, and
42-
// Playwright refuses to point it at the same folder as
43-
// `outputDir` (which holds per-test artifacts like traces and
44-
// screenshots).
29+
// Playwright requires outputDir and the HTML reporter folder to be
30+
// separate directories, so both live as siblings under test-results/.
4531
outputDir: 'test-results/playwright-artifacts',
4632
reporter: [
4733
['list'],
@@ -50,10 +36,9 @@ module.exports = defineConfig({
5036
],
5137

5238
use: {
53-
// Resolve relative `page.goto()` URLs against the built
54-
// playground so individual tests don't need to compute paths
55-
// relative to their own location on disk.
56-
baseURL: `file://${path.resolve(__dirname, 'build')}/`,
39+
// Resolve relative `page.goto()` URLs against the playground
40+
// build output in `build/`.
41+
baseURL: `${pathToFileURL(path.resolve(__dirname, 'build'))}/`,
5742

5843
// Capture a trace on the first retry so CI failures are
5944
// debuggable without re-running tests locally.

‎packages/scratch-gui/test/playwright/editor-loads.spec.js‎

Lines changed: 8 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,33 +1,14 @@
11
// @ts-check
22
const {test, expect} = require('@playwright/test');
33

4-
// `baseURL` in playwright.config.js points at the built playground
5-
// (`packages/scratch-gui/build/`), so relative URLs here resolve
6-
// against that directory. CI downloads the build artifact before
7-
// running this suite; locally, run `npm run build` in scratch-gui
8-
// first.
4+
test('the editor loads and the Backpack header is visible', async ({page}) => {
5+
// Capture uncaught page exceptions so a broken bundle fails with
6+
// the actual error rather than a generic visibility timeout.
7+
const pageErrors = [];
8+
page.on('pageerror', err => pageErrors.push(err.stack || err.message || String(err)));
99

10-
test.describe('editor smoke test', () => {
11-
test('the editor loads and the Backpack header is visible', async ({page}) => {
12-
// Capture uncaught exceptions so a broken bundle fails loudly with
13-
// the underlying error instead of silently timing out on the
14-
// visibility assertion below. Note: we deliberately do NOT capture
15-
// `console.error` events. The current scratch-gui playground emits
16-
// a flood of React `defaultProps` deprecation warnings on mount,
17-
// which would drown out any real signal. Uncaught exceptions
18-
// (`pageerror`) are the right granularity for catching things
19-
// like a bundle that fails to evaluate at all.
20-
const pageErrors = [];
21-
page.on('pageerror', err => pageErrors.push(err.message));
10+
await page.goto('index.html');
2211

23-
await page.goto('index.html');
24-
25-
// The Backpack header is always rendered in the playground build,
26-
// even when no backpack host is configured (it just gets wrapped in
27-
// a "Coming Soon" tooltip). It is a stable, user-visible marker
28-
// that the editor mounted and rendered the menu bar / footer area.
29-
await expect(page.getByText('Backpack', {exact: true})).toBeVisible();
30-
31-
expect(pageErrors, 'uncaught exceptions during editor load').toEqual([]);
32-
});
12+
await expect(page.getByText('Backpack', {exact: true})).toBeVisible();
13+
expect(pageErrors, 'uncaught exceptions during editor load').toEqual([]);
3314
});

0 commit comments

Comments
 (0)