Skip to content

Commit 4683ce3

Browse files
Merge pull request #69 from off-grid-ai/test/e2e-settings-coverage-and-gating
test(e2e): fix 7 failing specs on main, cover Settings, and stop the advisory gate hiding drift
2 parents 556d435 + e4ab953 commit 4683ce3

50 files changed

Lines changed: 627 additions & 108 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.github/workflows/ci.yml

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -139,13 +139,20 @@ jobs:
139139
# Electron needs an X server on Linux; xvfb-run provides a virtual one. --no-sandbox via env
140140
# (Electron's sandbox can't run under the CI user without SUID setup).
141141
#
142-
# ADVISORY (continue-on-error) for now — same policy as lint/test:heavy above. A first clean run
143-
# is 49 passed / 6 skipped / 15 failed, and the 15 are headless-Electron flakiness on the runner
144-
# (waitForEvent 'window' timeouts, "page/context closed", clipboard/display-dependent specs), NOT
145-
# product failures — the local/pre-push e2e on a real display shows no regressions. This still
146-
# RUNS the tour and uploads screenshots so a real break is visible; graduate it to BLOCKING once
147-
# the headless-Electron launch is stabilized (GPU/swiftshader + sandbox flags + retries).
148-
# Tracked in docs/GAPS_BACKLOG.md.
142+
# ADVISORY (continue-on-error) for now — same policy as lint/test:heavy above. The remaining
143+
# reason is headless-Electron launch instability on the ubuntu runner (waitForEvent 'window'
144+
# timeouts, "page/context closed", clipboard/display-dependent specs), NOT product failures.
145+
#
146+
# NOTE: this being advisory hid four real spec defects on main for days (a selector that
147+
# could never match, a section never opened, two stale accessible names) — the job was green
148+
# while 7 specs failed. Every one is fixed and the full suite is 73/73 on a real display, so
149+
# the ONLY thing still standing between this and BLOCKING is runner stability. Steps taken:
150+
# retries: 2 in CI (playwright.config.ts) for whole-instance launch flakes, and fixed-port
151+
# specs now self-skip instead of failing (e2e/helpers/ports.ts).
152+
#
153+
# Graduate to BLOCKING (delete continue-on-error) once a few consecutive runs are green —
154+
# do NOT flip it while the runner still drops instances, or the gate gets reverted and we
155+
# lose the signal again. Tracked in docs/GAPS_BACKLOG.md.
149156
- name: E2E (Playwright, xvfb)
150157
timeout-minutes: 28
151158
continue-on-error: true

docs/GAPS_BACKLOG.md

Lines changed: 46 additions & 15 deletions

e2e/helpers/onboarding.ts

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
import { expect, type Page } from '@playwright/test'
2+
3+
/**
4+
* Click through onboarding into the app shell.
5+
*
6+
* Two failure modes this exists to prevent, both seen in the suite:
7+
*
8+
* 1. Racing the first paint. Specs looped `if (!visible) break` immediately after
9+
* domcontentloaded, so if onboarding had not rendered yet the loop exited on step 0 and
10+
* left the app sitting on onboarding — every later nav lookup then timed out against the
11+
* onboarding DOM. We wait for the CTA to appear before stepping.
12+
*
13+
* 2. A CTA regex that misses the last step. meeting-transcription.spec.ts matched
14+
* /Continue|Start using Off Grid AI Desktop/, but the final button renders
15+
* 'Start using Off Grid' (src/renderer/src/components/Onboarding.tsx), so the last step
16+
* never got clicked. One shared matcher means one place to keep in sync.
17+
*
18+
* NOTE: the button copy is 'Start using Off Grid' while the product name is 'Off Grid AI
19+
* Desktop'. Matched as-is here rather than changing user-facing copy from a test fix.
20+
*/
21+
const ONBOARDING_CTA = /^(Continue|Start using Off Grid)/i
22+
23+
export const completeOnboarding = async (page: Page, maxSteps = 10): Promise<void> => {
24+
const cta = page.getByRole('button', { name: ONBOARDING_CTA }).first()
25+
// An existing/seeded profile may skip onboarding entirely — that is not a failure.
26+
const onOnboarding = await cta
27+
.waitFor({ state: 'visible', timeout: 10_000 })
28+
.then(() => true)
29+
.catch(() => false)
30+
if (!onOnboarding) return
31+
32+
for (let step = 0; step < maxSteps; step += 1) {
33+
if (!(await cta.isVisible().catch(() => false))) break
34+
await cta.click().catch(() => {})
35+
await page.waitForTimeout(350)
36+
}
37+
// Reaching the shell is the point of this helper — assert it rather than hoping.
38+
await expect(cta).toBeHidden()
39+
}

e2e/helpers/ports.ts

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
import net from 'node:net'
2+
import { GATEWAY_HOST, GATEWAY_PORT, LLAMA_SERVER_PORT, MEDIA_PORT } from '../../src/shared/ports'
3+
4+
/**
5+
* Precondition guard for specs that assert against the FIXED engine ports.
6+
*
7+
* Those ports are single-owner. When a dev has the real app (or `npm run dev`) running, it
8+
* owns 7878/7879/8439, and specs probing them talk to the WRONG process:
9+
*
10+
* - smoke.spec.ts asserts llama-server is unreachable on a fresh profile, but a running
11+
* app's llama-server answers on 8439, so the assertion inverts
12+
* - smoke.spec.ts polls the gateway for its own fixture model, but 7878 belongs to the
13+
* other app, which never serves that fixture
14+
* - resilience-single-instance.spec.ts waits for the ports to close after teardown, and the
15+
* other app's ports never close
16+
*
17+
* All three produced false failures that read exactly like product regressions. The app also
18+
* auto-falls-back to a free port when a fixed one is held, so the e2e instance may not even
19+
* be on the port the spec is probing.
20+
*
21+
* Rather than assert against a port we do not own, these specs SKIP with the reason. CI runs
22+
* on a clean machine, so it still executes them for real — which is what lets the e2e job
23+
* gate instead of running advisory.
24+
*/
25+
26+
const ENGINE_PORTS: { port: number; what: string }[] = [
27+
{ port: GATEWAY_PORT, what: 'gateway' },
28+
{ port: MEDIA_PORT, what: 'media server' },
29+
{ port: LLAMA_SERVER_PORT, what: 'llama-server' }
30+
]
31+
32+
/**
33+
* True when something is LISTENING on the port. A raw TCP connect, not an HTTP probe: a
34+
* server answering 404 is still an owner, and an HTTP error would misreport it as free.
35+
*/
36+
export const portIsBusy = async (port: number, timeoutMs = 400): Promise<boolean> =>
37+
new Promise((resolve) => {
38+
const socket = net.connect({ port, host: GATEWAY_HOST })
39+
const finish = (busy: boolean): void => {
40+
socket.destroy()
41+
resolve(busy)
42+
}
43+
socket.setTimeout(timeoutMs)
44+
socket.once('connect', () => finish(true))
45+
socket.once('timeout', () => finish(false))
46+
socket.once('error', () => finish(false))
47+
})
48+
49+
/** Which canonical engine ports are already owned, as human-readable labels. */
50+
export const busyEnginePorts = async (): Promise<string[]> => {
51+
const checks = await Promise.all(
52+
ENGINE_PORTS.map(async ({ port, what }) =>
53+
(await portIsBusy(port)) ? `${what} :${port}` : null
54+
)
55+
)
56+
return checks.filter((entry): entry is string => entry !== null)
57+
}
58+
59+
/**
60+
* Reason string to pass to test.skip() when the fixed ports are not ours, or null when they
61+
* are all free and the spec can run for real. Call BEFORE launching the Electron app.
62+
*/
63+
export const enginePortsUnavailableReason = async (): Promise<string | null> => {
64+
const busy = await busyEnginePorts()
65+
if (busy.length === 0) return null
66+
return `engine ports already owned by another process (${busy.join(', ')}) — quit Off Grid AI Desktop / npm run dev and re-run. These specs assert against the fixed ports and cannot share them.`
67+
}

e2e/helpers/settings.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
import { expect, type Locator, type Page } from '@playwright/test'
2+
3+
/**
4+
* Shared Settings navigation for E2E specs.
5+
*
6+
* Why this exists: Settings sections are a SINGLE-OPEN accordion group
7+
* (SettingsCardsGroup in src/renderer/src/components/SettingsCard.tsx). Opening one card
8+
* makes it the full-width L2 detail and EXIT-ANIMATES every sibling out of the DOM. Specs
9+
* that opened one section and then clicked another were clicking a card that was on its way
10+
* out — Playwright reported "element is not stable", then "element was detached from the
11+
* DOM", then timed out. That is what broke both Settings tests in tour.spec.ts.
12+
*
13+
* The fix is to model the real interaction: collapse the open detail (back to the grid)
14+
* before opening the next section. Every spec goes through these helpers so the next
15+
* section added gets the correct behaviour for free.
16+
*/
17+
18+
const escapeForRegExp = (value: string): string => value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
19+
20+
/**
21+
* A primary-nav button, tier-agnostic. Locked Pro items compute an accessible name with a
22+
* "Pro" suffix ("Meetings Pro") because the lock badge is an <img alt="Pro">, so an exact
23+
* 'Meetings' match finds nothing in a free build — the drift that broke
24+
* meeting-transcription.spec.ts. Matches the label with or without the suffix.
25+
*/
26+
export const navButton = (page: Page, label: string): Locator =>
27+
page.getByRole('button', { name: new RegExp(`^${escapeForRegExp(label)}( Pro)?$`) }).first()
28+
29+
/** Expand the sidebar (so nav labels are visible) and open the Settings screen. */
30+
export const gotoSettings = async (page: Page): Promise<void> => {
31+
const expandSidebar = page.getByRole('button', { name: 'Expand sidebar' })
32+
if (await expandSidebar.isVisible().catch(() => false)) await expandSidebar.click()
33+
await page.getByRole('button', { name: 'Settings', exact: true }).first().click()
34+
await expect(page.getByRole('heading', { name: 'Settings' })).toBeVisible()
35+
}
36+
37+
/** The accordion header button for a section (its accessible name starts with the title). */
38+
export const settingsSectionHeader = (page: Page, title: string): Locator =>
39+
page.getByRole('button', { name: new RegExp(`^(All settings\\s*)?${escapeForRegExp(title)}`) })
40+
41+
/**
42+
* Collapse whichever section is currently the open detail, returning to the card grid.
43+
* Uses the group's own Cmd/Ctrl+] shortcut — the same seam the app ships — so this keeps
44+
* working if the header markup changes. No-op when nothing is open.
45+
*/
46+
export const closeSettingsSection = async (page: Page): Promise<void> => {
47+
const open = page.locator('button[aria-expanded="true"]')
48+
if ((await open.count()) === 0) return
49+
await page.keyboard.press('Control+]')
50+
await expect(open).toHaveCount(0)
51+
}
52+
53+
/**
54+
* Open a Settings section by title and wait until its body is actually expanded.
55+
* Collapses any other open section first, because siblings are hidden while one is open.
56+
*/
57+
export const openSettingsSection = async (page: Page, title: string): Promise<void> => {
58+
await closeSettingsSection(page)
59+
const header = settingsSectionHeader(page, title)
60+
await expect(header).toBeVisible()
61+
await header.click()
62+
await expect(header).toHaveAttribute('aria-expanded', 'true')
63+
}

e2e/meeting-transcription.spec.ts

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,18 @@
55
* saved recording; seeded meetings are transcript-only), so here we verify the provenance the
66
* user sees and capture a screenshot for the record.
77
*/
8-
import { test, expect, _electron as electron, type ElectronApplication, type Page } from '@playwright/test'
8+
import {
9+
test,
10+
expect,
11+
_electron as electron,
12+
type ElectronApplication,
13+
type Page
14+
} from '@playwright/test'
915
import os from 'os'
1016
import path from 'path'
1117
import fs from 'fs'
18+
import { navButton } from './helpers/settings'
19+
import { completeOnboarding } from './helpers/onboarding'
1220

1321
const PRO_PRESENT = fs.existsSync(path.resolve('pro/package.json'))
1422
let app: ElectronApplication
@@ -32,12 +40,7 @@ test.beforeAll(async () => {
3240
page = await app.firstWindow()
3341
await page.emulateMedia({ reducedMotion: 'reduce' })
3442
await page.waitForLoadState('domcontentloaded')
35-
for (let i = 0; i < 8; i++) {
36-
const btn = page.getByRole('button', { name: /Continue|Start using Off Grid AI Desktop/i })
37-
if (!(await btn.isVisible().catch(() => false))) break
38-
await btn.click().catch(() => {})
39-
await page.waitForTimeout(400)
40-
}
43+
await completeOnboarding(page)
4144
await page.waitForTimeout(1500)
4245
})
4346

@@ -47,7 +50,9 @@ test.afterAll(async () => {
4750
})
4851

4952
test('meeting detail exposes the STT model picker (view + change)', async () => {
50-
await page.getByRole('button', { name: 'Meetings', exact: true }).first().click()
53+
// navButton tolerates the locked-Pro accessible name ('Meetings Pro') so this lookup does
54+
// not depend on the tier the build was launched in.
55+
await navButton(page, 'Meetings').click()
5156
await page.waitForTimeout(800)
5257
// Open the first seeded meeting so the detail (with the transcription controls) renders.
5358
const firstMeeting = page.locator('[class*="cursor-pointer"]').filter({ hasText: /·/ }).first()

e2e/pro.spec.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,26 @@ test('Clipboard quick-open renders populated content on the first native hotkey
422422
timeout: 10_000
423423
})
424424
if (process.platform === 'darwin') {
425+
// System Events delivers the keystroke to whatever app is FRONTMOST, so the app has to
426+
// be it. Without this the spec only passed when an earlier spec happened to leave the
427+
// window focused: it passed in a full-suite run and failed 3/3 with pro.spec.ts alone,
428+
// because the keystroke went to the terminal instead. Order-dependent, not a product bug.
429+
await app.evaluate(({ app: electronApp, BrowserWindow }) => {
430+
BrowserWindow.getAllWindows()[0]?.show()
431+
electronApp.focus({ steal: true })
432+
})
433+
// Focus is asynchronous at the OS level — asking for it is not the same as having it, and
434+
// sending the keystroke too early delivers it to the previously-frontmost app. Wait for
435+
// the window to actually report focus.
436+
await expect
437+
.poll(
438+
() =>
439+
app.evaluate(
440+
({ BrowserWindow }) => BrowserWindow.getAllWindows()[0]?.isFocused() ?? false
441+
),
442+
{ timeout: 5_000 }
443+
)
444+
.toBe(true)
425445
await execFileAsync('/usr/bin/osascript', [
426446
'-e',
427447
'tell application "System Events" to keystroke "c" using {command down, shift down}'

e2e/resilience-single-instance.spec.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import os from 'node:os'
1111
import path from 'node:path'
1212
import electronExecutable from 'electron'
1313
import { GATEWAY_PORT, MEDIA_PORT } from '../src/shared/ports'
14+
import { enginePortsUnavailableReason } from './helpers/ports'
1415

1516
const executable = electronExecutable as unknown as string
1617

@@ -46,6 +47,14 @@ const waitForOwnedPortsToClose = async (): Promise<void> => {
4647
throw new Error('Electron model ports remained reachable after app teardown')
4748
}
4849

50+
// This whole spec is about exclusive ownership of the fixed model ports — including the
51+
// teardown assertion that they close. It cannot share them with a running app, so skip
52+
// rather than emit a false failure. CI runs clean and executes it for real.
53+
test.beforeAll(async () => {
54+
const reason = await enginePortsUnavailableReason()
55+
test.skip(reason !== null, reason ?? '')
56+
})
57+
4958
test.beforeEach(async () => {
5059
userDataDir = fs.mkdtempSync(path.join(os.tmpdir(), 'offgrid-single-instance-e2e-'))
5160
app = await electron.launch({
760 Bytes
490 Bytes

0 commit comments

Comments
 (0)