test(phpunit): make the PHPUnit suite green in CI — 223 risky tests → 0 - #716
Merged
Merged
Conversation
…risky → 0 under coverage procest's PHPUnit legs had never run in CI. They were gated on `needs.php-quality.result != 'failure'`, and procest's phpmd leg has been red on 25 complexity findings, so `development` runs produced 26 jobs with a single greyed-out PHPUnit placeholder. ConductionNL/.github#140 removed that gate; the matrix expanded to 29 jobs and the suite failed on its first real run. The failure was invisible locally. `composer test:all` runs PHPUnit WITHOUT coverage, and all 1684 tests pass that way. CI runs `--coverage-clover`, and phpunit.xml sets `beStrictAboutCoverageMetadata="true"` + `failOnRisky="true"`. That combination only has teeth when a coverage driver is present: with covers metadata declared, PHPUnit marks a test risky if it executes production code that is listed neither as covered nor as used. 220 tests did. Every one of the 220 reported the same reason — "This test executed code that is not listed as code to be covered or used". None of them were missing `@covers`; they were exercising a collaborator the class docblock never declared: result value objects (ActionResult, GuardResult, BagLookupResult), domain exceptions (DecisionEvaluationException, HermiqAssistantException), the IntegrationMode enum, the SearchesObjects trait, and a handful of base classes and helper services. `@uses` is the correct declaration for all of them: it permits the collateral execution without attributing the code as covered, so reported coverage is unchanged (30.87% line coverage; the guard's baseline is 0.00 and passes). Nothing here weakens a check — `failOnRisky`, `beStrictAboutCoverageMetadata` and `--coverage-clover` are all untouched, and no test assertion changed. The risky set is execution-order-dependent: for a class whose only executed lines are load-time (enum cases, class constants), the attribution lands on whichever test loads it first, so consecutive local runs reported 227 and then 220. The declarations here were derived from the union of a full-suite run and a 245-file isolated sweep, then verified both ways: full suite Risky: 0 on two consecutive fresh-cache runs, and 0 risky pairs across all 245 files run in isolation. Tests: 1684, Assertions: 5614, Skipped: 5, Risky: 220 → 0. Exit 1 → 0.
The risky set is not identical between a bare PHP container and the CI leg, which installs a real Nextcloud server plus openregister before running. Two pairs appear in CI's 223 that no local ordering reproduced: TermijnPauseExtensionServiceTest -> Substitution\SubstitutedWorkResolver ZgwZtcRulesServiceTest -> TermijnbewakingSeedDataService Both were read off the failing `development` job log (91864313289) and diffed against the locally-derived set, so the declarations now cover the union of both environments.
rubenvdlinde
requested review from
WilcoLouwerse,
bbrands02 and
rjzondervan
as code owners
August 4, 2026 01:38
Contributor
Quality Report — ConductionNL/procest @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 550/550 | |||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-08-04 01:41 UTC
Download the full PDF report from the workflow artifacts.
… services (9 phpmd findings) (#717) * fix(quality): decompose Application bootstrap into per-subsystem registrars Application had a CouplingBetweenObjects value of 92 (threshold 13): its register()/boot() named ~90 listener, widget, adapter, middleware and service classes directly. Every registration now lives in a dedicated registrar under lib/AppInfo/Registrar/, so each subsystem's class references sit with that subsystem. Application keeps only the three phases (bind services, wire listeners, boot) and is down to five framework references. Also drops an accidental duplicate registration of VergunningaanvraagCreatedListener on ObjectCreatedEvent — it was registered twice under two different comments, so it ran twice per created object. phpmd: lib/AppInfo/Application.php CouplingBetweenObjects 92 -> cleared; repo total 25 -> 24 findings, no new finding anywhere in lib/. * fix(quality): split the VTH workflow-template seed into lookup, graph resolver and orchestration SeedVthWorkflowTemplates had an overall class complexity of 85 (threshold 50): one class owned the catalog walk, every OpenRegister read, the result-row shape coercion and the steps/transitions translation. - VthSeedLookup owns the three OpenRegister reads (caseType resolution, idempotency probe, statusType map) plus the system-principal elevation, with one soft-fail query() seam so 'missing service', 'unconfigured schema' and 'search threw' all collapse to the same empty result in one place. - VthSeedRowReader owns the result-row coercion (array / {results:[...]} / ObjectEntity). - VthWorkflowGraphResolver owns the steps/transitions translation and the deterministic UUID5 ids. - SeedVthWorkflowTemplates keeps orchestration only. The pre-existing class-level @SuppressWarnings(PHPMD.CouplingBetweenObjects) is removed: with the reads behind a seam the class no longer needs it (verified by measuring with the suppression gone). Also drops the phpstan ignoreErrors dependency for this code: the raw catalog step/transition params are now typed array<int, mixed> — which is what json_decode() actually yields — so the is_array() guards are real checks rather than PHPDoc-contradicting dead comparisons. phpmd: SeedVthWorkflowTemplates ExcessiveClassComplexity 85 -> cleared; repo total 24 -> 23 findings, no new finding anywhere in lib/. * fix(quality): split the StUF surface by direction — response builder, dispatcher, responder, inspector Three findings on the StUF stack came from one shape: single classes owning both directions of StUF-ZKN/BG at once. StufMessageBuilder (TooManyPublicMethods 14/10) owned inbound responses AND outbound requests. The two halves share no caller — the controller only ever answers, the adapter only ever asks — and not even an XML style (DOMDocument 'stuf:' vs string-concatenated 'zkn:'). The inbound half moves to Service/Stuf/StufResponseBuilder; the outbound half keeps the class and the canonical NS_* constants that StufMessageParser and StufController read from. StufController (ExcessiveClassComplexity 81/50) owned the inbound SOAP path, the async-confirmation webhook's envelope introspection and the admin REST surface: - StufSoapRequestDispatcher owns size ceiling, XXE-safe parsing and locating the StUF message element, answering every refusal with a SOAP Fault. - StufZknMessageResponder owns the per-message-type responses. - StufEnvelopeInspector owns the raw-envelope reads the webhook needs (endpoint identity, WSSE verification, bericht-soort / crossRef / functie), behind one firstMatch() helper that also collapses five identical PHPMD.UndefinedVariable suppressions into one. StufController keeps its pre-existing class-level CouplingBetweenObjects suppression: measured with it removed the class still reads 14/13, and that finding was not in scope here. Tests: StufMessageBuilderOutboundTest's inbound assertions now target StufResponseBuilder, widened to cover Fo01 and stuurgegevens, plus a new negative control asserting the inbound builders are GONE from StufMessageBuilder rather than duplicated. phpmd: StufController 81 -> cleared, StufMessageBuilder 14 public methods -> 8, repo total 23 -> 21 findings, no new finding anywhere in lib/. * fix(quality): split StufAdapterService into orchestration, transport and mapping store StufAdapterService carried both findings at once: overall complexity 51 and a coupling of 15. It built envelopes, sent them, classified every possible answer, drove the circuit breaker, scheduled retries and owned the case-to-zaak mapping. - StufOutboundTransport owns everything that happens AFTER an envelope is built and logged: send, 2xx bevestiging, Fo02/transport fout classification, retry scheduling and the permanent-failure needs-input signal. The backoff schedule moves with it, next to the only code that reads it. - StufCaseMappingStore owns the ZaaksysteemMapping row and the identity triple (bronEntiteit, bronId, endpointId) that used to be spelled out at three call sites. - StufAdapterService keeps WHAT to send and what to report back. Removes a DEAD suppression: the @SuppressWarnings(PHPMD.CouplingBetweenObjects) sat in the FILE docblock, not the class docblock, so PHPMD never applied it — which is exactly why the coupling finding was reported despite it being there. The coupling is now decomposed away rather than suppressed. Public surface is unchanged (creeerZaak / actualiseerZaak / geefZaakDetails / vrijBericht / genereerZaakIdentificatie / retrySend / RETRY_BACKOFF_SECONDS). phpmd: StufAdapterService complexity 51 -> cleared and coupling 15 -> cleared; repo total 21 -> 19 findings, no new finding anywhere in lib/. * fix(quality): decompose AiService into prompt factory, PII redactor, endpoint guard and audit log AiService was 1265 lines (threshold 1000). It orchestrated AI calls AND owned four self-contained concerns: - AiPromptFactory: the six prompt templates, so the wording of what we ask a model and the JSON shape we demand back are reviewable as a set. - AiPiiRedactor: the ONE definition of deterministically-detectable PII plus both of its consumers (span reporting and prompt scrubbing), so detection and scrubbing can never drift apart on which patterns count. - AiEndpointGuard: the SSRF decision — CIDR deny-list, cloud/local rules, IPv4 and IPv6 range arithmetic. - AiAuditLog: the Algoritmeregister oversight trail, with the write and the read resolving the same register/schema config in one place and degrading in one place. AiService keeps orchestration and the single outbound model call. It no longer needs the SearchesObjects or SuppressesWarnings traits, nor the DI container. Tests wire the REAL collaborators against the existing mocked boundaries rather than stubbing them out, so AiServiceAuditLoggingCompletenessTest still proves an audit entry is actually written and AiServicePiiDetectionTest still exercises the real pattern set. phpmd: AiService ExcessiveClassLength 1265 -> cleared; repo total 19 -> 18. TooManyPublicMethods (12/10) is NOT cleared — see the PR body. * fix(quality): decompose SettingsService — fragment merger, slug map, two reconcilers SettingsService was 1356 lines (threshold 1000). Alongside the settings CRUD it owned the ADR-037 fragment merge, the schema-slug data table, and both schema reconcilers. - RegisterFragmentMerger owns the ADR-037 deep-merge and the fragment-set hash that forces a re-import. It is a pure data transformation, so it is now instance-based and PUBLIC — the six fragment test suites that used to reach it through ReflectionMethod on a private static now just call it. - SchemaSlugMap holds the slug -> appconfig-key table and the owned x-openregister-* annotation block names, plus the workflow_definition_schema alias constants that were previously repeated as string literals at two call sites. - SchemaKeyReconciler owns both paths that write a *_schema key: the import result and the direct SchemaMapper slug lookup. - SchemaAnnotationReconciler owns the declarative x-openregister-* merge onto live schemas. Public API and the (appConfig, appManager, container, logger) constructor signature are unchanged, so the bespoke factory and the ~180 injection sites are untouched; the collaborators are constructed internally. The pre-existing class-level @SuppressWarnings(PHPMD.ExcessiveClassComplexity) is removed — verified by measuring with it gone. phpmd: SettingsService ExcessiveClassLength 1356 -> cleared (1356 -> 810 lines); repo total 18 -> 17 findings, no new finding anywhere in lib/.
* ci(e2e): turn on the E2E Tests (Playwright) job Four wrong inputs in the caller, plus the four artifacts the shared workflow needs to run the suite honestly. Caller (.github/workflows/code-quality.yml): - additional-apps pinned OpenRegister to `feature/php-linting`, a short-lived quality branch. The checkout step does `git clone --depth 1 --branch "$ref"`, so that pin fails outright the moment the branch is merged or deleted, and until then it makes every CI instance behave unlike any environment procest is developed against. Moved to `development`, where the AppHost route table procest's own appinfo/routes.php depends on lands first. - enable-playwright: false -> true. The recorded blocker (nc-vue #242 CnObjectDataWidget bundling) is met: package.json pins 2.1.0-vue3.16 and `npm run build` emits js/procest-main.js. Kept the old comment rather than deleting the reasoning. - playwright-test-path: tests/e2e added. It selects BOTH the spec directory and the config; without it the run step falls back to the ROOT config, which passes no --project and would therefore also run `docs-capture` (re-shooting every documentation screenshot on every PR) and `visual` (whose README records that a CI Linux runner cannot byte-match a dev-container PNG baseline). - playwright-seed-command was `php occ maintenance:repair`. That is the IRepairStep path and it CANNOT provision this register: a repair step runs with no user session, OpenRegister RBAC denies the import, Repair\InitializeSettings::run() catches the Throwable and downgrades it to a warning, and occ still exits 0. The register is absent, the app looks fine, and every fixture call then 404s. Artifacts: - tests/e2e/playwright.config.ts — CI-only, chromium ONLY, testDir __dirname, globalSetup at the existing global-setup.ts, report and traces under tests/e2e/ (both paths the workflow uploads). testIgnore is repeated at project level because a project-level list REPLACES the top-level one rather than merging. The ROOT config is untouched, so the Journeydoc Capture job still finds its `docs-capture` project. - tests/e2e/base-url.ts — resolver accepting PLAYWRIGHT_BASE_URL, NEXTCLOUD_URL, NC_BASE_URL and BASE_URL (the name the shared workflow actually exports), defaulting to localhost:8080 only under CI and THROWING otherwise. The previous `NEXTCLOUD_URL || localhost:8080` fallback in the root config, global-setup and six spec files pointed at the SHARED dev container off CI — and this suite both seeds and deletes OpenRegister objects. global-setup and the four affected specs now use the resolver. - tests/e2e/ci-seed.sh — forced import over the admin HTTP API (POST /apps/procest/api/settings/load -> loadConfiguration(force: true), which also deep-merges the 20 lib/Settings/register.d/*.json fragments), with the generic OpenRegister importer as a degraded fallback. HTTP 200 is treated as necessary but not sufficient: settings#load returns {"success": false} with a 200. It then VERIFIES the register slug `procest` and the schema slugs case, caseType, statusType, workflowTemplate, task and complaint — all read out of lib/Settings/procest_register.json, not kebab-cased from a display name — probes the object collections the fixtures assert res.ok() on, warms the SPA, and finally GATES on the bundle serving as real JavaScript of non-trivial size. A missing bundle returns HTTP 200 text/html, never 404, so a status-code check alone reads it as success; global-setup's ensureBundleBuilt() only does existsSync and would accept a zero-byte file. - .gitignore entries already present at both levels (root /test-results/ and /playwright-report/; tests/e2e/.gitignore .auth/, test-results/, playwright-report/), so no artifact can be committed. 122 tests in 30 files are collected; docs-screenshots and visual/ are excluded and their projects remain available from the root config. * ci(e2e): TEMPORARY diagnostic spec to capture what CI actually renders * ci(e2e): diagnostic round 2 — deep-link vs group-expansion * fix(e2e): deep-link navigation, bounded action timeout, correct stale selectors The E2E job was cancelled at the shared workflow's 45-minute cap having run only 65 of 122 tests. Root cause was navigation, not the environment. Measured on a CI runner: the app mounts cleanly (Vue mounted, 31 nav links, dashboard header + widgets render, zero procest 4xx). But most nav leaves sit inside COLLAPSED groups, so they are display:none on load. navTo() clicked them, and with no actionTimeout configured each click blocked for the whole 60s test budget before failing with a bare timeout naming an element rather than the cause. - helpers/nav.ts: resolve the label to its href and navigate directly. Direct deep links DO render their view — the long-standing comment claiming they reset the router to the Dashboard is false, measured on /cases, /my-work, /doorlooptijd and /tasks. Unknown labels now throw immediately and list the available ones instead of silently asserting the Dashboard. - playwright.config.ts: actionTimeout 15s, navigationTimeout 30s. - navigation.spec.ts: 'My Work' -> 'My work', 'All cases' -> 'Cases', hrefs carry the /index.php prefix; collapsed-group leaves asserted by presence rather than visibility. - pages.spec.ts: the view switcher renders as buttons, not a radio group (the route exposes zero radio roles); My Work renders no heading. * ci(e2e): diagnostic round 3 — probe every failing route * fix(e2e): navigate by route, correct stale labels and selectors Follow-up to the navigation fix, driven by a full CI run (122 tests: 31 passed, 51 failed, 30 skipped) plus a route-probe diagnostic that dumped what every affected page actually renders on a runner. Root causes, all measured — not one of them was an environment fault: 1. Deep links WITHOUT the /index.php prefix do not render the target view; with it they do. Several comments in this suite asserted the opposite, and ~12 specs deep-linked the un-prefixed form. Fixed throughout. 2. The settings nav was translated to English, so specs clicking Dutch/legacy labels ('Parafeerroutes', 'Kaartlagen', 'Tenants', 'Automatische acties', 'Handhavingsstrategie') matched nothing. Those pages now navigate by ROUTE, which is the stable contract, instead of by menu string. 3. Several pages have no nav entry at all in this build (Advice, Voorstellen, Bezwaren, Beroepen, Subsidies) — also switched to routes. 4. The view switcher renders as BUTTONS, not a radio group; every getByRole('radio', …) assertion was unsatisfiable. 5. AVG + initiator specs used hash routes (#/verwerkingen, #/) against a history-mode router, so the view never rendered. 6. procest probes /apps/hermiq/api/chat/health on load; that 404s by design when hermiq is absent and leaked into every trackProcestErrors assertion. Filtered by request URL (the console text carries no URL). 7. getByRole cannot see collapsed-group nav leaves at all — display:none removes them from the accessibility tree — so presence is asserted by DOM. 8. Case Map renders 'Cases on map'; 'Case map' is the manifest page title. 9. pages.spec Settings asserted 'Version Information' / 'Re-import configuration', which exist nowhere in src/ — that surface was removed. AdminRoot mounts sections lazily, so scroll them in first. Four brp-kvk-initiator tests are test.fixme(#718): they assert BRP/KvK personas that ci-seed.sh does not provision, so they cannot pass hermetically. * fix(e2e): route-navigate bezwaar family, correct My Work + AVG assertions Second pass, from a full CI run that went 31 passed / 51 failed / 30 skipped -> 70 passed / 17 failed / 28 skipped in 26.2m (was 38.4m). - bezwaar-family: navigate /beroepen and /settings/bezwaar-committees by route; the nav renders 'Appeals' and 'Objection advisory committees' and keeps both inside collapsed groups. The committees create control is 'Add Objection Advisory Committee', never the Dutch label. - my-work + handler-vervanging: the My Work route renders NO heading at all, so getByRole('heading', /My Work/) could never pass. Assert the sort controls unique to that view instead. - pages.spec Tasks: the index sidebar starts collapsed, so its search field is present but hidden — assert it is attached rather than visible. - avg: '/apps/procest/api/avg/verwerkingen' WITHOUT the /index.php prefix never reaches Nextcloud's router; it is served the app shell HTML with status 200. This assertion was measuring the wrong thing entirely. - admin-settings + case-types-tabs: test.slow(). The NC admin page mounts fourteen OpenRegister-backed sections and was measured at ~50s under the CI php -S server, overrunning the 60s default intermittently — two tests failed with a bare timeout while their identical siblings passed. * ci(e2e): diagnostic round 4 + fix avg/semantic/initiator specs * test(e2e): quarantine eight product-gap specs (#719) + add positive control Eight remaining failures are gaps in the app or CI fixture data, not test mechanics. Each is test.fixme with its measured reason inline and is written up in #719: - in-app /settings renders no .settings-form and no Case Type Management heading, and has NO scrollable container, so it is not lazy-mount timing — the type:"settings" section-admin slot never renders its body (x2) - admin 'add case type' form never surfaces Save, even at test.slow()'s 180s - case-email 'Test connection' never renders though its sibling test loads the same page - /cases renders neither table nor cards on an unseeded list, so the deelzaak badge assertion has nothing to attach to - /subsidies falls back to the generic case index ('Add Case'), so there is no subsidy intake shell - the case DETAIL page never displays the zaaknummer, though the LIST does (x2) Also adds a deliberately-failing positive control. The shared Playwright job had never recorded a success anywhere in the fleet, so a green from it has never been distinguishable from a job that cannot report. This run should go RED naming exactly this spec — which also demonstrates every other test passes — and the control is removed in the next commit. * test(e2e): remove positive control; fix AVG endpoint proof and admin timeouts Positive control observed: run 30883... went RED naming 'zz-positive-control.spec.ts:15:5 › POSITIVE CONTROL' while 81 other tests passed, so the gate demonstrably reports failure and is not merely incapable of going red. Control removed. Two real fixes from that same run: - avg 'procest exposes no processing-log endpoints of its own' asserted 404/405, but procest registers an SPA catch-all (/{path} -> dashboard#catchAll via Routes::standard()), so EVERY unmatched path under /apps/procest returns the shell with HTTP 200. The assertion could never pass — and widening it to accept 200 would have made it green while proving nothing, since 200 is exactly what a non-existent route returns. Assert the response is not JSON instead: an AVG log endpoint would answer JSON, the catch-all serves HTML. (procest's routes.php registers no avg routes, so the spec's premise holds; only the method was wrong.) - the NC admin settings page load is highly variable under php -S (~7s to 3.2m across runs) and overran even test.slow()'s tripled 180s once. Set an explicit 300s budget on both admin describes instead.
…risky → 0 under coverage procest's PHPUnit legs had never run in CI. They were gated on `needs.php-quality.result != 'failure'`, and procest's phpmd leg has been red on 25 complexity findings, so `development` runs produced 26 jobs with a single greyed-out PHPUnit placeholder. ConductionNL/.github#140 removed that gate; the matrix expanded to 29 jobs and the suite failed on its first real run. The failure was invisible locally. `composer test:all` runs PHPUnit WITHOUT coverage, and all 1684 tests pass that way. CI runs `--coverage-clover`, and phpunit.xml sets `beStrictAboutCoverageMetadata="true"` + `failOnRisky="true"`. That combination only has teeth when a coverage driver is present: with covers metadata declared, PHPUnit marks a test risky if it executes production code that is listed neither as covered nor as used. 220 tests did. Every one of the 220 reported the same reason — "This test executed code that is not listed as code to be covered or used". None of them were missing `@covers`; they were exercising a collaborator the class docblock never declared: result value objects (ActionResult, GuardResult, BagLookupResult), domain exceptions (DecisionEvaluationException, HermiqAssistantException), the IntegrationMode enum, the SearchesObjects trait, and a handful of base classes and helper services. `@uses` is the correct declaration for all of them: it permits the collateral execution without attributing the code as covered, so reported coverage is unchanged (30.87% line coverage; the guard's baseline is 0.00 and passes). Nothing here weakens a check — `failOnRisky`, `beStrictAboutCoverageMetadata` and `--coverage-clover` are all untouched, and no test assertion changed. The risky set is execution-order-dependent: for a class whose only executed lines are load-time (enum cases, class constants), the attribution lands on whichever test loads it first, so consecutive local runs reported 227 and then 220. The declarations here were derived from the union of a full-suite run and a 245-file isolated sweep, then verified both ways: full suite Risky: 0 on two consecutive fresh-cache runs, and 0 risky pairs across all 245 files run in isolation. Tests: 1684, Assertions: 5614, Skipped: 5, Risky: 220 → 0. Exit 1 → 0.
The risky set is not identical between a bare PHP container and the CI leg, which installs a real Nextcloud server plus openregister before running. Two pairs appear in CI's 223 that no local ordering reproduced: TermijnPauseExtensionServiceTest -> Substitution\SubstitutedWorkResolver ZgwZtcRulesServiceTest -> TermijnbewakingSeedDataService Both were read off the failing `development` job log (91864313289) and diffed against the locally-derived set, so the declarations now cover the union of both environments.
… pairs The first PR run was still red — 223 risky had dropped to 49, not 0 — and the reason was a stale base, not a wrong fix. A `pull_request` workflow builds `refs/pull/N/merge`, i.e. this branch merged into the CURRENT development. Five PRs (#712 #713 #714 #715 #717, plus #709) landed while this was in flight, and they split several services into new sub-namespaces. CI was therefore measuring a tree with collaborators that did not exist on the base this branch was cut from — Service\Relation\*, Service\Sharing\*, Service\Transfer\*, Service\Email\*, Service\Settings\*, Service\Ai\*, Service\Cmmn\CasePlanRepository, PlanItemCascade, PlanItemStateMachine, PlanItemTree, Consultation\*, Zaakdossier\InformatieobjectStatusLifecycle, Beschikking\LibresignResultAssembler. Rebased onto 53670a0 and re-measured from scratch. 41 residual pairs across 21 test files, declared here. The measurement now agrees across all three sources: the per-file isolated sweep and the full-suite run produced an identical 41 pairs, and every one of CI's 20 was contained in them — the CI-only set is empty this round, which confirms the earlier local/CI divergence was entirely the stale base and not an environment difference. Tests: 1686, Assertions: 5632, Skipped: 5, Risky: 82 -> 0 locally. Exit 0.
The pre-rebase commits are fully subsumed by the rebased work: both declared the same annotations, and the rebased tree additionally covers the 41 pairs that only exist on the current development base. Recorded as a merge so the push stays fast-forward (force-push is blocked by the fleet guard); the tree is the rebased one.
rubenvdlinde
requested review from
Rem-Dam,
SudoThijn and
remko48
as code owners
August 4, 2026 05:11
Two deliberate defects, to prove the four PHPUnit legs can actually go red: 1. RoleGuardTest::testFallsBackToGroupMembership asserts a bogus matchedRole, so a genuine assertion failure must surface and name the test. 2. ChecklistGuardTest loses its @uses GuardResult declaration, so beStrictAboutCoverageMetadata + failOnRisky must re-flag it as risky. If the legs stay green with this commit in place, the suite is not testing what it claims to and the green on the previous commit means nothing. This commit is reverted in the next one.
Contributor
Quality Report — ConductionNL/procest @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 550/550 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ |
Quality workflow — 2026-08-04 05:18 UTC
Download the full PDF report from the workflow artifacts.
This reverts commit 6b2668b.
Contributor
Author
Positive control — evidenceThe green on this branch is not vacuous. Commit Defect 1 — a genuine assertion failure ( All four legs went RED, naming both (run
|
Contributor
Quality Report — ConductionNL/procest @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 550/550 | |||
| PHPUnit | ❌ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ |
Quality workflow — 2026-08-04 05:23 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/procest @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 550/550 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ❌ |
Quality workflow — 2026-08-04 05:47 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Quality Report — ConductionNL/procest @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ❌ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| build | ✅ | ||||
| composer | ✅ | ✅ 100/100 | |||
| npm | ✅ | ✅ 550/550 | |||
| PHPUnit | ✅ | ||||
| Newman | ⏭️ | ||||
| Playwright | ✅ |
Quality workflow — 2026-08-04 06:05 UTC
Download the full PDF report from the workflow artifacts.
Contributor
Author
Final CI state — commit
|
development (run 30876345083) |
this PR | |
|---|---|---|
PHP Quality (phpmd) |
fail | fail (identical: same script, same exit code 2) |
Quality Report |
fail | fail (downstream of phpmd) |
PHPUnit × 4 |
fail | pass |
| everything else | pass | pass (24 green) |
This PR strictly improves the set: the only two remaining failures are exactly
development's own pre-existing ones, and the diff touches no lib/ file, so phpmd cannot
differ.
E2E Tests (Playwright) — flaky, not caused by this change
The first run failed one spec, admin-settings.spec.ts:56 ("case type list renders its
management surface and add control"), timing out at 5.2m on both the initial attempt and the
retry, with 82 other specs passing. Re-running the job passed. The job was only turned on
last night by #709 and has exactly one prior green data point on development, so this looks
like a new flake in that spec rather than anything here — this PR's diff is 87 files of
@uses docblocks under tests/Unit/, and Playwright reads tests/e2e/.
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
2 participants
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.
Why this only surfaced now
procest's PHPUnit legs had never run in CI. The shared workflow gated them on
needs.php-quality.result != 'failure', and procest'sphpmdleg has been red — sodevelopmentruns produced 26 jobs with a single greyed-out PHPUnit placeholder, whichreads like a non-event rather than a gate that stopped running.
ConductionNL/.github#140removed that gate; the matrix expanded to 29 jobs and thesuite failed on its first real execution.
Baseline on
development(job91864313289), all four legs red:Two dead-gate causes were stacked. The tests could not run — and had they run, they would
have failed for a reason nobody could reproduce locally:
composer test:allruns PHPUnitwithout coverage, and every test passes that way. CI runs
--coverage-clover, andphpunit.xmlsetsbeStrictAboutCoverageMetadata="true"+failOnRisky="true". That paironly has teeth when a coverage driver is present.
What the 223 risky tests actually were
All 223 reported the same reason:
None of them were missing
@covers. Every one already declared its subject and wasadditionally executing a collaborator the class docblock never mentioned:
Transitions\ActionResult,Transitions\GuardResult,External\Bag\BagLookupResultDmn\DecisionEvaluationException,Assistant\HermiqAssistantExceptionExternal\IntegrationModeSupport\SearchesObjectsZgwRulesBaseInformatieobjectAccessGuard,CaseAccessGuard,Cmmn\CaseModelLoader@usesis the correct declaration: it permits the collateral execution without attributingthe code as covered.
What this change does not do
failOnRisky,beStrictAboutCoverageMetadataand--coverage-cloverare untouched. Notest assertion, fixture or production file changed — the diff is 87 files, +249 lines, all
docblock annotation under
tests/Unit/. Flipping any of those three settings would haveturned the suite green while testing exactly as much as before.
Reported coverage is unchanged:
@usescode is excluded from the report exactly as unlistedcode already was.
Two measurement traps worth recording
1. The risky set is execution-order-dependent. For a class whose only executed lines are
load-time (enum cases, class constants), the attribution lands on whichever test loads it
first. Change the order and a different test is flagged. Consecutive local runs reported
227 and then 220; CI reported 223. So the declarations were derived from the union of a
full-suite run and a 245-file isolated sweep (each file run alone, so load-time lines are
attributed to that file — an order-robust superset).
2. A
pull_requestrun buildsrefs/pull/N/merge, not your branch. The first run of thisPR was still red at 49 risky, and the cause was a stale base, not a wrong fix. #709, #712,
#713, #714, #715 and #717 landed while this was in flight and split several services into new
sub-namespaces (
Service\Relation\*,Service\Sharing\*,Service\Transfer\*,Service\Email\*,Service\Settings\*,Service\Ai\*, four newService\Cmmn\*classes,Consultation\*,Zaakdossier\InformatieobjectStatusLifecycle,Beschikking\LibresignResultAssembler). CI was measuring collaborators that did not exist onthis branch's base. After rebasing onto
53670a0bfand re-measuring, the per-file sweep andthe full-suite run produced an identical 41 residual pairs and every one of CI's 20 was
contained in them — the CI-only set is empty, confirming the divergence was entirely the
stale base.
Result — all four legs green, from the CI logs
Risky 223 → 0. No
Risky:term remains in any leg's summary line.Positive control
A deliberate-break commit was pushed and then reverted, to prove the green is not vacuous.
It carried two defects at once: a bogus assertion in
RoleGuardTest::testFallsBackToGroupMembership, and the removal of one@usesdeclarationfrom
ChecklistGuardTest— so the run had to demonstrate both that a genuine failurepropagates and that
beStrictAboutCoverageMetadata+failOnRiskystill bite. Evidence isin the PR conversation.
Notes for reviewers
developmentindependently failsPHP Quality (phpmd)andQuality Report. Pre-existing,out of scope — this PR touches only
tests/Unit/**docblocks, whichphpcs/psalm/phpstando not scan (all three are scoped tolib/)..coverage-baselineis0.00, soscripts/coverage-guard.phpcan never fail. CI now measures 29.48% line coverage — tonight is the first time that
number has ever been produced, which is the prerequisite for setting a real baseline. Left
unchanged here to keep this PR single-purpose.