Skip to content

ci(quality): let an app declare the Playwright front-controller preflight URL - #141

Closed
rubenvdlinde wants to merge 1 commit into
mainfrom
ci/playwright-preflight-path
Closed

ci(quality): let an app declare the Playwright front-controller preflight URL#141
rubenvdlinde wants to merge 1 commit into
mainfrom
ci/playwright-preflight-path

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Problem

The Playwright job's preflight probes /apps/<app-name>/ and treats HTTP 404 as proof that the php -S front controller is not routing pretty URLs:

::error::The php -S front-controller router is not routing pretty URLs —
/apps/nldesign/ returned 404. Every spec using a pretty Nextcloud URL will
fail on a selector timeout.

That inference is only valid for an app that actually serves a main page.

nldesign does not. Its appinfo/routes.php declares only /api/* and /settings/* routes, and its appinfo/info.xml declares a <settings> section with no <navigation> — it is an admin-settings-only theming app. /apps/nldesign/ is a correct 404. The preflight read that correct 404 as a broken front controller and exited 1 before a single spec ran, so enabling e2e on nldesign could never produce a verdict.

Observed live on ConductionNL/nldesign#206.

Change

Adds one input, playwright-preflight-path.

  • Default is empty, and empty falls back to /apps/<app-name>/.
  • So every existing caller probes byte-for-byte what it probed before. This cannot change behaviour for any repo that does not set it.

The failure message now also names the input, so the next app to hit this reads the actual cause instead of investigating a router that is working fine.

The OCS entry-point check immediately below is untouched. That check — /ocs/v2.php/cloud/user returning 401 (reached ocs/v2.php) versus 404 (swallowed into index.php) — remains the load-bearing proof that the router itself is functioning, and it is not weakened or made optional here. An app overriding the pretty-URL path still has to pass it.

Verification

Fallback semantics checked directly:

input probes
"" (default) /apps/<app-name>/
/settings/admin/nldesign /settings/admin/nldesign

YAML parses; input count 41; playwright-test-path and every other default unchanged. The diff against main is exactly two hunks.

Note on concurrency

This branch was rebased onto current main before pushing. An earlier attempt was based on the main I read at the start of the session and would have silently reverted #140 ("stop a phpmd finding from silently deleting the PHPUnit signal") — a contents PUT overwrites the whole file, so a stale base reverts anything that landed meanwhile without any conflict being reported. Verified after rebase: #140's change is intact on this branch.

Blocking

ConductionNL/nldesign#206 cannot get an E2E verdict until this lands, since callers reference quality.yml@main.

Not self-merging — this workflow is consumed by ~30 repos and main is left to a human.

…ight URL

The preflight probes /apps/<app-name>/ and treats HTTP 404 as proof the
php -S front controller is not routing pretty URLs. That inference only
holds for an app that actually serves a main page.

nldesign does not. Its routes.php declares only /api/* and /settings/*,
and info.xml declares <settings> with no <navigation> - it is an
admin-settings-only theming app. /apps/nldesign/ is therefore a CORRECT
404, and the preflight reported it as a broken front controller and
exited 1 before a single spec ran.

Add playwright-preflight-path so such an app can name a URL it really
serves. The default is empty and falls back to /apps/<app-name>/, so
every existing caller probes exactly what it probed before - this cannot
change behaviour for any repo that does not set it.

Also point the failure message at the new input. The OCS entry-point
check below is untouched and remains the load-bearing proof that the
router itself works.
@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Independent confirmation from tonight's fleet sweep: this is the sole remaining blocker on ConductionNL/nldesign#206, and the diagnosis here is correct.

Re-verified against the current run (nldesign 30879432992):

Front-controller check: /apps/nldesign/ -> HTTP 404
::error::The php -S front-controller router is not routing pretty URLs — /apps/nldesign/ returned 404.
::error::Process completed with exit code 1.

Checked against nldesign's appinfo/routes.php directly: it declares only /api/* and /settings/* routes and no root route, so /apps/nldesign/ is a correct 404. The job exits before a single spec runs, so those 37 spec files — 33 of them under spec-coverage/ — have still never executed once.

I also read the diff rather than taking the description's word for it. The behaviour claim holds: the input defaults to empty, empty falls back to /apps/<app-name>/, so every existing caller probes byte-for-byte what it probed before. The OCS entry-point check below it — 401 (reached ocs/v2.php) versus 404 (swallowed into index.php) — is untouched and remains the load-bearing proof that the router itself is functioning. This narrows an assertion that is wrong for one class of app; it does not weaken the preflight.

Not merging it, per your note that main here is left to a human and this workflow is consumed by ~30 repos. Flagging only that nldesign#206 is parked until it lands.

One incidental note in case it is useful for the same file later: nldesign only reaches the 404 at all because #206 installs OpenRegister via additional-apps. Without it the same probe returned HTTP 500 (Class "OCA\OpenRegister\AppHost\Controller\GenericHealthController" not found), because Nextcloud's router reflects over every registered controller while matching a route — so an unresolvable superclass takes down every route in the app, not just the one that declares it. Now filed as ConductionNL/nldesign#207. Not something this PR needs to handle; mentioned only because the preflight's error message was the thing that surfaced it, which is an argument that the message improvement in this diff is pulling real weight.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #145, which merged at 09:57 today and fixes the same failure more generally.

Both PRs target the same bug: the Playwright job proved the php -S front controller was routing by requesting /apps/<app-name>/ and failing on 404, which is a correct 404 for an app that legitimately serves no main page (nldesign, whose routes.php declares only /api/* and /settings/*). The gate read that as a broken router and killed the job before a single spec ran.

The difference is where the fix sits:

  • This PR adds a playwright-preflight-path input that each affected app must opt into. Every settings-only app in the fleet would have to discover the failure and set it.
  • fix(quality): the E2E front-controller gate blocked every app without an index route #145 changes the probe itself to /apps/files/ — a shipped, always-enabled app whose directory exists under the docroot with no index.php, which is the exact shape php -S hard-404s without a router. That discriminates "router working" from "router missing" for every app with no per-app configuration, and it still reports /apps/<app-name>/ as a ::notice:: for diagnostics.

With #145 in place the input this PR adds would be dead config — no caller needs it. That is also why the branch shows a conflict: both edit the same preflight block.

No work is lost by closing this; the branch stays on the remote.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant