Skip to content

Add a CI check that changed test cases fail on the base branch - #13469

Closed
alies-dev wants to merge 4 commits into
vimeo:masterfrom
alies-dev:alies-dev/validate-tests-CI
Closed

alies-dev wants to merge 4 commits into
vimeo:masterfrom
alies-dev:alies-dev/validate-tests-CI

Conversation

@alies-dev

Copy link
Copy Markdown
Contributor

Adds a CI job that requires every test case a PR adds or changes to fail without the PR's changes: a case that passes on the merge base doesn't assert the change.

Each added or changed provider entry and test* method must pass on the PR head, then fail when the PR's tests/ run against the merge base's src/, stubs and dictionaries. Every run has a 120 s timeout (a timeout or crash on the base counts as a failure), and more than 60 selected cases fails the check.

A case that has to keep passing on the base (an edge case guarding the change) needs a marker directly above the provider entry, or above the test method and its docblock:

// guard: <reason>
'caseName' => [

The release:internal, release:typo and release:docs labels skip the job. Replaying #13455 (0cc606cc3..2abd992eb) flags 19 of its 48 new or changed cases.

Runs each provider entry and test method a PR adds or changes alone, on
the head and on the merge base with the head's tests. A case that passes
on the base asserts nothing and needs a `// guard: <reason>` marker.
Skipped by the release:internal, release:typo and release:docs labels.
- taint filter picks Valid or Invalid from the entry's provider, so a
  base failure of the same-named other case cannot hide a pass
- item ids carry class and provider, so same names in different
  providers or classes no longer collide
- numeric string keys, which PHP turns into int, get a `#N` filter
- all-skipped is SKIPPED whatever the exit code
- guard markers come from the PHP comments of the AST node, never
  from string or heredoc content
@alies-dev
alies-dev marked this pull request as ready for review November 8, 2026 16:21

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 45ebaac. Configure here.

=> 'passes on base, asserts nothing: make it fail on base, or add a "// guard: <reason>" marker above it',
$base !== 'PASS' && $guard !== null => 'stale guard marker: the case fails on base, remove the marker',
default => null,
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipped head tests pass CI

Medium Severity

When a selected case is skipped on the PR head, verdictOf returns no problem and base is never exercised, even though the check is meant to require a pass on head and a failure on the merge base (unless // guard).

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 45ebaac. Configure here.

@github-actions

github-actions Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

✅ Performance: unchanged · Issues: unchanged

Project Memory (MB) Wall (s) Issues Weighted
✅ laravel 1 821 60.4 $\color{gray}{\textsf{+0.3\%}}$ 25 912 51 144
✅ symfony 4 002 176.6 $\color{gray}{\textsf{+0.4\%}}$ 51 063 101 050
✅ doctrine-orm 428 10.8 3 505 5 574
✅ psl 651 13.6 $\color{gray}{\textsf{-2.4\%}}$ 1 424 5 514
✅ phpunit 506 10.5 $\color{gray}{\textsf{-2.5\%}}$ 1 627 1 265
✅ php-parser 245 10.0 $\color{gray}{\textsf{+3.8\%}}$ 17 169 20 624
✅ tempest 922 20.4 $\color{gray}{\textsf{+1.6\%}}$ 5 016 9 616
✅ amphp-amp 60 2.0 $\color{gray}{\textsf{-0.5\%}}$ 32 79
✅ webmozarts-assert 272 4.4 $\color{gray}{\textsf{-0.2\%}}$ 577 2 714
✅ guzzle-promises 55 2.0 $\color{gray}{\textsf{+0.5\%}}$ 163 407
✅ monolog 161 5.1 $\color{gray}{\textsf{-1.2\%}}$ 1 004 2 722
✅ drupal 5 196 192.6 $\color{gray}{\textsf{-0.5\%}}$ 98 825 161 668
Total 14 319 508.2 206 317 362 377
How to read this table
  • Memory: Psalm's own peak figure, threads=1. More than +5 % fails, unless the PR changes composer.json.
  • Wall: base and head run on the same runner; ±10 % is noise. Above +20 % warns, never fails. re-run: a reversed second run was taken and the faster kept.
  • Issues: head count, then $\color{red}{\textsf{+added}}$ $\color{green}{\textsf{-removed}}$ against base.
  • Weighted: issues weighted by error level (levels 1–8 as is, always-errors 10, feature-specific and purity suggestions 0). The projects' own code is pinned and their dependencies resolve identically for base and head, so a change means Psalm reports more, fewer or differently serious issues. A rise is not necessarily a regression.
  • Total: sum over the projects compared on both sides. Thresholds apply per project, not to the total.
  • Colors: $\color{red}{\textsf{red}}$ past a fail or warn threshold (or more issues), $\color{green}{\textsf{green}}$ an improvement past noise (or fewer issues), $\color{gray}{\textsf{gray}}$ noise.
  • Performance: regressed if a head run crashed or timed out or a memory or wall delta is red, else improved if one is green, else unchanged. Its icon is the check result: ❌ fail · ⚠️ warn · ✅ pass; the icon next to each project is that project's result, 💥 = not comparable: base crashed or timed out (counts as ⚠️).
  • Issues (heading): the total weighted change, increased · decreased · unchanged. Informational, never fails the check.

head 45ebaac · base 6130439 (merge-base) · workflow run

@alies-dev

Copy link
Copy Markdown
Contributor Author

i've changed my mind: we don't need it on CI (low ROI)

@alies-dev alies-dev closed this Nov 8, 2026
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