Skip to content

fix(tests): green the Unit suite — 51 errors + 2 failures to zero - #2175

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/pre-existing-unit-failures
Jul 28, 2026
Merged

fix(tests): green the Unit suite — 51 errors + 2 failures to zero#2175
rubenvdlinde merged 1 commit into
developmentfrom
fix/pre-existing-unit-failures

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

tests/Unit/ reported 51 errors and 2 failures on development. Exactly one was a production bug; the rest were harness faults — which is the real cost, because 53 permanent red results are noise that a genuine regression hides in.

before:  Tests: 15454, Errors: 51, Failures: 2
after:   Tests: 15454, Errors:  0, Failures: 0

25 errors — one test leaked a global

AttributeMcpDiscoveryTest::invokeRealSeam() did \OC::$server = $server and never put it back. The replacement is a bare OC_FakeServer subclass that overrides one method and registers no services, so every later test saw a locator whose get() returns null — and OCP\AppFramework\Http\Response::getHeaders() calls \OC::$server->get(IRequest::class)->getId() unconditionally.

That produced 21 × Call to a member function getId() on null, plus the getSystemValueBool() and findLanguage() variants. Every one of those tests passed in isolation, which is exactly what made this read as 21 broken tests rather than one leak.

Restored in tearDown() rather than at the end of the test, so a failing test cannot leak it either.

19 errors — no ContextChat stubs

OCP\ContextChat\* ships with the context_chat app, which is an optional seam here (ContentProvider implements its interface; ContextChatService resolves the manager lazily). Absent from a bare composer install, so the interface did not exist and every createMock(IContentManager::class) raised UnknownTypeException.

Added guarded stubs next to the existing Doriath ones, with the surface taken from the call sites rather than guessed: isContextChatAvailable / submitContent / deleteContent / registerContentProvider, IContentProvider, ContentItem, and the ContentProviderRegisterEvent that forwards to the manager.

4 errors — a constructor grew, its test did not

FlowRunController gained an IUserSession parameter; the test still passed five arguments, so every test in the class died with ArgumentCountError before reaching an assertion.

2 errors — a stub missing a method the code calls

ContactService calls CardDavBackend::getAddressBookById(); the stub never declared it, so mocking it raised MethodCannotBeConfiguredException.

1 error — bootstrap output corrupted an isolated worker

A @runInSeparateProcess test re-runs the bootstrap in a forked PHPUnit worker, and any output from that worker corrupts the channel PHPUnit reads the result back through — so the test failed with the bootstrap's own notice as its error message. The notice is now emitted once and the harness's existing OPENREGISTER_TEST_SKIP_NC switch is set for inherited children: humans still see the diagnostic, workers stay quiet.

1 failure — a real production bug

RelationsController's leaf-integration loop carries this comment:

…so a missing/disabled app is silently skipped and never breaks the core relations response.

It did not do that. \OCP\Server::get() can hand back null rather than throw, and calling the availability probe on null was caught and recorded in $errors — so a response where every requested lookup succeeded still carried an _errors key, for all 13 leaf integrations, visible to every consumer that walks the envelope's top-level keys.

Resolution is now separated from the call: failing to resolve a service is not an error to report; only a failure of an available service's own call is.

Verification

  • 15454 tests, 0 errors, 0 failures
  • phpcs clean on the one changed lib/ file
  • each fix verified in isolation before the full run, so the counts above are attributable rather than incidental

The suite reported 51 errors and 2 failures on development. Only one was a
production bug; the rest were harness faults that made real regressions hard to
see. Fixed in descending order of blast radius.

25 errors — ONE test leaked a global.
`AttributeMcpDiscoveryTest::invokeRealSeam()` assigned `\OC::$server` to a bare
OC_FakeServer subclass that overrides one method and registers NO services, and
never put it back. Every later test then saw a locator whose `get()` returns
null, and `OCP\AppFramework\Http\Response::getHeaders()` calls
`\OC::$server->get(IRequest::class)->getId()` unconditionally — so 21 tests died
with "getId() on null", plus getSystemValueBool() and findLanguage() variants.
They all passed in isolation, which is what made this look like 21 broken tests
instead of one leak. Restored in tearDown so a FAILING test cannot leak either.

19 errors — no ContextChat stubs.
`OCP\ContextChat\*` ships with the context_chat app, an optional seam here
(ContentProvider implements its interface, ContextChatService resolves the
manager lazily). Absent from a bare composer install, so the interface did not
exist and every createMock(IContentManager::class) raised UnknownTypeException.
Added guarded stubs alongside the existing Doriath ones, with the surface taken
from the call sites: isContextChatAvailable/submitContent/deleteContent/
registerContentProvider, IContentProvider, ContentItem, and the
ContentProviderRegisterEvent that forwards to the manager.

4 errors — a constructor grew, its test did not.
FlowRunController gained an IUserSession parameter; the test still passed five
arguments, so every test in the class died with ArgumentCountError before
reaching an assertion.

2 errors — a stub missing a method the code calls.
ContactService calls CardDavBackend::getAddressBookById(); the stub did not
declare it, so mocking it raised MethodCannotBeConfiguredException.

1 error — bootstrap output corrupted an isolated worker.
A `@runInSeparateProcess` test re-runs the bootstrap in a forked PHPUnit worker,
and ANY output from that worker corrupts the channel PHPUnit reads the result
back through — the test failed with the bootstrap's own notice as its error
message. The notice is now emitted once and the harness's existing
OPENREGISTER_TEST_SKIP_NC switch is set for inherited children, so humans still
see it and workers stay quiet.

1 failure — A REAL PRODUCTION BUG.
RelationsController's leaf-integration loop documents that "a missing/disabled
app is silently skipped and never breaks the core relations response". It did
not do that: `\OCP\Server::get()` can hand back NULL rather than throw, and
calling the availability probe on null was caught and recorded in `$errors`, so
a response where every requested lookup succeeded still carried an `_errors`
key — visible to every consumer that walks the envelope's top-level keys, for
all 13 leaf integrations. Resolution is now separated from the call: failing to
resolve a service is not an error to report, only a failure of an AVAILABLE
service's own call is.

15454 tests, 0 errors, 0 failures. phpcs clean on the changed lib file.
@rubenvdlinde
rubenvdlinde merged commit 13daf80 into development Jul 28, 2026
16 of 18 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/pre-existing-unit-failures branch July 28, 2026 06:03
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ f2c4745

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 174/174
npm ✅ 555/555
PHPUnit ⏭️
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-07-28 06:07 UTC

Download the full PDF report from the workflow artifacts.

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