Skip to content

gate-25: the PHPUnit coverage match is unscoped and skip-blind — measured false coverage on 3 endpoints with zero tests #367

Description

@rubenvdlinde

Summary

gate-25's PHPUnit coverage test is a bare substring search over the concatenated text of every tests/**/*Test.php:

hydra-gates/scripts/lib/check_contract_coverage.py:341-343

if phpunit:
    if re.search(rf"->\s*{re.escape(method)}\s*\(", phpunit):
        return True

Three consequences, all measured in pipelinq while closing 96 → 3 findings with ~330 real contract tests.

1. It never checks which class the call landed on — false coverage already present before we started

These endpoints had zero tests and were nonetheless absent from the finding list:

endpoint what actually matched
posCustomer#search ->search( in XWikiControllerTest, ContactSyncControllerTest, KvkApiClientTest
posPayment#initiate ->initiate( in MollieAdapterTest / StripeAdapterTest / AdyenAdapterTest / CcvAdapterTest — on payment adapters, not the controller
posPayment#refund ->refund( in the same adapter tests

So the gate's finding list understates the debt, and it does so silently. All three now have real tests, but they were not on any list to be assigned.

2. A call inside a markTestSkipped test counts as coverage

The check cannot see PHPUnit semantics. One agent noted that a ->login( call living only in a skipped test's setup would have satisfied the gate over a call that never executes. In this exercise 21 tests are legitimately skipped (each pinning a reproduced product defect), and every one of them counts toward gate-25.

An agent trying to close this gate cheaply could therefore write N unconditionally-skipped test bodies and turn it green. That is the .github#345 shape (an @e2e exclude scored as positive coverage) in a different gate.

3. It has no notion of assertions

A bare $controller->foo(); with no assertion closes an endpoint. Combined with (1), so does a bare call on an unrelated mock.

Suggested fix — cheap and closes 1 and 2

  • Scope the match: prefer a match inside a test file whose name corresponds to the controller (FooControllerFooControllerTest.php), or at minimum require the controller class name to appear in the same file as the call.
  • Refuse unconditionally-skipping tests: a test method whose body reaches markTestSkipped before the call, with no branch, should not credit coverage.

Both are file-local text checks of the same order of cost as what is already there.

⚠️ Expect a false-RED shape — tightening the match will surface endpoints across the fleet that are currently credited by an unrelated call. Same sequencing hazard as #347/#356/#358/#365. Filing, not fixing.

What is right about the gate, for the record

The scope handling is exemplary and is the model the other diff-scoped gates should copy: run_gate() reads os.environ.get("HYDRA_GATE_BASE_REF") with no default (:417), falls back to a genuine whole-tree sweep, and distinguishes EMPTY SCOPE (exit 3) from PASS with an explicit message saying "this is not a pass". That is precisely what #361/#364 had to retrofit into gate-16 and what #347 still needs for gate-61. run_report() at :499 does still carry the "origin/development" default, which is worth aligning.

Measured on canonical ConductionNL/.github@765d081 (checker byte-identical at 81c8c97), pipelinq development @ 206db4be, full scope.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions