Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions TESTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@ For an Electron test that trace is an action log rather than a replay: each step

A failing journey also attaches the screen at the moment it failed and the state the app had persisted, because the interesting half of a failure in this app is usually on disk rather than on screen. In CI both come back as a workflow artifact.

A launch that gives the test no window has no screen to attach, so its error carries what the main process said of itself instead: whether Electron became ready, and each window it had with its address and whether its page was still loading or had crashed. "did not answer" in its place means the main process could still be reached and gave nothing back in five seconds, most likely because it was stuck; "could not be asked", with the reason, means it could not be reached at all, which is what an app that exited during its launch looks like. Whichever it is, an app still running five seconds after it was asked to quit is ended by force, so one such launch costs its test a retry and not the run.

## What CI runs

Three workflows, on every pull request and on push to `trunk`. None needs a secret.
Expand Down
126 changes: 117 additions & 9 deletions tests/e2e/helpers/app.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,18 @@
// goes through TOOLKIT_USER_DATA_DIR — the `!app.isPackaged` hook in
// src/main.js:137 — and `start()` refuses to launch without one.
// 2. Teardown always terminates. `close()` is known to hang on Windows in
// Electron apps that keep child processes alive, which this one does.
// Electron apps that keep child processes alive, which this one does, so
// it is given a limit; and an app still running at that limit is ended by
// force, because Playwright waits for it again when the worker stops.
// 3. A failure leaves evidence. The trace is Playwright's; the screenshot and
// the persisted settings.json are attached here, because the interesting
// half of a failure in this app is what ended up on disk.
// half of a failure in this app is what ended up on disk. A launch that
// opens no window says what the main process knew of itself.
//
// The packaged smoke test does not use this: it launches a different binary and
// deliberately writes no state at all.

const { spawnSync } = require( 'node:child_process' );
const fs = require( 'node:fs' );
const os = require( 'node:os' );
const path = require( 'node:path' );
Expand Down Expand Up @@ -67,6 +71,85 @@ function samePath( a, b ) {

const EMPTY_SETTINGS = Object.freeze( { sites: [], siteMeta: {}, preferences: {} } );

// How long an app gets to end by itself once asked, and how long anything asked
// of an app that may no longer be answering is waited for.
const PATIENCE_MS = 5_000;

/**
* A promise, or nothing once `ms` have passed. The timer is cleared either way,
* so a call that answers at once does not keep the worker alive for the rest.
*
* @param {Promise<*>} promise
* @param {number} ms
* @return {Promise<*>} What the promise settled with, or undefined.
*/
function within( promise, ms ) {
let timer;
const late = new Promise( ( resolve ) => {
timer = setTimeout( resolve, ms );
} );
return Promise.race( [ promise, late ] ).finally( () => clearTimeout( timer ) );
}

/**
* What the main process says of itself: whether Electron ever became ready, and
* the windows it has.
*
* For a launch that opens no window, where there is no screen to photograph and
* these are the only facts that tell the three ways it happens apart: the app
* never became ready, it became ready and made no window, or it made one whose
* page never started.
*
* @param {Object} app The Electron app.
* @return {Promise<{ready: boolean, windows: Object[]}>} The state, read in the main process.
*/
function launchState( app ) {
return app.evaluate( ( { app: electronApp, BrowserWindow } ) => ( {
ready: electronApp.isReady(),
windows: BrowserWindow.getAllWindows().map( ( win ) => ( {
visible: win.isVisible(),
url: win.webContents.getURL(),
loading: win.webContents.isLoading(),
crashed: win.webContents.isCrashed(),
} ) ),
} ) );
}

/**
* Ends an app that did not end when asked.
*
* Not `proc.kill()`, which is what this used to be. On macOS and Linux that
* sends SIGTERM, which a main process that has stopped answering survives: put
* it back and the engine's own test of this fails. On Windows it ends the shell
* Playwright launched through and leaves the app. Playwright then waits for
* the process to exit when the worker stops, with no limit of its own, so one
* launch that gave no window on a macOS runner failed a whole run on "Worker
* teardown timeout" after its test had passed on the retry. `proc.kill()` also
* marks the process as killed whether or not it went, which is what Playwright
* reads before its last forced kill, when the worker exits, and skips it.
*
* This does not wait for a quit that is merely slow. An app still sweeping its
* children at the limit is ended mid-sweep, and a child it started as the
* leader of a group of its own is then left behind.
*
* @param {Object} proc The process Playwright started.
*/
function forceKill( proc ) {
if ( process.platform === 'win32' ) {
// Playwright launches through a shell there, so this is cmd.exe and the
// app is its child: /T takes the tree.
spawnSync( 'taskkill', [ '/pid', String( proc.pid ), '/T', '/F' ], { windowsHide: true } );
return;
}
try {
// Playwright starts the app as the leader of its own process group, so
// this takes the app's helpers with it.
process.kill( -proc.pid, 'SIGKILL' );
} catch {
// Gone in the meantime.
}
}

/**
* Where to record each journey, so a person can watch what the test did:
* `E2E_VIDEO=/tmp/e2e-video npm run test:e2e`. Unset, nothing is recorded and
Expand Down Expand Up @@ -164,7 +247,25 @@ class Session {
TOOLKIT_USER_DATA_DIR: this.userDataDir,
},
} );
this.page = await this.app.firstWindow();
try {
this.page = await this.app.firstWindow();
} catch ( error ) {
// Nothing to photograph, so the evidence is what the main process
// knows. It may be the thing that is stuck, hence the limit; and it
// may be gone, which is a different answer and is told apart.
const state = await within(
launchState( this.app ).then(
( read ) => `reported ${ JSON.stringify( read ) }.`,
( refusal ) => `could not be asked for its state: ${ refusal?.message ?? refusal }.`
),
PATIENCE_MS
);
throw new Error(
`${ error.message }\nThe launch gave the test no window. The app's main process ` +
( state || 'did not answer when asked for its state.' ),
{ cause: error }
);
}
if ( VIDEO_DIR ) {
const video = this.page.video();
if ( video ) this.videos.push( video );
Expand Down Expand Up @@ -265,11 +366,18 @@ class Session {
// Grab the handle before closing — `process()` throws once the connection
// to the app is gone.
const proc = app.process();
await Promise.race( [
app.close().catch( () => {} ),
new Promise( ( resolve ) => setTimeout( resolve, 5_000 ) ),
] );
if ( proc && proc.exitCode === null ) proc.kill();
const exited = new Promise( ( resolve ) => proc.once( 'exit', resolve ) );
const running = () => proc.exitCode === null && proc.signalCode === null;

// `close()` resolves once the process has gone, hence the limit: an app
// that will not quit would hold the teardown for good.
await within( app.close().catch( () => {} ), PATIENCE_MS );
if ( ! running() ) return;

forceKill( proc );
// Waited for, so that the directories the app had open are removed after
// it has let go of them, and with a limit, so that teardown ends anyway.
await within( exited, PATIENCE_MS );
}

/**
Expand Down Expand Up @@ -376,4 +484,4 @@ const test = base.extend( {

const { expect } = base;

module.exports = { test, expect, Session, EMPTY_SETTINGS, REPO_ROOT };
module.exports = { test, expect, Session, launchState, EMPTY_SETTINGS, REPO_ROOT };
63 changes: 61 additions & 2 deletions tests/e2e/journeys/engine.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,24 @@
* The engine's own test (#360).
*
* Not a journey: it asserts nothing about ticket branches or patches. It asserts
* that the five things every journey depends on actually work, so that when a
* that the things every journey depends on actually work, so that when a
* journey fails it is about the flow and not about the harness.
*
* 1. The app launches from the source tree and paints.
* 2. The design system's tokens and the styles it adds at run time are applied.
* 3. It uses the throwaway profile, and nothing else.
* 4. What it persists survives closing and reopening it.
* 5. A native file dialog can be answered from the test.
* 6. An app that will not quit is ended, so teardown always finishes.
* 7. A launch can be asked what it came to, for the one that opens no window.
*
* If this file is red, no other journey's result means anything.
*/

const fs = require( 'node:fs' );
const os = require( 'node:os' );
const path = require( 'node:path' );
const { test, expect } = require( '../helpers/app.cjs' );
const { test, expect, launchState } = require( '../helpers/app.cjs' );
const ui = require( '../helpers/ui.cjs' );

// Directories made during a test, removed after it. The profile is the session
Expand Down Expand Up @@ -181,6 +183,63 @@ test( 'a native file dialog can be answered from the test', async ( { session }
expect( chosen.text ).toContain( 'wp-login.php' );
} );

/**
* Whether a process is still there. Signal 0 delivers nothing and only asks.
*
* @param {number} pid
* @return {boolean} False once the process has gone.
*/
function isRunning( pid ) {
try {
process.kill( pid, 0 );
return true;
} catch {
return false;
}
}

test( 'closing ends an app that has stopped answering', async ( { session } ) => {
const { app } = await session.start();

// A main process that has stopped answering, which is how a launch that
// never finished looked from outside on a macOS runner: asked to quit it
// stays, and a termination signal is one more thing it never gets to.
const pid = await app.evaluate( () => {
setTimeout( () => {
for ( ;; ) {
// Busy for good, once this call has answered.
}
}, 0 );
return process.pid;
} );
expect( isRunning( pid ) ).toBe( true );

// INVARIANT: close() ends the app. Left running, it is what Playwright waits
// on when the worker stops, and the run fails on "Worker teardown timeout" a
// minute after its last test passed. Polled for Windows, where what close()
// waits for is the shell the app was launched through, and the app itself
// can be a moment behind it.
await session.close();
await expect.poll( () => isRunning( pid ) ).toBe( false );
} );

test( 'the main process says what a launch came to', async ( { session } ) => {
const { app, page } = await session.start();
await expect( ui.renderedApp( page ) ).not.toHaveCount( 0 );

// Read here on a launch that worked, because the launch it is for cannot be
// staged: it is what the helper puts in the error when no window opens.
await expect.poll( () => launchState( app ) ).toEqual( {
ready: true,
windows: [ {
visible: true,
url: expect.stringMatching( /\/src\/renderer\/index\.html$/ ),
loading: false,
crashed: false,
} ],
} );
} );

test( 'a failed site deletion stays visible, reports the failure, and can be retried (#414)', async ( { session } ) => {
const site = makeListedSite( session, 'delete-retry' );
const { app, page } = await session.start( site.settings );
Expand Down
Loading