test(e2e): make the delegation suites establish their own preconditions - #2901
Merged
Conversation
Running the three delegation suites live against merged `development` gave a
different answer three times over identical code — 18 passed, then 8 failed,
then 2 failed. Neither the code nor the assertions were wrong; the suites were
inheriting state instead of establishing it.
Two separate causes, both of which let a run report something it had not
measured:
1. A HARDCODED FIXTURE UID. `NEXTCLOUD_OTHER_USER || 'ddauth-alice'` named an
account that existed when the specs were written and did not exist after the
dev instance was rebuilt. The failure read
`"ddauth-alice" resolves to no account you may ask` — which is the delegation
guard's own refusal message, i.e. a dead fixture wearing the words of a
working control. The uid is now DISCOVERED from the instance, and a
single-account instance SKIPS with a sentence naming what went unverified
rather than failing for a reason that is not about the code.
2. A LEAKED GRANT. Every suite opens by asserting the save is REFUSED — the
baseline the later "now it saves" assertion is measured against. Grants
outlive a run, and a suite killed mid-way (a `head` closing the reporter's
pipe is enough) leaves a `granted` row behind, so the next run's baseline
got a cheerful 201. The failing direction was the lucky one: a leaked
REVOKED grant would have made the same baseline pass for the wrong reason
and the suite would have proved nothing. Each suite now revokes any live
grant over its target before asserting.
The account probe deliberately does NOT swallow errors. An earlier draft
wrapped it in `catch { return null }`, and when the probe came back as
Nextcloud's login page — HTTP 200, HTML, `.json()` throws — the catch reported
"no second account", eleven specs skipped citing a single-account instance, and
the run said `0 failed`. It now throws with the status and body, because a
probe that cannot answer must say so.
Also lets `NC_CONTAINER=nextcloud` through under an explicit
`NC_ALLOW_SHARED_CONTAINER=1`. The guard exists to stop an accidental default,
not to make the parking path — which only exists once a real TimedJob ticks —
permanently unverifiable.
Verified: 18 passed / 0 failed, twice back to back, against merged
`development` on a live instance.
The container guard refused the shared `nextcloud` container for every purpose,
and that was too blunt in both directions.
🔑 THE TWO ACTIONS ARE NOT THE SAME RISK, so they no longer share a rule.
`resolveContainer('exec')` — the default — now DEFAULTS TO the shared container
instead of returning null. Running one named `occ` command there is how the dev
box is meant to be exercised, and refusing to do so bought nothing: it made
every spec that needs a real TimedJob tick skip everywhere. The delegation
parking suite is the clearest casualty — a run parked on `awaiting_consent` and
released by a cron sweep only exists once a job actually runs, so the headline
behaviour of that subsystem was verified by nothing but a unit test, while the
summary said "3 skipped" in a tone indistinguishable from "3 passed".
`resolveContainer('restart')` still refuses the shared container without an
explicit `NC_ALLOW_SHARED_RESTART=1`. That is the action the original guard was
really about: `docker restart nextcloud` bounces an environment that bind-mounts
several developers' working trees, mid-session, with no warning to them. One
`occ` command is recoverable; restarting somebody else's instance is not.
`federated-config-store.spec.ts` is the only caller that restarts, and it asks
for that purpose explicitly.
Verified with NO container env set at all — the new default path:
26 passed, 5 skipped, 1 failed
All three delegation-parking tests now RUN and pass, including the park and the
release through real FlowScheduleWorker and FlowRunWorker ticks. The remaining
failure is pre-existing and unrelated: `federated-config.spec.ts` (like
`flow-schedule.spec.ts`) needs a `flows` REGISTER, which this rebuilt dev
instance never seeded — its beforeAll fails on the register lookup regardless of
any container setting.
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ⏭️ | ||||
| phpcs | ⏭️ | ||||
| phpmd | ⏭️ | ||||
| psalm | ⏭️ | ||||
| phpstan | ⏭️ | ||||
| phpmetrics | ⏭️ | ||||
| eslint | ⏭️ | ||||
| stylelint | ⏭️ | ||||
| build | ⏭️ | ||||
| composer | ⏭️ | ⏭️ | |||
| npm | ⏭️ | ⏭️ | |||
| app:check-code | ⏭️ | ||||
| info.xml | ⏭️ | ||||
| REUSE | ⏭️ | ||||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ❌ | ||||
| Hydra gates | ❌ |
Quality workflow — 2026-08-27 05:59 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ❌ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 545/545 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-27 07:15 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| check-specs | ✅ | ||||
| test-l10n | ✅ | ||||
| format | ✅ | ||||
| check-schema-l10n | ✅ | ||||
| check-l10n-js | ✅ | ||||
| composer | ✅ | ✅ 175/175 | |||
| npm | ✅ | ✅ 545/545 | |||
| app:check-code | ⏭️ | ||||
| info.xml | ✅ | ||||
| REUSE | ❌ | ||||
| PHPUnit | ✅ | ||||
| Newman | ✅ | ||||
| Playwright | ✅ | ||||
| Hydra gates | ✅ |
Quality workflow — 2026-08-27 09:18 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
The three delegation e2e suites gave three different answers over identical code when run back to back against merged
development— 18 passed, then 8 failed, then 2 failed. The assertions were right; the suites were inheriting state instead of establishing it.The two causes
1. A hardcoded fixture uid.
NEXTCLOUD_OTHER_USER || 'ddauth-alice'named an account that existed when the specs were written and stopped existing when the dev instance was rebuilt. The resulting failure read:That is the delegation guard's own refusal message. A dead fixture was wearing the words of a working control, and it cost a diagnosis. The uid is now discovered from
/ocs/v2.php/cloud/users; an instance with only one account skips with a sentence naming what went unverified.2. A leaked grant. Every suite opens by asserting the save is REFUSED — the baseline that the later "now it saves" assertion is measured against. Grants outlive a run, and a suite killed mid-way leaves a
grantedrow behind, so the next run's baseline got a cheerful201.The failing direction was the lucky one. Had the leak been a revoked grant rather than a granted one, the refusal baseline would have passed for the wrong reason and the suite would have proved nothing at all. Each suite now revokes any live grant over its target before asserting.
The probe does not swallow errors
An earlier draft of the account probe wrapped the request in
catch { return null }. When the probe came back as Nextcloud's login page — HTTP 200, HTML body,.json()throws — the catch reported "no second account": eleven specs skipped citing a single-account instance, and the run reported0 failed. An auth failure had put on a fixture's clothes.It now throws with the status and a body excerpt. A probe that cannot answer has to say so.
Also
NC_CONTAINER=nextcloudis let through under an explicitNC_ALLOW_SHARED_CONTAINER=1. The guard exists to stop an accidental default, not to make theawaiting_consentparking path — which only exists once a real TimedJob ticks — permanently unverifiable by anything but a unit test.Verification
Ran live against merged
developmentonlocalhost:8080:Two identical results back to back is the property that was missing.
The zero skips are the second commit's doing: with
execdefaulting to the shared container, the threedelegation-parkingtests now run rather than skip — the park onawaiting_consentand the release, driven by realFlowScheduleWorkerandFlowRunWorkerticks. Before this branch that path was verified by nothing but a unit test, while the summary said3 skippedin a tone indistinguishable from3 passed.Re-verified after merging current
developmentinto the branch.🤖 Generated with Claude Code
Second commit: the container guard now prefers the shared box
The guard refused the shared
nextcloudcontainer for every purpose, which was too blunt in both directions.The two actions are not the same risk, so they no longer share a rule.
'exec'(default)occcommand is how a dev box is meant to be exercised'restart'NC_ALLOW_SHARED_RESTART=1docker restartbounces an environment that bind-mounts several developers' trees, mid-sessionRefusing
execbought nothing and cost real coverage: every spec needing a TimedJob tick skipped everywhere. Theawaiting_consentpark-and-release only exists once a job runs, so the headline behaviour of that subsystem was verified by nothing but a unit test — while the summary said3 skippedin a tone indistinguishable from3 passed.federated-config-store.spec.tsis the only caller that restarts, and it now asks for that purpose explicitly.Verified with no container env set at all
All three
delegation-parkingtests now run and pass — the park and the release through realFlowScheduleWorker/FlowRunWorkerticks.One pre-existing failure, unrelated to this PR
federated-config.spec.ts(likeflow-schedule.spec.ts) resolves aflowsregister inbeforeAlland fails when it is absent — independently of any container setting. Flows themselves live inopenregister_flows; the register is imported by theImportFlowRegisterrepair step, which runs on install andocc upgrade. On an instance whoseinstalled_versionalready matchesinfo.xml,occ upgradereports "no upgrade required" and the repair step never runs, so the register is never materialised and both specs are dead on arrival. Worth a follow-up — it is an environment/seed gap, not a code change this PR should make.