Skip to content

CI: mobile e2e smoke + drop SKIP_MOBILE from Makefile#262

Open
moodyjmz wants to merge 4 commits into
mainfrom
ci/mobile-e2e-smoke
Open

CI: mobile e2e smoke + drop SKIP_MOBILE from Makefile#262
moodyjmz wants to merge 4 commits into
mainfrom
ci/mobile-e2e-smoke

Conversation

@moodyjmz

@moodyjmz moodyjmz commented Jul 1, 2026

Copy link
Copy Markdown
Member

Closes #261.

Scope

The gate verifies the mobile build is present and renders a document. It keys on the loading skeleton (.doc-placeholder) clearing — the real "document loaded" signal — rather than the canvas, which mounts with the SDK bundle before any document loads and so can't distinguish "loaded" from "never loaded".

Infra faults (e.g. an unresponsive docservice channel) are out of scope: CI runs a healthy docservice from the same image and won't produce them. A known false-pass in that mode is deferred — closing it properly needs a content-bearing sample file rather than a blank new document.

Test plan

  • Existing desktop suite (example-page.spec.ts) unaffected
  • New mobile suite passes on a working build (4/4)
  • Reds on a missing mobile bundle (.doc-placeholder never clears)

moodyjmz and others added 3 commits July 1, 2026 20:45
Picks up Euro-Office/web-apps#158, which removes SKIP_MOBILE from
build-pipeline.js — mobile now always builds and is always gated.
Required before dropping the dead SKIP_MOBILE pass-through from the
Makefile.

Refs #261

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Since web-apps#158 the build pipeline ignores SKIP_MOBILE, so the
Makefile flag is a silent no-op. Removes the comment block, default,
and both web-apps/web-apps-dev pass-throughs.

Refs #261

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
verify-deploy.mjs only checks that mobile artifacts exist at build
time; it can't catch a mobile editor that deploys but fails at runtime
(#258 — a permanent loading skeleton, or a fatal version-mismatch
dialog blocking the document load).

Forces the mobile bundle deterministically via ?type=mobile (bypassing
the example app's User-Agent sniffing), then asserts the rendering
canvas mounts and that no framework7 error surface is showing. Both
dialog (.dialog.modal-in) and notification (.notification.modal-in)
surfaces are checked, since LoadingScriptError uses f7.notification
rather than f7.dialog.

Covers docx/xlsx/pptx/pdf. Visio is excluded: the example app has no
blank vsdx create-new template to exercise.

Refs #261

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
@moodyjmz moodyjmz requested a review from a team as a code owner July 1, 2026 19:06
@moodyjmz moodyjmz requested review from chrip and removed request for a team July 1, 2026 19:06
@moodyjmz moodyjmz self-assigned this Jul 1, 2026
@MonaAghili

Copy link
Copy Markdown
Contributor

PR #262 Review Issues

Issue 1 — Canvas toBeVisible is not a loading-completion signal

mobile-editor.spec.ts lines 54–55:

await expect(frame.locator(editor.canvasSelector)).toBeVisible({ timeout: 30_000 });
await expect(frame.locator(ERROR_SURFACE_SELECTOR)).toHaveCount(0, { timeout: 30_000 });

The test's own comment admits #id_viewer / #ws-canvas are "part of the static UI scaffold" that stay visible even when the document fails to load. So the toBeVisible check only confirms the React app shell mounted — it says nothing about whether the document loaded. The real gate is the toHaveCount(0) line below it.

This matters when the test fails: Playwright reports "canvas not visible" when the actual problem is likely an error dialog or a stuck preloader. Compare to the desktop test (example-page.spec.ts) which checks #loading-mask toBeHidden — a direct "document finished loading" signal. If a mobile equivalent selector exists, it would give clearer diagnostics and closer parity with the desktop suite.


Issue 2 — Two sequential 30 s timeouts against a 60 s global

Note: You did not introduce the 60 s global timeout — that is pre-existing in playwright.config.ts and unchanged by this branch. This issue is flagged because his new test introduces the two back-to-back timeout: 30_000 assertions that, combined with the existing ceiling, leave no margin. The problem only exists because of what he added; fixing it is his to do.

playwright.config.ts sets timeout: 60_000 per test (pre-existing). The new mobile tests chain two assertions that each allow up to 30 s:

// step 1 — up to 30 s
await expect(frame.locator(editor.canvasSelector)).toBeVisible({ timeout: 30_000 });
// step 2 — up to 30 s
await expect(frame.locator(ERROR_SURFACE_SELECTOR)).toHaveCount(0, { timeout: 30_000 });

Worst-case: step 1 takes 29 s (slow container, slow JS parse), step 2 takes 29 s (slow document load) → 58 s total before the test even returns. Playwright's global timer is already at the ceiling. In practice the canvas check resolves quickly, but on a degraded self-hosted runner there is no margin. Either reduce the canvas timeout (e.g. 15_000, matching the global expect.timeout in config) or raise the per-test timeout for this describe block via test.setTimeout.

@Aiiaiiio Aiiaiiio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice one! This verbose commenting will make this a bit hard to maintain should anything change that is stated here. But at least it's obvious what is happening.

The canvas mounts from the SDK bundle regardless of doc load, so the gate
false-passed when the doc never loaded. Gate on .doc-placeholder clearing
(the isDocReady signal) instead, with a generous timeout. Scope is
build-present-and-renders; infra faults like an unresponsive docservice
channel are out of scope.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 📄 To do

Development

Successfully merging this pull request may close these issues.

CI: mobile e2e smoke + drop SKIP_MOBILE from Makefile

3 participants