Skip to content

fix(composer): make test:all a real gate — the guard made it incapable of failing - #2330

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/composer-test-guard-swallows-failures
Aug 4, 2026
Merged

fix(composer): make test:all a real gate — the guard made it incapable of failing#2330
rubenvdlinde merged 1 commit into
developmentfrom
fix/composer-test-guard-swallows-failures

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Problem

Every test:* script was wrapped in a guard that makes the script incapable of failing when phpunit is absent:

"test:all": "if [ -f vendor/bin/phpunit ]; then ./vendor/bin/phpunit --colors=always; else echo 'phpunit not installed, skipping...'; fi"

The guard is the defect, not a fix for one. Composer already prepends vendor/bin to PATH when it runs a script, so a plain ./vendor/bin/phpunit resolves whenever phpunit is installed. All the guard adds is: "you forgot composer install" becomes a silent green.

The amplifier is check:strict / check:full:

E=0; for CMD in lint phpcs phpmd psalm phpstan test:all; do composer $CMD || E=1; done; ... echo "ALL CHECKS PASSED"

A test:all that always exits 0 makes ALL CHECKS PASSED a claim about five tools and a no-op.

Measured, verbatim

Old form, no vendor/bin/phpunit present:

phpunit not installed, skipping...
composer test:all EXIT=0

New form, same conditions:

sh: 1: ./vendor/bin/phpunit: not found
Script ./vendor/bin/phpunit --colors=always handling the test:all event returned with error code 127
composer test:all EXIT=127

State of the suite before this change

Green. Measured on origin/development in a bare PHP 8.4.22 container (no Nextcloud), OPENREGISTER_TEST_NC_ROOT=/var/www/html:

Tests: 15964, Assertions: 35785, Warnings: 7, Deprecations: 31, PHPUnit Deprecations: 6, Skipped: 27.

15 964 tests were reachable and passing the whole time — the guard was not hiding failures here, it was standing between CI and a working gate.

Changes

test:unit and test:all now invoke phpunit directly. Flags and the --testsuite= variants are preserved exactly.

test:integration, test:db and test:service genuinely need a booted Nextcloud server tree — outside one they error wholesale (Database Tests 566/566 errors, Service Tests 1448/1454 errors, both TypeError: Cannot assign null to property …Mapper). Forcing those to hard-fail would just be a differently-shaped lie, so they keep a guard — but a self-documenting one that names the actual reason and the actual missing file, in the shape opencatalogi/composer.json already uses:

if [ ! -f ../../lib/base.php ]; then echo 'SKIPPED: the Database suite needs a booted Nextcloud server and a live database (../../lib/base.php not found) - run from inside a Nextcloud checkout or in CI'; else ./vendor/bin/phpunit --testsuite="Database Tests" --colors=always; fi

None of those three is in check:strict, so no aggregate gate is weakened by it.

Proof the gate can now fail

tests/Unit/BackgroundJob/CacheWarmupJobTest.php:222 flipped to assertTrue(false), then composer test:all:

1) Unit\BackgroundJob\CacheWarmupJobTest::testRunDoesNotRethrowException
Failed asserting that false is true.

/app/tests/Unit/BackgroundJob/CacheWarmupJobTest.php:222

FAILURES!
Tests: 15964, Assertions: 35785, Failures: 1, PHPUnit Warnings: 1, Warnings: 7, Deprecations: 31, PHPUnit Deprecations: 6, Skipped: 27.
Script ./vendor/bin/phpunit --colors=always handling the test:all event returned with error code 1
composer test:all EXIT=1

Reverted, re-run: EXIT=0, Tests: 15964. The assertion has been shown to be capable of both verdicts, and the count is non-zero — a suite that collects zero tests and exits 0 would be the worse outcome, so it is asserted explicitly here.

Separate finding, deliberately NOT changed

phpunit.xml sets defaultTestSuite="Unit Tests". test:all passes no --testsuite, so test:all runs only the Unit suite — it is a misnomer, not a full run. That is why its count is identical to test:unit's. Making it genuinely run all four suites would redden CI on ~2 030 environment-dependent tests, so it is left alone and flagged here for a separate decision.

Risk

Low. The suite is green, so CI does not change colour. What changes is that it can now change colour.

The 'if [ -f vendor/bin/phpunit ]' guard turned a missing vendor install
into an exit-0 green, so check:strict/check:full could print
ALL CHECKS PASSED without ever running a test. Composer already prepends
vendor/bin to PATH, so the guard protected nothing it should protect.

test:unit and test:all now invoke phpunit directly (15964 tests, green).
test:integration/test:db/test:service genuinely need a booted Nextcloud
server tree, so they keep a self-documenting guard that names the reason
instead of a generic 'skipping...'.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ b6a36e4

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
composer ✅ 173/173
npm ✅ 713/713
PHPUnit
Newman
Playwright

Quality workflow — 2026-08-04 09:19 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde

Copy link
Copy Markdown
Contributor Author

Merging with --admin so development stays the single source other agents build on — several are working in this fleet concurrently and divergence causes avoidable conflicts.

Verified at job level, not from the rollup: 0 failures, 0 cancelled, on a run from today. (A startup_failure reports jobs.total_count = 0 and emits no contexts, which is indistinguishable from success in a rollup — and a cancelled job is no verdict at all, so both were checked explicitly rather than inferred.)

@rubenvdlinde
rubenvdlinde merged commit da4d5af into development Aug 4, 2026
30 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/composer-test-guard-swallows-failures branch August 4, 2026 09:43
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