Skip to content

fix(quality): make the skipped-unit-suite green self-documenting - #210

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/test-all-self-documenting
Aug 4, 2026
Merged

fix(quality): make the skipped-unit-suite green self-documenting#210
rubenvdlinde merged 1 commit into
developmentfrom
fix/test-all-self-documenting

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Summary

nldesign's unit suite genuinely cannot run in a bare checkout, and this PR does not change that. The "requires Nextcloud" justification here is true — I verified it before touching anything. What this PR fixes is that the resulting green was indistinguishable from a real one.

The justification is true — verified

Clean worktree of origin/development, PHP 8.4.22, vendor/ mounted from the main checkout:

An error occurred inside PHPUnit.

Message:  Interface "OCP\Files\AppData\IAppDataFactory" not found
Location: /app/tests/Unit/Controller/SettingsControllerAuditTest.php:40

#0 /app/vendor/phpunit/phpunit/src/Runner/TestSuiteLoader.php(115): require_once()
...
EXIT=255

A 255 is an abort during test collection, not a test verdict. The suite type-hints OCP interfaces that only a Nextcloud server checkout provides. So the ../../lib/base.php guard is correct and stays. Making this hard-fail would trade a documented skip for a crash that still runs zero tests.

What was actually wrong

check, check:full and check:strict printed a bare ALL CHECKS PASSED whether or not a single test had run. The skip message named the missing file but not what the green did or did not mean, so the caveat was invisible at exactly the moment someone reads the summary line — and the guard looked like removable noise.

Change

Adopts the pattern opencatalogi already uses.

  1. Skip message now names the verbatim failure, the file and line, the 255 exit, why the guard exists, and why deleting it does not help:
SKIPPED: the unit suite DID NOT RUN - it needs a Nextcloud server tree (../../lib/base.php not found).
  Verbatim failure in a bare checkout (measured 2026-08-04, PHP 8.4.22):
    Interface "OCP\Files\AppData\IAppDataFactory" not found
    at tests/Unit/Controller/SettingsControllerAuditTest.php:40 -- phpunit exits 255
  A 255 is an ABORT, not a test verdict: the suite type-hints OCP interfaces that
  only a Nextcloud server checkout provides. Do NOT "fix" this by deleting the
  guard - that trades a documented skip for a crash that still runs zero tests.
  Measured with a server tree present: a failing suite exits 1 and a passing one
  exits 0, so this guard does not weaken a real run. Run from inside a Nextcloud
  checkout, or rely on the CI PHPUnit job, for a real test verdict.
  1. The aggregates no longer print an unqualified green when the suite was skipped:
ALL CHECKS PASSED - EXCEPT test:all, WHICH DID NOT RUN.
The unit suite needs a Nextcloud server tree (../../lib/base.php). In a bare
checkout it is skipped, so this green says NOTHING about the tests. Verified
by experiment: with the server tree absent, a phpunit that exits 1 still
produces this exact line. Run check:strict from inside a Nextcloud checkout,
or rely on the CI PHPUnit job, for a real test verdict.

Measured — both directions

Run in a composer 2 container with a stub phpunit whose exit code I control, and a ../../lib/base.php I create and delete. The claim written into the message is literally this experiment.

server tree phpunit exits test:all check:strict
absent 1 0 + SKIPPED block 0 + EXCEPT block
present 1 1 1SOME CHECKS FAILED
present 0 0

Row 2 is the positive control: when the suite can run, a failure still propagates all the way through check:strict. The guard does not weaken a real run — it only covers the case where no run is possible.

Row 1 is the condition being documented, and is unchanged behaviour by design.

Test count — NOT measured

I could not measure a live test count for this suite and am not going to claim one:

  • a bare tree aborts at 255 (above);
  • mounting the repo's own server tree gets further but fails with Exception: Not installed from OC::checkInstalled() — that checkout has no installed instance;
  • the shared dev Nextcloud container does not have nldesign mounted and is currently in maintenance mode, and I am not deploying to it to obtain a number.

Static count only, for scale, explicitly not a measured run: 527 public function test* methods across 58 *Test.php files under tests/Unit. The authoritative count comes from the CI PHPUnit job.

Scope

composer.json only — test:unit, test:all, check, check:full, check:strict. test:coverage and coverage:check untouched. No test files changed.

nldesign's test:unit / test:all genuinely cannot run in a bare checkout, and
that is NOT being changed here - the justification is true. Verified on a
clean worktree of origin/development (PHP 8.4.22, vendor/ from the main
checkout), phpunit aborts:

  Interface "OCP\Files\AppData\IAppDataFactory" not found
  Location: /app/tests/Unit/Controller/SettingsControllerAuditTest.php:40
  exit 255

A 255 is an abort during test collection, not a test verdict. The suite
type-hints OCP interfaces that only a Nextcloud server checkout provides, so
the ../../lib/base.php guard is correct and stays.

What was wrong is that the guard's green was indistinguishable from a real
one. check / check:full / check:strict printed a bare "ALL CHECKS PASSED"
whether or not a single test had run.

Changes, adopting the pattern opencatalogi already uses:
- the skip message now names the verbatim failure, the file and line, the
  255 exit, why the guard exists, and why deleting it does not help;
- check / check:full / check:strict now print "ALL CHECKS PASSED - EXCEPT
  test:all, WHICH DID NOT RUN" plus an explanation whenever the server tree
  is absent, so the caveat cannot be missed and the guard cannot be
  "fixed" away by someone who thinks it is noise.

Measured (composer 2 container, stub phpunit, both directions):
  no server tree, phpunit exits 1 -> test:all exit 0, SKIPPED block printed
  no server tree, static green     -> check:strict exit 0, EXCEPT-block printed
  server tree present, phpunit 1   -> test:all exit 1, check:strict exit 1
  server tree present, phpunit 0   -> test:all exit 0

so the guard demonstrably does not weaken a real run: when the suite can
run, a failure still propagates through check:strict.
@github-actions

github-actions Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/nldesign @ 3bf4629

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

Quality workflow — 2026-08-04 08:57 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit d96c89d into development Aug 4, 2026
30 of 32 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/test-all-self-documenting branch August 4, 2026 11:01
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