fix(ci): make composer check:strict able to fail on tests - #15
Merged
Conversation
test:unit and test:all ended in ./vendor/bin/phpunit --colors=always || echo 'No PHP test suite in this repo ... skipping...' so phpunit's exit status was discarded unconditionally. The message was true today (this repo is a Python ExApp wrapper with no PHP test suite) but the mechanism was not conditional on it: any real test failure, now or after a test suite is added, would have been swallowed the same way. Replaced with a guard on the actual precondition, so an absent suite is a visible stated fact and a present-but-failing suite fails the gate: if [ -f phpunit.xml ] || [ -f phpunit.xml.dist ]; then ./vendor/bin/phpunit --colors=always; else echo 'SKIPPED: no phpunit.xml ...'; fi Positive control, measured on this tree in a PHP 8.3.32 container with a freshly installed vendor/, injecting a phpunit.xml plus one deliberately failing test: old composer.json + failing test -> check:strict exit 0 (swallowed) new composer.json + failing test -> check:strict exit 1, naming test:all new composer.json, test removed -> check:strict exit 0, loud SKIPPED Tooling only.
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
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.
Problem
test:unitandtest:allended in:phpunit's exit status was discarded unconditionally. The message happens to be true today — this repo is a Python ExApp wrapper with no PHP test suite — but the mechanism never checked that. It swallowed whatever phpunit did, so a real failure (now, or the day a test suite is added here) would look exactly like "no tests to run", and
composer check:strictcould not fail on tests.That matters because
check:strictis about to be the thing an agent checks before deciding its job is done. A step that cannot fail satisfies that instruction with a no-op.Change
composer.jsononly. Guard on the actual precondition instead of masking the exit code:A genuinely absent suite is now a stated fact in the output, and a present-but-failing suite fails the gate.
Positive control — measured on this tree
PHP 8.3.32 container,
vendor/freshly installed fromcomposer.lock, injecting a temporaryphpunit.xmlplus one deliberately failing test:composer check:strictcomposer.json+ failing testALL CHECKS PASSED; the failure printed and was swallowed by|| echocomposer.json+ failing testSOME CHECKS FAILED, namingtest:all(Tests: 1, Assertions: 1, Failures: 1)composer.json, test removedSKIPPED: no phpunit.xml in this repoThe temporary
phpunit.xmland control test are not part of this PR — the diff iscomposer.jsononly.State after this PR
composer check:strictexits 0 on this branch:lint,phpcs,phpmd,psalmandphpstanall pass, andtest:allskips loudly. No findings needed fixing here.Note on measurement
The first measurement round in this repo showed psalm failing with
Cannot resolve stubfile path .../OCP/Capabilities/ICapability.php. That turned out to be an artefact of the shared local dev checkout (itsvendor/nextcloud/ocp/OCPhad been renamed toOCP.bak), not a repo defect. All numbers above are from a cleancomposer install.