Skip to content

fix(e2e): 4 red specs — a stale window.confirm handler and a fixture that only existed on one laptop - #169

Closed
rubenvdlinde wants to merge 3 commits into
developmentfrom
test-debt/openbuild-e2e-newman
Closed

fix(e2e): 4 red specs — a stale window.confirm handler and a fixture that only existed on one laptop#169
rubenvdlinde wants to merge 3 commits into
developmentfrom
test-debt/openbuild-e2e-newman

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

E2E Tests (Playwright) has been red on development179 passed / 68 skipped / 4 unexpected (run 31386150604). Quality Report is red only because E2E is; it is a pure aggregator.

All four are tests that stopped matching the product, not product defects. Two independent causes.

1. Two specs still accept a native dialog that no longer exists

PR #163 replaced all seven window.confirm/prompt calls with real dialogs (gate-34). automations.spec.ts and spec-coverage/docudesk-document-templates.spec.ts still drove those flows with page.once('dialog', (d) => d.accept())a handler for an event that can no longer fire. Playwright reports nothing for a handler that never runs, so the click only opened the Vue dialog, nobody confirmed it, and the delete/detach never happened: Expected 0, Received 1 on a row that was never deleted.

That it went red at all was luck. Both specs happen to assert the item DISAPPEARS. A spec asserting "the list is unchanged" after a cancelled confirm, or merely checking for a toast, would have gone green over a click that did nothing. The new shared confirmAction() carries that whole reason in its docblock, and addresses the dialog by ACCESSIBLE NAME + the confirming control by ITS OWN LABEL — the row that opens the flow usually carries a button with the identical label.

Swept the suite: these were the only two page.on/once('dialog') handlers left.

2. automations-rbac.spec.ts assumed a fixture built by hand, once

The file's own comment said rbac-automations-app was "created via the wizard's dev-prod preset during this session's live-verification". Nothing in ci-seed.sh or global-setup.ts creates it, so on a runner the app-picker had no such option and the test spent its full 30s budget waiting for getByRole('option', { name: /rbac.?automations.?app/i })a missing fixture that reads exactly like an RBAC defect.

The first test failed for a second, independent reason: ci-seed.sh marks the first-visit overlays as seen for the admin only — deliberately, because pre-marking the rbac-* users would make non-admin-access.spec.ts pass without the product doing anything. So every rbac-* session is a first visit and gets CnSupportDialog's full-viewport backdrop. The combobox was "visible, enabled and stable" and the click still could not land.

beforeAll now builds the precondition through the same shared helpers every other fixture-owning spec uses: a two-version app (REQ-AUTD-008 is "editor may enable on a draft, is refused on production" — one version cannot express it), the role grants, and one disabled automation to enable.

Collateral corrections

  • ensureApp() takes the version list instead of hardcoding production.
  • grantAppRoles() takes owners. REQ-AUTD-008's second half is "an owner succeeds where the editor was rejected"; if that owner were the admin the success would come from the admin bypass and prove nothing about the grant.
  • Both rows are located by name. .first() asserted on the container — "some automation exists" — so the 403/200 pair could have been measured against an automation the spec never set up.
  • The owner browser.newContext() now says storageState: undefined explicitly. A context created inside a test inherits the config's root use.storageState (the shared admin session) — the shape that made an "anonymous visitor" test elsewhere a logged-in admin, green.
  • appRoles.ts carried a "role-scoped scenarios stay skipped" instruction citing Group-based app grants do not make an Application visible to its editors/viewers #76. That issue was closed 2026-08-01 — every openbuild schema declared no read, so OR's SQL gate discarded grants that had landed correctly; read: ["authenticated"] on all 15 schemas fixed it (measured 0 → 21). A stale blocker note is an active instruction to skip.
  • New collectFailedResponses() — deliberately a reporter, not a gate. A sibling repo lost a session to one 404 fired on every page (a probe for an optional ExApp, present on the dev container, absent in CI). Failing on any ≥400 would manufacture red over correct behaviour — this only makes the refusals available to name in an assertion message.

Bar

No waivers, no @e2e exclude, no .skip, no continue-on-error, no baselines, no timeout/threshold changes. npx tsc --noEmit clean on every touched file; playwright test --list collects 251 tests in 49 files.

Red direction already measured on run 31386150604 — each of the four failed with its specific cause. This run is the green direction.

…that only existed on one laptop

`E2E Tests (Playwright)` has been red on `development` with 4 unexpected
failures (179 passed / 68 skipped, run 31386150604). Two causes, both of
them a test that stopped matching the product rather than a product defect.

## 1. Two specs still accepted a native dialog that no longer exists (2 tests)

PR #163 replaced all seven `window.confirm`/`prompt` calls with real,
themable, translatable dialogs (gate-34). `automations.spec.ts` and
`spec-coverage/docudesk-document-templates.spec.ts` still drove those flows
with `page.once('dialog', (d) => d.accept())` — a handler for an event that
can no longer fire. Playwright reports nothing for a handler that never
runs, so the click merely opened the Vue dialog, nobody confirmed it, and
the delete/detach never happened: "Expected 0, Received 1" on a row that was
never deleted.

That it went red at all was luck. Both specs happen to assert the item
DISAPPEARS. A spec asserting "the list is unchanged" after a cancelled
confirm, or merely checking for a toast, would have gone GREEN over a click
that did nothing — so the new shared `confirmAction()` helper carries the
whole reason in its docblock. It addresses the dialog by ACCESSIBLE NAME and
the confirming control by ITS OWN LABEL, because the row that opens the flow
usually carries a button with the identical label.

## 2. `automations-rbac.spec.ts` assumed a fixture built by hand, once (2 tests)

The file's own comment said `rbac-automations-app` was "created via the
wizard's dev-prod preset during this session's live-verification". Nothing in
`ci-seed.sh` or `global-setup.ts` creates it, so on a runner the app-picker
had no such option and the second test spent its whole 30s budget waiting for
`getByRole('option', { name: /rbac.?automations.?app/i })` — a missing
fixture that reads exactly like an RBAC defect.

The first test failed for a second, independent reason: `ci-seed.sh` marks
the first-visit overlays as seen for the ADMIN ONLY (deliberately — pre-marking
the rbac-* users would make `non-admin-access.spec.ts` pass without the
product doing anything), so every rbac-* session is a first visit and gets
`CnSupportDialog`'s full-viewport backdrop. The combobox was "visible, enabled
and stable" and the click still could not land.

`beforeAll` now BUILDS the precondition through the same shared helpers every
other fixture-owning spec uses: a two-version app (REQ-AUTD-008 is "editor may
enable on a draft, is refused on production", which one version cannot
express), the role grants, and one disabled automation to enable.

## Collateral corrections

- `ensureApp()` takes the version list instead of hardcoding `production`.
- `grantAppRoles()` takes `owners`. REQ-AUTD-008's second half is "an OWNER
  succeeds where the editor was rejected"; if that owner were the admin, the
  success would come from the admin bypass and prove nothing about the grant.
- Both rows are now located BY NAME. `.first()` asserted on the container —
  "some automation exists" — so the 403/200 pair could have been measured
  against an automation the spec never set up.
- `browser.newContext()` for the owner session now says `storageState: undefined`
  explicitly. A context created inside a test inherits the config's root
  `use.storageState` (the shared admin session), which is how an "anonymous
  visitor" test elsewhere turned out to be a logged-in admin and stayed green.
- `appRoles.ts` carried a "role-scoped scenarios stay skipped" instruction
  citing openbuild#76. That issue was CLOSED 2026-08-01 (every openbuild schema
  declared no `read`, so OR's SQL gate discarded grants that had landed
  correctly; `read: ["authenticated"]` on all 15 schemas fixed it, measured
  0 -> 21). A stale blocker note is an active instruction to skip.
- New `collectFailedResponses()` — deliberately a REPORTER, not a gate. A
  sibling repo lost a session to one 404 fired on every page (a probe for an
  optional ExApp, present on the dev container and absent in CI). Failing on
  any >= 400 would manufacture red over correct behaviour, so this only makes
  the refusals available to name in an assertion message.

No waivers, no skips, no timeout changes.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ c2464b0

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
test-l10n
check-gitignore
check-nc-floor
composer ✅ 101/101
npm ✅ 654/654
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-10 14:42 UTC

Download the full PDF report from the workflow artifacts.

Run 31396466326 took this suite from 4 unexpected to 1, and the one left
was my own fixture rather than a spec:

  ensureDisabledAutomation(rbac-automations-app/production) — create
  failed: 400 "The required property (slug) is missing."

The Automation schema declares required: [slug, name, applicationSlug,
versionUuid, trigger] (lib/Settings/register.d/40-automations.json) and
the fixture payload omitted slug.

Worth noting the failure shape was the intended one: the helper threw
with the server’s own message naming the field, instead of the test
dying 30s later on a row that never appeared.

Measured on that run: automations.spec.ts delete and
docudesk-document-templates.spec.ts detach both GREEN (the stale
window.confirm handlers were the whole cause), 179 -> 181 passing.
The second RBAC test did not run — a failing beforeAll skips the
describe — so it is still unmeasured, not passing.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ a6cd06a

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
test-l10n
check-gitignore
check-nc-floor
composer ✅ 101/101
npm ✅ 654/654
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-10 15:13 UTC

Download the full PDF report from the workflow artifacts.

…es not accept

Run 31399749365: 181 passing, 1 unexpected — and the one left is still my
own fixture, not a spec. Third and (schema-checked) final correction:

  * `trigger.type` was 'event'. The enum is object-created | object-updated
    | object-deleted | lifecycle-transition | schedule | manual. Now
    object-created, with the target schema alongside it.
  * `actions[].type` was 'notification'. That enum is send-notification |
    run-synchronization | object-op | webhook | approval | generateDocument.
    Only `type` is required per item, so the fixture now carries the
    smallest schema-valid action — it exists to be ENABLED, not to act.
  * `condition: {}` is now OMITTED. OpenRegister rejects both `{}` and
    `null` for an object-typed property, and the schema restricts condition
    to the `manual` trigger in v1 anyway.

Read off lib/Settings/register.d/40-automations.json rather than guessed,
because each wrong guess costs a ~35-minute CI cycle. The three previous
rounds each failed on exactly one field, which is the shape the helper was
designed to produce: it throws with the server's own message naming the
offending property instead of timing out 30s later on a row that never
appeared.

⚠️ Worth noting for whoever meets it next: OR's rejection for the trigger
read "Property 'trigger.type' should be one of: , but is 'event'" — the
allowed list came back EMPTY. The enum is right there in the schema, so the
message is dropping it. A validation error that omits the allowed values is
one round-trip of guessing per field.

Unchanged and still true: the two window.confirm fixes are green, and the
SECOND RBAC test has still never executed — a failing beforeAll skips the
whole describe, so it is unmeasured, not passing.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ cfbc05c

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
test-l10n
check-gitignore
check-nc-floor
composer ✅ 101/101
npm ✅ 654/654
PHPUnit
Newman ⏭️
Playwright
Hydra gates

Quality workflow — 2026-08-10 15:52 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Status: 4 unexpected → 2, and the 2 left are a newly-exposed product question, not a test defect

Run 31402629015 (job 93501746124): 181 passed / 68 skipped / 2 unexpected. Baseline was 179 / 68 / 4 (run 31386150604).

Fixed and confirmed green

  • automations.spec.ts — REQ-AUTD-002 + REQ-AUTD-005 delete
  • spec-coverage/docudesk-document-templates.spec.ts — REQ-DDT-005 detach

Both were stale page.once('dialog', …) handlers waiting for a native dialog PR #163 removed. Both directions measured: red on 31386150604 with Expected 0, Received 1, green on 31396466326 and every run since.

The 2 remaining — automations-rbac.spec.ts

These have now genuinely run for the first time. Previously they could not: the fixture app existed only on one developer's container, and a failing beforeAll skipped the describe (visible as the skip count sitting at 69 instead of 68 in the intermediate runs — it is back to 68 now).

beforeAll succeeds. The app is created with both versions, and grantAppRoles() — which throws on a non-OK response — writes editors: ['user:rbac-editor']. Then, as that editor:

the rbac-automations-app fixture application must be listed for this user;
refused requests so far: ["GET 403 .../apps/openbuild/api/setup/status",
                          "GET 404 .../apps/hermiq/api/chat/health"]

.automations-page mounts, and /apps/openbuild/api/applications is not among the refusals — so it answered 200 with a list that does not contain the app. A user named explicitly in permissions.editors is not getting the Application back from the endpoint whose job is to return the Applications they hold a role on.

Filed as #171. That is a product question and I am not going to paper over it here — reaching for the admin session, or asserting on .first() option, would make this green while deleting the only evidence.

The response collector earned its place immediately

The two refusals above came from the new collectFailedResponses(). Without it this was "an option that never appeared". With it, two collateral findings fell out (both in #171):

  • GET 404 /apps/hermiq/api/chat/health on every page — a probe for an optional ExApp, present on the dev container, absent in CI. Precisely the shape that makes local screening green and CI red for reasons nobody can name.
  • GET 403 /apps/openbuild/api/setup/status for a non-admin — the endpoint whose 403 nc-vue's useSetupStatus used to read as "setup unfinished", showing non-admins a setup wizard instead of the app (fix(setup): a 403 from setup/status is an answer, not a failure nextcloud-vue#574).

Merge position

This PR is a strict improvement — it removes 2 of the baseline's 4 E2E failures and introduces no new failing cause; the 2 that remain are the same two tests that were already failing at baseline, now failing one layer deeper because they finally execute.

But E2E Tests (Playwright) is still red, so development stays red either way and there is no urgency in landing it red. I am leaving this for a human/coordinator decision rather than admin-merging over a red required check. Baseline for the comparison: run 31386150604 (completed, push on development, 31 jobs, 0 cancelled, failures = E2E + its aggregator).

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Overlap notice — cause 1 of this PR has landed separately as #172; cause 2 is still the valuable half

Apologies: I worked the same four failures without checking open PRs first, and merged #172 into development (875e30f) before finding this one. #172 covers only cause 1 — the two page.once('dialog', …) handlers, via a shared tests/e2e/support/confirmDialog.ts that asserts the dialog appeared before answering it and addresses it by accessible name. That is the same fix and the same reasoning as yours; yours was first, and this PR will now conflict on automations.spec.ts, spec-coverage/docudesk-document-templates.spec.ts and support/confirmDialog.ts. Rebasing and dropping cause 1 should resolve it.

Cause 2 is not covered by anything merged and is still needed. Current development (run 31431360507, sha 875e30f) measures 2 failed / 181 passed / 68 skipped — the two remaining failures are exactly the automations-rbac.spec.ts pair.

What your run tells us that is worth not losing

Your run 31402629015 is the only measurement anyone has of these tests with the fixture actually built, and it is more informative than the failure it reports. With rbac-automations-app created (dev+prod) and owners: ['user:rbac-owner'] / editors: ['user:rbac-editor'] granted, the tests still fail on

the rbac-automations-app fixture application must be listed for this user

i.e. rbac-editor does not see the app in /api/applications even though the grant was written. That is the #76 family reappearing — #76 fixed it for the seeded app by setting read: ["authenticated"] on the schemas; this is a wizard-created app, and something is still filtering it out. Worth reading the permissions block back from OR and diffing it against what ApplicationsController::filterApplicationsByRole() matches on before concluding fixture-vs-product.

I filed #173 for a second, independent blocker these tests would hit next: automation declares create/update/delete: ["admin"], AutomationEditDialog writes straight to OR, and AutomationsController::recompileAndRespond() calls saveObject() without _rbac: false — so an authorisation openbuild has already granted is refused one layer down and surfaces as a 500. I have corrected that issue to say your run shows the listing failure comes first.

Also worth stating plainly on the assertion you added: making the fixture's absence say "the fixture is not listed" instead of a bare 30s selector timeout is the change that produced this information at all. The previous message accused the app-picker.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Closing this without merging. Two of its three parts have been overtaken, and the third does not do what the title says.

1. The window.confirm half is already on development

#172 (merged 2026-08-10 20:55) landed the same fix for the same two files — automations.spec.ts and spec-coverage/docudesk-document-templates.spec.ts — via the shared confirmDialog.ts helper. This PR touches both files too, which is why it now reports CONFLICTING / DIRTY. Merging it would be merging a duplicate.

2. The rbac half does not fix the rbac specs — and its own run says so

Run 31402629015 (this PR's head) finished 181 passed / 2 failed. development at 2b1a8900 also reports 181 passed / 2 failed, and the two failures are the same two tests:

automations-rbac.spec.ts:335 › editor authors + enables an automation on a non-production (draft) version
automations-rbac.spec.ts:357 › editor gets 403 enabling on the production version; owner succeeds

So the net effect of this PR on the suite is zero. It cannot be merged on the grounds of making anything green.

3. What it did prove is worth keeping — moved to #173

The beforeAll fixture builder worked. The app was created, the grants were written, and the specs then failed later and for a different reason than the "fixture only exists on one laptop" story in the description:

Error: the rbac-automations-app fixture application must be listed for this user;
refused requests so far: ["GET 403 http://localhost:8080/apps/openbuild/api/setup/status", ...]

Locator: getByRole('option', { name: /rbac.?automations.?app/i }).first()
Expected: visible — element(s) not found
    at openAutomationsFor (tests/e2e/automations-rbac.spec.ts:286:4)

That is not the schema write-gate. rbac-editor cannot see the application in the picker at all — a read visibility failure that happens before any write is attempted. The _rbac / create/update/delete: ["admin"] theory is the second blocker, not the first. Recorded on #173 so the next attempt starts from the measured symptom rather than the predicted one.

The collectFailedResponses() reporter is what made that visible, and it is the piece worth re-landing on its own once #173 has an agreed fix.

Branch

test-debt/openbuild-e2e-newman is not deleted — it is unmerged and carries the fixture-builder work that a fix for #173 will want to start from.

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.

2 participants