ci(e2e): turn on the shared "E2E Tests (Playwright)" job - #248
Merged
Conversation
The 10-file e2e suite under tests/e2e/ has existed and been maintained for months, but `enable-playwright` was never set in the caller — so the shared workflow reported "E2E Tests (Playwright)" as `skipped` on every run. A skipped job looks exactly like a job with nothing to complain about, so the suite was effectively dead while reading green. Four things had to be true before the switch could be honest. 1. A CI-only config, tests/e2e/playwright.config.ts The shared workflow runs `npx playwright test --config=<path>` with NO `--project`, so every project in whichever config it picks runs. The root config declares three: `chromium`, `docs-capture` (re-shoots every documentation screenshot) and `visual` (whose committed PNG baselines are host-font/GPU specific and cannot byte-match a Linux runner — its own README says so). `playwright-test-path: tests/e2e` makes the workflow's FIRST config lookup hit the new file, which declares exactly one project. The root config is untouched: the separate "Journeydoc Capture" job greps it for `name: 'docs-capture'` and would break if that project were removed. `testIgnore` is spelled out at BOTH top level and project level, because a project-level `testIgnore` REPLACES the top-level one rather than merging. 2. tests/e2e/_base-url.ts now accepts BASE_URL That is the name the shared workflow actually exports, and it was the one spelling the resolver omitted. openconnector shipped a PLAYWRIGHT_BASE_URL-only resolver and its E2E job has hard-failed on every run since. The `localhost:8080` fallback is allowed ONLY when CI / GITHUB_ACTIONS is set (there it is the runner's own throwaway `php -S`); off CI it still throws, because :8080 on a developer box is the shared dev container. 3. tests/e2e/ci-seed.sh — explicit, forced, VERIFIED register import `occ app:enable larpingapp` imports the register from an IRepairStep, which runs with no user session, so OpenRegister's RBAC denies it — and the repair step swallows the exception as a warning, so `occ app:enable` still exits 0. The app enables cleanly and the register simply is not there. The seed posts to /apps/larpingapp/api/settings/reimport, which is loadSettings(force: true) and, unlike OpenRegister's generic importer, merges the lib/Settings/register.d/*.json fragments first. (larpingapp does NOT use `AppHost\Routes::standard()`, so there is no `settings#load` route here.) It then VERIFIES the register plus all 11 schema slugs, read out of the repo's own JSON rather than derived — three of them are prefixed (`larping_item`, `larping_event`, `larping_attendance`) precisely because the bare `item` slug collides with a foreign app's QTI schema. Finally it gates on the bundle SERVING as JavaScript. A missing bundle does not 404 on Nextcloud; it returns HTTP 200 text/html, which every status-code check in the pipeline reads as success. 4. The stat harness no longer assumes docker tests/e2e/workflows/fixtures.ts ran CharacterService via `docker exec … nextcloud` with the bootstrap hardcoded to /var/www/html/lib/base.php. CI has no docker daemon and no such container, so the harness returned null — which two tests treated as `test.skip` (reads as a pass) and the third as a failure blaming the arithmetic. It now finds the server root by walking up to `lib/base.php` + `config/config.php` and runs the bootstrap in-process, falling back to docker on a developer box. The `test.skip` escape hatch is now gated OFF on CI: there, a null harness result must fail rather than silently turn the two highest-value correctness assertions in this repo into a no-op. Also adds `additional-apps` pinned to openregister@development. larpingapp is a thin client with no tables of its own — without openregister there is nothing for the SPA to render or the fixtures to create. No assertion was weakened, no test skipped, and no timeout raised.
Contributor
Quality Report — ConductionNL/larpingapp @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 537/537 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ |
Quality workflow — 2026-08-03 22:26 UTC
Download the full PDF report from the workflow artifacts.
…d the whole player schema
Found by the newly-enabled E2E job (run 30858375923), which is the first thing
in this repo to verify the register import on a CLEAN instance:
[ci-seed] schemas present: [ability, character, condition, effect,
larping_attendance, larping_event, larping_item, setting, skill, xpaward]
::error::LarpingApp schemas missing after import: ['player']
The cause, from the Nextcloud log in that same run:
[ImportHandler] Failed to create schema (Pass 1) schemaKey=player
Invalid string format 'user' at '/userUid'. Must be one of: , text, markdown,
html, date-time, … , semver
`player.userUid` has carried `"format": "user"` since f434f76 (2026-07-08).
OpenRegister has never had a `user` format — see
OCA\OpenRegister\Service\Schemas\PropertyValidatorHandler::$validStringFormats —
and it rejects unknown formats at PASS 1, which drops the ENTIRE schema.
So for four weeks EVERY FRESH INSTALL of larpingapp has had no `player` schema:
no Players index, no player detail, no character→player link target. This is a
product bug, not a test-harness artefact.
What kept it invisible for four weeks is worth recording, because none of it is
specific to this property:
* a dev instance already had a `player` schema created BEFORE the change, so
the re-import failing changed nothing observable there;
* the repair step catches \Throwable and downgrades it to a warning, so
`occ app:enable larpingapp` still exits 0; and
* `settings#reimport` returned HTTP 200 with `{"success": true}` while
dropping the schema.
Three independent green signals over a schema that was not there.
Fix: `userUid` is a Nextcloud user UID — a plain string. The `format` is
removed, not translated: there is nothing in OpenRegister's vocabulary that
means "a Nextcloud user", and inventing one client-side is what broke it. The
schema version is bumped 1.1.0 → 1.1.1 so OpenRegister's version-gated import
re-applies it on instances that already carry the broken definition.
Regression gate: tests/validate-register.js now checks every string `format`
against a mirror of OpenRegister's allow-list, recursing into `properties`,
`items` and `$defs`, and covering the lib/Settings/register.d/*.json fragments
too (they are merged into the same import payload). Verified BOTH ways — it
passes on the fixed file, and re-injecting `"format": "user"` makes it exit 1
naming the property. A static check is the only thing that can catch this,
since a dev instance that already has the schema cannot.
Contributor
Quality Report — ConductionNL/larpingapp @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 537/537 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ |
Quality workflow — 2026-08-04 05:16 UTC
Download the full PDF report from the workflow artifacts.
Baseline on this branch (run 30879501930, job 91897968947):
143 passed / 5 failed / 24 skipped in 7.4 min.
1. `character.ocName` is a RELATION, not a name (product/schema vs fixtures)
`lib/Settings/larpingapp_register.json` declares
`ocName: {type: string, format: uuid, $ref: player}` and marks it
`required` — "The player who plays this character". Two fixtures passed the
character's own display name there, so every create returned
HTTP 400 "Property 'ocName' should match format 'uuid'".
* `workflows/fixtures.ts::seedStatScenario` — the direct cause of the three
red character-stat specs.
* `spec-coverage/detail-forms-admin.spec.ts::beforeAll` — this one failed
SILENTLY: the seed helper returns null on error and the caller substituted
the literal string `'seed-missing'`, so seventeen character-detail specs
navigated to `#/characters/seed-missing` and still reported green.
Both now seed a real `player` row first and reference its UUID.
`x-allow-create: true` is a picker affordance for the Vue relation widget;
it does not make the REST API accept a bare name.
`ocName` was also being sent to player/ability/skill/item/condition/effect/
event, which have no such property — dropped.
2. `gotoDetail()` asserted a heading that is never rendered (test defect)
A detail page renders the OBJECT's name as its `<h2>`; the entity type is a
kicker paragraph above it. The old assertion — "some heading in .app-content
matches /<type>/i" — passed for the wrong reasons:
* it matched incidental widget titles ("Skills granting this effect"
satisfies /Effect/i);
* it matched the LIST heading ("Characters" satisfies /Character/i), which
is exactly what kept the seventeen `seed-missing` specs green;
* and it happened to hard-fail only on `events`, whose seeded name
("…-summer-larp") contains no occurrence of "event".
It now asserts the seeded object's own name, which proves the hash route
resolved AND the right object was fetched AND rendered. A missing seed is
now a thrown error naming the type, not a vacuous pass.
3. Sidebar groups were collapsed on the dashboard (product)
`openspec/specs/dashboard/spec.md#sidebar-shows-all-entity-views` requires
the sidebar to display Characters, Events, Players, Items, Conditions,
Abilities, Skills and Effects. The manifest puts all eight inside three
collapsible groups and `CnAppNav.isItemOpen()` falls back to
`Boolean(item.open)` — default false — so on `/` none of the eight entries
exist in the DOM at all. They appeared only on a route that is a child of
the group (`hasActiveChild()`), which is why the same test ids assert fine
from `/characters` and not from `/`.
`"open": true` on CharactersGroup / MechanicsGroup / WorldGroup. The key is
part of the v2 manifest menu schema; validated with Ajv against
app-manifest-v2.schema.json 2.22.0 (PASS, and a deliberately injected bad
key FAILs, so the validator is live).
Also: `retries: 0`. A retry can only turn red green, so it converts an
intermittent defect into a reported PASS, and it doubles the cost of the worst
case (a spec that fails by exhausting the 60 s timeout). The suite runs at
~37% of the 20 min cap, so there is room to see flake instead of hiding it.
No test skipped, no assertion weakened, no timeout raised, no allow-list
widened, no suppression added.
This was referenced Aug 4, 2026
Contributor
Quality Report — ConductionNL/larpingapp @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 537/537 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ |
Quality workflow — 2026-08-04 08:13 UTC
Download the full PDF report from the workflow artifacts.
Round 1 of the enabled gate: 145 passed / 3 failed / 24 skipped in 8.3 min
(run 30890120431, job 91930353528). Two of the three are assertions that only
held while a defect was present, and went red the moment it was fixed.
1. character-stat-computation.workflow.spec.ts:164 — the last surviving
`ocName: name`. This test builds its own character inline rather than going
through seedStatScenario(), so the fixtures.ts fix did not reach it. Seeds a
real `player` and references its UUID, same as the other two.
2. detail-forms-admin.spec.ts "character detail renders approval-capable
shell" — asserted a second copy of the old type-heading check
("some heading matches /Character/i"). That was satisfied by the NOT-FOUND
shell: when the object fails to load, CnDetailPage falls back to the
manifest page title ("Character") as its heading. So the line passed
precisely while the character seed was broken and stopped passing as soon as
a real character loaded — exactly backwards. It now asserts the approval
control the scenario ("approve a character") is actually about.
3. spa-ui.spec.ts "empty dashboard renders KPI 0 counts" — encoded the spec's
"GIVEN no entities exist in the system" as toHaveText('0') on a shared
instance that other specs in this suite seed into, without establishing the
precondition. It passed only because the character seed was returning
HTTP 400, so the count really was 0. Now reads the real total from
OpenRegister (asserting the STATUS, not just the payload — a 403 body parses
to {} and would become a silent "0") and asserts the KPI tile equals it,
plus the empty-state row appears iff the collection is empty. Tiles are
addressed by their manifest widget id via CnDashboardGrid's role=group
aria-label instead of .first().
No test skipped, no assertion weakened, no timeout raised.
Contributor
Quality Report — ConductionNL/larpingapp @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 537/537 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ |
Quality workflow — 2026-08-04 08:28 UTC
Download the full PDF report from the workflow artifacts.
…sted nothing ## Unpark (23 specs: skipped -> passing) Their blocker annotation already ended with 'RESOLVED (verified 2026-08-01): the cause is a MISSING player SCHEMA', and both halves of that are fixed on this branch — 74fab69 restored the schema to the import, and the fixtures now reference a real player UUID instead of a display name. Measured before proposing it, on a throwaway PR cut from this branch's head (#253, run 30892287689, job 91937270750): 171 passed / 0 failed / 1 skipped in 8.8 min. All 23 pass, including every per-object Actions-menu assertion. The obsolete blocker docblock + its now-unreferenced DETAIL_ACTIONS_BLOCKER const are replaced by a short record of what the cause actually was and how the first two diagnoses were wrong. ## Two specs that asserted nothing (found by the truncation control) The negative control (#251, job 91930520433) truncated every js/*.js to 0 bytes and ran the suite unchanged: 118 of the 122 specs that got a verdict FAILED. Four passed with no JavaScript at all. Two are legitimate backend probes (event-runsheet-export, skill-requirement-enforcement — HTTP endpoint checks that correctly need no SPA). The other two were dead: * 'settings-management-ui > admin settings panel loads' asserted '.app-content, #app-content, .section' is visible — Nextcloud's OWN server-rendered settings chrome, present whether or not larpingapp mounts — plus a URL check that a goto cannot fail. * 'admin-settings > admin opens larpingapp settings panel' asserted expect(page.locator('body')).toBeVisible(), true on any page that loads, plus the same tautological URL check. Both now assert the app's own Vue admin panel (its heading and the 'Save All' control), which exists only if larpingapp's bundle loaded and mounted.
Contributor
Quality Report — ConductionNL/larpingapp @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 537/537 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ |
Quality workflow — 2026-08-04 09:07 UTC
Download the full PDF report from the workflow artifacts.
…lity
E2E job 91942310154 (170 passed / 1 failed / 1 skipped, 10.2 min) lost
'character list renders view toggle and add controls' to a full 60 s timeout.
The log is unambiguous: the sidebar link was resolved, 'visible, enabled and
stable' on every one of 114 retry passes, and every click was intercepted by
<div class="cn-walkthrough__dim cn-walkthrough__dim--full">
from <div role="dialog" aria-modal="true" class="cn-walkthrough"
aria-label="Welcome to LARPing">
i.e. the six-step first-visit onboarding tour declared in src/manifest.json,
whose dim layer covers the viewport and swallows pointer events.
Two changes, one cause:
1. ci-seed.sh now marks the tour as already seen for the CI admin user, by
PUTting the manifest's own `walkthrough.completionConfigKey`
(`walkthrough_completed_version`) — the store `useWalkthrough` treats as
AUTHORITATIVE — with a version above every step's `sinceVersion`. It then
READS IT BACK and fails the seed if it did not stick: a 200 alone proves
nothing here, because an app not serving `/api/preferences/{key}` answers
200 with the SPA's HTML, which useWalkthrough reads as 'no opinion' and
which would leave the tour armed while the step looked successful.
This suppresses a first-run affordance, not behaviour under test: no spec in
tests/e2e asserts the walkthrough — every mention of it is a comment about
fighting it.
2. dismissSupportDialog() now verifies its own postcondition. Every click in it
is `.catch(() => {})`, so it returned 'successfully' with the overlay still
up, and the caller's next click then burned 60 s and reported the failure
against whatever element it was aiming at. It now throws with the actual
cause named. Scoped to `.cn-walkthrough__dim` specifically rather than to
every aria-modal dialog, so it cannot start failing on modals that are open
but not blocking.
No timeout raised, no retry added, no assertion weakened.
Contributor
Quality Report — ConductionNL/larpingapp @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 101/101 | |||
| npm | ✅ | ✅ 537/537 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ |
Quality workflow — 2026-08-04 09:24 UTC
Download the full PDF report from the workflow artifacts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Turns
enable-playwright: trueon in.github/workflows/code-quality.ymland ships the four artifacts the shared workflow needs to run the existing 10-file e2e suite honestly.Before this PR the shared workflow reported
quality / E2E Tests (Playwright)asskippedon every run — a conclusion indistinguishable, in every summary the pipeline prints, from a job that passed. The suite undertests/e2e/(172 tests across 10 spec files) has been maintained for months and was never executed by CI.Why each piece exists
1.
tests/e2e/playwright.config.ts(new) — CI-only, ONE projectThe shared workflow runs:
No
--projectis passed, so every project in the chosen config runs. The root config declares three:chromiumdocs-capturevisualplaywright-test-path: tests/e2emakes the workflow's first config lookup hit the new file, which declares exactly one project. The root config is deliberately untouched — the separateJourneydoc Capturejob greps it forname: 'docs-capture'and would hard-fail if that project were removed.testIgnoreis spelled out at both top level and project level, because a project-leveltestIgnorereplaces the top-level one rather than merging with it.2.
tests/e2e/_base-url.ts— acceptsBASE_URLBASE_URLis the name the shared workflow actually exports, and it was the one spelling this resolver omitted. openconnector shipped aPLAYWRIGHT_BASE_URL-only resolver and its E2E job has hard-failed on every run since withPLAYWRIGHT_BASE_URL is not set.The
localhost:8080fallback is added only underCI/GITHUB_ACTIONS, where it is the runner's own throwawayphp -S. Off CI it still throws —:8080on a developer box is the shared dev container, and the no-default rule exists to protect it.3.
tests/e2e/ci-seed.sh(new) — explicit, forced, verified register importocc app:enable larpingappimports the register from anIRepairStep, which runs with no user session, so OpenRegister's RBAC denies it — and the repair step catches\Throwableand downgrades it to a warning, soocc app:enablestill exits0. The app enables cleanly, the SPA boots, and the register simply is not there.So the seed does the import explicitly over the admin HTTP API:
POST /index.php/apps/larpingapp/api/settings/reimport→loadSettings(force: true). Unlike OpenRegister's generic importer this goes throughConfigFileLoaderService, which merges thelib/Settings/register.d/*.jsonfragments into the base register first.\OCA\OpenRegister\AppHost\Routes::standard()fromappinfo/routes.php, so there is nosettings#loadroute here.settings/reimportis the real one.OCS-APIRequest: trueis load-bearing —reimport()has no#[NoCSRFRequired](removed deliberately in [HIGH] SettingsController::create and reimport POST endpoints have @NoCSRFRequired — admin CSRF-forgable #206), so without the header the POST is a CSRF failure.POST /apps/openregister/api/configurations/import(base file + each fragment) if that does not report an explicit"success": true. HTTP 200 alone is not sufficient:reimport()returns{"success": false}with a 200.larping_item,larping_event,larping_attendance— the bareitemslug collides globally with a foreign app's QTI schema, which is what used to make every fixture create 400.<type>_schemaids, so "the fixtures fell back to bootstrap literals and wrote into a foreign schema" has a name in the log.text/html, which every status-code check in the pipeline reads as success.4.
tests/e2e/workflows/fixtures.ts— the stat harness no longer assumes dockerrunPhpHarness()ranCharacterServiceviadocker exec … nextcloudwith the bootstrap hardcoded to/var/www/html/lib/base.php. CI has no docker daemon and no container callednextcloud, so the harness returnednull— which two tests treated astest.skip(reads as a pass) and the third as a failure blaming the stat arithmetic for an environment problem.It now locates the Nextcloud root by walking up to
lib/base.php+config/config.phpand runs the bootstrap in-process (same PHP binary andconfig.phpas thephp -Sinstance under test), falling back to docker on a developer box.The
test.skipescape hatch is now gated off on CI: there a null harness result must fail, rather than silently turning the two highest-value correctness assertions in this repo into a green-looking no-op.5.
additional-appsAdded, pinned to
ConductionNL/openregister@development. larpingapp is a thin client with no database tables of its own — every entity is an OpenRegister object, so without openregister there is nothing for the SPA to render or the fixtures to create. Pinned todevelopment(notmain) because that is the branch this app is developed against.What was NOT done
enable-psalm: falseleft as-is — not part of this change.enable-playwright-coverageleft off.playwright.config.ts,tests/e2e/docs-screenshots.spec.tsandtests/e2e/visual/**all left untouched.🤖 Generated with Claude Code