fix(quality): remove PHP test scripts that manufacture a green in a repo with no PHP tests - #19
Merged
Conversation
…epo with no PHP tests This repo is a Python ExApp. The application is ex_app/lib/main.py, shipped via Dockerfile + entrypoint.sh + requirements.txt. The only PHP present is phpcs-custom-sniffs/ (one sniff) plus analysis-bootstrap.php. Verified in this repo: no phpunit.xml, no phpunit.xml.dist, no tests/, no pytest.ini, no pyproject.toml, no conftest.py, no test_*.py, no *_test.py. The composer.json nevertheless declared test:unit / test:all as if [ -f phpunit.xml ] || [ -f phpunit.xml.dist ]; then ./vendor/bin/phpunit ...; else echo "SKIPPED: no phpunit.xml in this repo ..."; fi The condition is false by construction, so both scripts were a guaranteed exit 0 that ran nothing - and check, check:full and check:strict all chained them, so each aggregate carried a test leg that could never fail. It also shipped test:coverage and coverage:check (a 75% clover threshold) for a suite that does not exist. A script that pretends to run a PHP test suite in a repo with no PHP application code is worse than no script: it manufactures a green. Removing it makes the absence of tests VISIBLE. check:strict now prints, on success, exactly what it did and did not cover. Removed: test:unit, test:all, test:coverage, coverage:check. Rewritten: check, check:full, check:strict - no longer reference them. Verified nothing else calls them: - no reference in .github/workflows/ - the shared ConductionNL/.github quality.yml never invokes composer test:* (its frontend-tests job reads package.json, not composer.json), and enable-phpunit defaults to false and is not set by this repo, so no PHPUnit job existed in CI either. Measured after the change (openklant, PHP 8.5 container): composer test:all -> exit 1, "Command \"test:all\" is not defined" composer check:strict -> runs lint phpcs phpmd psalm phpstan only NOTE: this repo still has NO automated tests of any kind. The Makefile `test` target is `docker run --rm -it ...`, an interactive container launch that asserts nothing and cannot run in CI. Wiring up a real pytest suite is separate, larger work and is NOT done here.
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
This repo is a Python ExApp. The application is
ex_app/lib/main.py, shipped viaDockerfile+entrypoint.sh+requirements.txt. The only PHP in the repo isphpcs-custom-sniffs/(a single sniff) plusanalysis-bootstrap.php.Verified in this repo — none of these exist:
phpunit.xml,phpunit.xml.disttests/pytest.ini,pyproject.toml,conftest.pytest_*.pyor*_test.pycomposer.jsonnevertheless declared:The condition is false by construction — there is no code path in this repo that could ever make it true. Both scripts were a guaranteed
exit 0that ran nothing, andcheck,check:fullandcheck:strictall chained them, so every aggregate carried a test leg that could never fail.test:coverageandcoverage:check(a 75% clover threshold) were also declared for a suite that does not exist.A script that pretends to run a PHP test suite in a repo with no PHP application code is worse than no script: it manufactures a green. Removing it makes the absence of tests visible rather than papered over.
Change
Removed:
test:unit,test:all,test:coverage,coverage:check.Rewritten:
check,check:full,check:strict— they no longer reference the removed scripts, and on success they now say what they did and did not cover:check:strictnow runslint phpcs phpmd psalm phpstanonly.Verified nothing else calls the removed scripts
test:all,test:unit,test:coverageorcoverage:checkanywhere in.github/workflows/.ConductionNL/.githubquality.ymlnever invokescomposer test:*— itsfrontend-testsjob readspackage.json, notcomposer.json.enable-phpunitdefaults tofalsein that shared workflow and is not set by this repo, so no PHPUnit job existed in CI either. Nothing regresses.Measured
After the change (openklant, composer 2 container — the four ExApp repos are byte-identical in this region and got the identical edit):
composer validatepasses on all fourcomposer.jsonfiles.Note:
composer psalmcurrently fails in these repos withCannot resolve stubfile path vendor/nextcloud/ocp/OCP/Capabilities/ICapability.php. That is pre-existing and environmental — I reproduced it on the untouchedorigin/developmentcheckout with the originalcomposer.json, so it is not caused by this PR. CI installsnextcloud/ocpand does not hit it.Measured test count
Zero. That is the point of this PR. There is no test count to surface because there is no suite.
What this PR does NOT do
This repo still has no automated tests of any kind. The
Makefiletesttarget is:— an interactive container launch that asserts nothing and cannot run in CI.
I did not invent a Python test suite. Wiring up pytest (a
pyproject.toml/pytest.ini, atests/package, fixtures for the ExApp's FastAPI surface, and a CI job to run them) is separate, larger work and is deliberately out of scope here. This PR only stops the repo from reporting a test result it never had.