amtronics/platform-actions

Watch

Fork 0

test: Keep the fixtures that proved each checker fix #34

Merged

ahmad merged 1 commit from feature/31-test-the-checker into main

2026-09-04 07:33:11 +00:00

ahmad commented

2026-09-04 06:36:20 +00:00

Owner

Issue

Closes #31.

Problem

check-actions.py gates every pull request here and had no test. It now has a dozen validation branches, each added because a shape passed as ok (0 run step(s) checked) — and each verified by hand against fixtures that were then thrown away. The evidence was about the commit, not the script.

Solution

Fifteen cases in .forgejo/scripts/test-check-actions.py, run by PR / validate before the real check, so a broken checker reports as a failing test rather than as a confusing result from checking the actions.

Fixtures live in a temporary tree, never in the repository. ROOT.rglob("action.yml") finds everything beneath the root, so a deliberately-broken fixture committed anywhere here would fail the real run — the guard broken by its own test. That is the second criterion and it is a trap I hit while testing #28 by hand.

Review notes — the mutation pass

The fourth criterion asks for each case to be proved by deleting the branch it covers. Twelve branches, deleted one at a time:

branch removed	result
missing-steps	caught
empty-list	caught
steps-not-a-list	caught
step-not-a-mapping	caught
runs-not-a-mapping	caught
using-must-be-composite	caught
required-fields	caught
`bash -n` check	caught
no-actions-found guard	caught
YAML parse guard	caught
not-a-mapping guard	caught
expression substitution	survived

One survived, and I am reporting it rather than hiding it. My case was called "an expression where a word is expected" and asserted a body containing ${{ }} passes. Removing EXPRESSION.sub entirely does not change that — no raw expression I could construct makes bash -n fail:

echo ${{ inputs.x }}                     parses fine raw
multi-line ${{ … }}                      parses fine raw
${{ … }} as a case pattern               parses fine raw

So the substitution is defensive against a false positive, not load-bearing for catching a real error. I have renamed the case to say what it actually asserts and written the reason above it, because a test named for a property it does not test is worse than no test — it is the thing that makes a suite look complete.

A limitation found while probing that branch

The expression regex is \$\{\{[^}]*\}\}, which stops at the first }. An expression containing one is not substituted at all:

echo ${{ fromJSON('{"a":1}').a }}   ->  unchanged; reaches bash -n raw

Latent — no action here uses such an expression — and a separate concern from this issue, so it is filed rather than fixed in a test-only change.

Risks and trade-offs

The suite tests the checker against its known failures. A shape nobody has thought of still passes silently; that is the nature of the thing and the reason the mutation table matters more than the case count.
validate now runs Python twice. Under a second, against a job that already installs a YAML parser and pulls a container.
The fixtures duplicate small pieces of action syntax. If the real actions change shape, these do not follow automatically — deliberate, since a fixture that tracks the code it tests stops being independent of it.

### Issue Closes #31. ### Problem `check-actions.py` gates every pull request here and had no test. It now has a dozen validation branches, each added because a shape passed as `ok (0 run step(s) checked)` — and each verified by hand against fixtures that were then thrown away. The evidence was about the commit, not the script. ### Solution Fifteen cases in `.forgejo/scripts/test-check-actions.py`, run by `PR / validate` **before** the real check, so a broken checker reports as a failing test rather than as a confusing result from checking the actions. **Fixtures live in a temporary tree, never in the repository.** `ROOT.rglob("action.yml")` finds everything beneath the root, so a deliberately-broken fixture committed anywhere here would fail the real run — the guard broken by its own test. That is the second criterion and it is a trap I hit while testing #28 by hand. ### Review notes — the mutation pass The fourth criterion asks for each case to be proved by deleting the branch it covers. Twelve branches, deleted one at a time: | branch removed | result | |---|---| | missing-steps | **caught** | | empty-list | **caught** | | steps-not-a-list | **caught** | | step-not-a-mapping | **caught** | | runs-not-a-mapping | **caught** | | using-must-be-composite | **caught** | | required-fields | **caught** | | `bash -n` check | **caught** | | no-actions-found guard | **caught** | | YAML parse guard | **caught** | | not-a-mapping guard | **caught** | | **expression substitution** | **survived** | **One survived, and I am reporting it rather than hiding it.** My case was called "an expression where a word is expected" and asserted a body containing `${{ }}` passes. Removing `EXPRESSION.sub` entirely does not change that — no raw expression I could construct makes `bash -n` fail: ``` echo ${{ inputs.x }} parses fine raw multi-line ${{ … }} parses fine raw ${{ … }} as a case pattern parses fine raw ``` So the substitution is **defensive against a false positive**, not load-bearing for catching a real error. I have renamed the case to say what it actually asserts and written the reason above it, because a test named for a property it does not test is worse than no test — it is the thing that makes a suite look complete. ### A limitation found while probing that branch The expression regex is `\$\{\{[^}]*\}\}`, which stops at the first `}`. An expression containing one is not substituted at all: ``` echo ${{ fromJSON('{"a":1}').a }} -> unchanged; reaches bash -n raw ``` Latent — no action here uses such an expression — and a separate concern from this issue, so it is filed rather than fixed in a test-only change. ### Risks and trade-offs - The suite tests the checker against its **known** failures. A shape nobody has thought of still passes silently; that is the nature of the thing and the reason the mutation table matters more than the case count. - `validate` now runs Python twice. Under a second, against a job that already installs a YAML parser and pulls a container. - The fixtures duplicate small pieces of action syntax. If the real actions change shape, these do not follow automatically — deliberate, since a fixture that tracks the code it tests stops being independent of it.

ahmad added 1 commit

2026-09-04 06:36:20 +00:00

test: Keep the fixtures that proved each checker fix

PR / validate (pull_request) Successful in 8m41s

Details

88192399a9

Every branch in check-actions.py exists because some shape passed as
ok (0 run step(s) checked). Each fix was verified by hand against
fixtures which were then thrown away, so the evidence was about the
commit rather than the script as it stands.

Fifteen cases, each building a throwaway tree with one fixture action.
They live in a temp directory rather than the repository because
ROOT.rglob finds everything beneath the root, so a deliberately-broken
fixture committed here would fail the real run — the guard broken by
its own test.

Proved by deleting each branch of the checker and watching the suite
notice: eleven of twelve mutations caught. The twelfth is recorded in
the pull request rather than papered over.

ahmad referenced this pull request

2026-09-04 06:36:48 +00:00

chore: The expression regex misses an expression containing braces #35

ahmad commented

2026-09-04 07:02:51 +00:00

Author

Owner

Verification against #31's acceptance criteria

PR / validate green on 8819239, and that green now includes the new Test the checker step running before the real check — so the suite is exercised by CI rather than only by me.

Given the checker is run against an action with each known-bad shape, when the test runs, then each is reported and the exit code is non-zero.
Eleven bad shapes, each asserted on both exit code and a fragment of the message: missing steps, steps with no value, empty list, not a list, a step that is not a mapping, runs not a mapping, using not composite, a missing required field, a shell syntax error, unparseable YAML, and a file that is not a mapping. Plus one that is not about a file at all — no action.yml anywhere must be reported, not passed, since an empty run is otherwise indistinguishable from a clean one.

Given a valid composite action and a valid non-composite one, when the test runs, then neither is reported.
Three passing cases: a valid composite, one whose steps are all uses: (which declares steps and checks zero run bodies — the case the reviewer of #30 found and I had not thought of), and a body carrying an expression.

Given a fixture exists in the repository, when PR / validate runs the checker over the real actions, then the fixture is not picked up.
No fixture is committed. Each case builds a temp tree containing a copy of the checker and one action.yml. This is the criterion I would most easily have failed: ROOT.rglob("action.yml") finds everything beneath the root, so a fixtures/ directory would fail the real run — the guard broken by its own test.

Given a branch of the checker is deleted, when the test runs, then it fails — verified by deleting one, not assumed.
Twelve branches deleted one at a time. Eleven caught. One survived, and it is in the pull request body rather than omitted: removing EXPRESSION.sub changes nothing, because no raw ${{ }} I could construct makes bash -n fail. That branch is defensive against a false positive, not load-bearing. The case asserting it has been renamed for what it actually tests, with the reason written above it.

Given the test runs in CI, when it fails, then its output identifies which shape regressed.
Each case prints ok <name> or FAIL <name>: exit N, expected M with the checker's output attached, and the step tees into /tmp/validate.log, so the existing failure reporter posts it on the pull request.

What this does not do

It tests the checker against the shapes someone has already been caught by. A shape nobody has thought of still passes silently — which is why the mutation table matters more than the case count, and why the survivor is reported rather than rounded away.

Filed #35 for the limitation found while probing that survivor: the expression regex stops at the first }, so ${{ fromJSON('{"a":1}').a }} is never substituted.

### Verification against #31's acceptance criteria `PR / validate` green on `8819239`, and that green now includes the new `Test the checker` step running before the real check — so the suite is exercised by CI rather than only by me. **Given the checker is run against an action with each known-bad shape, when the test runs, then each is reported and the exit code is non-zero.** Eleven bad shapes, each asserted on both exit code and a fragment of the message: missing `steps`, `steps` with no value, empty list, not a list, a step that is not a mapping, `runs` not a mapping, `using` not composite, a missing required field, a shell syntax error, unparseable YAML, and a file that is not a mapping. Plus one that is not about a file at all — no `action.yml` anywhere must be reported, not passed, since an empty run is otherwise indistinguishable from a clean one. **Given a valid composite action and a valid non-composite one, when the test runs, then neither is reported.** Three passing cases: a valid composite, one whose steps are all `uses:` (which declares steps and checks zero run bodies — the case the reviewer of #30 found and I had not thought of), and a body carrying an expression. **Given a fixture exists in the repository, when `PR / validate` runs the checker over the real actions, then the fixture is not picked up.** No fixture is committed. Each case builds a temp tree containing a copy of the checker and one `action.yml`. This is the criterion I would most easily have failed: `ROOT.rglob("action.yml")` finds everything beneath the root, so a `fixtures/` directory would fail the real run — the guard broken by its own test. **Given a branch of the checker is deleted, when the test runs, then it fails — verified by deleting one, not assumed.** Twelve branches deleted one at a time. **Eleven caught. One survived**, and it is in the pull request body rather than omitted: removing `EXPRESSION.sub` changes nothing, because no raw `${{ }}` I could construct makes `bash -n` fail. That branch is defensive against a false positive, not load-bearing. The case asserting it has been renamed for what it actually tests, with the reason written above it. **Given the test runs in CI, when it fails, then its output identifies which shape regressed.** Each case prints `ok <name>` or `FAIL <name>: exit N, expected M` with the checker's output attached, and the step tees into `/tmp/validate.log`, so the existing failure reporter posts it on the pull request. ### What this does not do It tests the checker against the shapes **someone has already been caught by**. A shape nobody has thought of still passes silently — which is why the mutation table matters more than the case count, and why the survivor is reported rather than rounded away. Filed `#35` for the limitation found while probing that survivor: the expression regex stops at the first `}`, so `${{ fromJSON('{"a":1}').a }}` is never substituted.

ahmad changed title from ~~WIP: test: Keep the fixtures that proved each checker fix~~ to test: Keep the fixtures that proved each checker fix

2026-09-04 07:02:53 +00:00

ahmad_bot approved these changes

2026-09-04 07:32:11 +00:00

ahmad_bot left a comment

Approving, and this is the PR I have least to add to. I ran my own mutation pass rather than reading yours, and it reproduces:

empty-list branch          -> CAUGHT   (1 failing case)
steps-not-a-list branch    -> CAUGHT   (2 failing cases)
using-must-be-composite    -> CAUGHT   (1 failing case)
bash -n check              -> CAUGHT   (1 failing case)
no-actions-found guard     -> CAUGHT   (1 failing case)
EXPRESSION substitution    -> SURVIVED (0 failing cases)

15/15 pass as committed. The precise failing-case counts are the useful part: each mutation is caught by the case written for it and not by collateral, which is what makes the table evidence rather than a coincidence. (My crude deletion of the missing-steps branch broke the file structurally and failed 14 cases, so that row of mine proves less than the others — the clean substitutions above are the ones I would stand behind.)

The survivor is real, and reporting it is the best thing in this PR. I probed it independently: no raw ${{ }} form I could construct makes bash -n fail — plain, as a word inside a test, as a case pattern, or with a brace inside the expression. So EXPRESSION.sub is defensive against a false positive rather than load-bearing against a real error, exactly as you say. Renaming the case to what it actually asserts, instead of leaving a name that implied a property it does not test, is the difference between a suite and a decoration — and it is the second time in this repository that checking a guard revealed the test for it was not testing it.

The fixtures-in-a-temp-tree decision earns its comment. rglob("action.yml") means a committed broken fixture would fail the real run — the guard broken by its own test. That is a trap worth the two paragraphs it gets, and the suite cleans up after itself (mkdtemp paired with rmtree in both helpers).

Running the test before the real check is the right order for the reason you give: a broken checker should report as a failing test, not as a confusing result from checking the actions. And the step tees into the same log with pipefail, so it inherits the failure reporting rather than being the one step that goes dark.

Against #31: the suite exists and runs in validate; fixtures are outside the repository; each case asserts an exit code and an output fragment; and the mutation requirement is met for eleven of twelve branches with the twelfth reported rather than quietly counted. That last one is the criterion satisfied more honestly than a clean table would have satisfied it.

Filing the [^}]* regex limitation separately rather than fixing it in a test-only change is right — I confirmed it is latent (echo ${{ fromJSON('{"a":1}').a }} reaches bash -n unsubstituted and parses fine), so nothing is failing today.

Approval does not merge.

Approving, and this is the PR I have least to add to. I ran my own mutation pass rather than reading yours, and it reproduces: ``` empty-list branch -> CAUGHT (1 failing case) steps-not-a-list branch -> CAUGHT (2 failing cases) using-must-be-composite -> CAUGHT (1 failing case) bash -n check -> CAUGHT (1 failing case) no-actions-found guard -> CAUGHT (1 failing case) EXPRESSION substitution -> SURVIVED (0 failing cases) ``` 15/15 pass as committed. The precise failing-case counts are the useful part: each mutation is caught by the case written for it and not by collateral, which is what makes the table evidence rather than a coincidence. (My crude deletion of the missing-steps branch broke the file structurally and failed 14 cases, so that row of mine proves less than the others — the clean substitutions above are the ones I would stand behind.) **The survivor is real, and reporting it is the best thing in this PR.** I probed it independently: no raw `${{ }}` form I could construct makes `bash -n` fail — plain, as a word inside a test, as a `case` pattern, or with a brace inside the expression. So `EXPRESSION.sub` is defensive against a false positive rather than load-bearing against a real error, exactly as you say. Renaming the case to what it actually asserts, instead of leaving a name that implied a property it does not test, is the difference between a suite and a decoration — and it is the second time in this repository that checking a guard revealed the test for it was not testing it. **The fixtures-in-a-temp-tree decision earns its comment.** `rglob("action.yml")` means a committed broken fixture would fail the real run — the guard broken by its own test. That is a trap worth the two paragraphs it gets, and the suite cleans up after itself (`mkdtemp` paired with `rmtree` in both helpers). Running the test **before** the real check is the right order for the reason you give: a broken checker should report as a failing test, not as a confusing result from checking the actions. And the step tees into the same log with `pipefail`, so it inherits the failure reporting rather than being the one step that goes dark. **Against #31:** the suite exists and runs in `validate`; fixtures are outside the repository; each case asserts an exit code and an output fragment; and the mutation requirement is met for eleven of twelve branches with the twelfth reported rather than quietly counted. That last one is the criterion satisfied more honestly than a clean table would have satisfied it. Filing the `[^}]*` regex limitation separately rather than fixing it in a test-only change is right — I confirmed it is latent (`echo ${{ fromJSON('{"a":1}').a }}` reaches `bash -n` unsubstituted and parses fine), so nothing is failing today. Approval does not merge.

ahmad commented

2026-09-04 07:33:05 +00:00

Author

Owner

Merging.

Your failing-case counts are a better artefact than my table, and I want to say why rather than just accept the approval. I recorded caught-or-survived; you recorded how many cases failed per mutation. One case failing for the mutation written for it is evidence the suite is precise; fourteen failing means the mutation broke the file rather than the branch — which is exactly what you noticed about your own missing-steps row and said so. Counting the blast radius distinguishes a test that caught the defect from a test that caught the wreckage. I will use that shape next time.

"The second time in this repository that checking a guard revealed the test for it was not testing it." That is the sentence worth keeping. #30's empty-steps case and this one both came from the same move — delete the thing and see whether anything notices — and both times the answer was "not what I thought". It is now in the knowledge file as the mechanical check rather than as a habit I might remember.

On v3. This one touches .forgejo/scripts/ and .forgejo/workflows/ only, no action.yml, so the tag moves to a commit whose actions are byte-identical. Consumers get a new tag and the same behaviour — same as #30, unlike #32.

The #35 regex limitation stays filed rather than folded in, for the reason you agree with: nothing is failing today, and a test-only change is the wrong place to alter what the checker matches.

Merging. **Your failing-case counts are a better artefact than my table**, and I want to say why rather than just accept the approval. I recorded caught-or-survived; you recorded *how many* cases failed per mutation. One case failing for the mutation written for it is evidence the suite is precise; fourteen failing means the mutation broke the file rather than the branch — which is exactly what you noticed about your own missing-steps row and said so. Counting the blast radius distinguishes a test that caught the defect from a test that caught the wreckage. I will use that shape next time. **"The second time in this repository that checking a guard revealed the test for it was not testing it."** That is the sentence worth keeping. `#30`'s empty-`steps` case and this one both came from the same move — delete the thing and see whether anything notices — and both times the answer was "not what I thought". It is now in the knowledge file as the mechanical check rather than as a habit I might remember. **On `v3`.** This one touches `.forgejo/scripts/` and `.forgejo/workflows/` only, no `action.yml`, so the tag moves to a commit whose actions are byte-identical. Consumers get a new tag and the same behaviour — same as `#30`, unlike `#32`. The `#35` regex limitation stays filed rather than folded in, for the reason you agree with: nothing is failing today, and a test-only change is the wrong place to alter what the checker matches.

ahmad merged commit f5eea459a7 into main

2026-09-04 07:33:11 +00:00

ahmad referenced this pull request from a commit

2026-09-04 07:33:14 +00:00

test: Keep the fixtures that proved each checker fix (#34)

ahmad referenced this pull request

2026-09-04 08:03:04 +00:00

chore: The expression regex misses an expression containing braces #35

ahmad referenced this pull request

2026-09-04 08:06:46 +00:00

fix: Match an expression that contains braces #36

ahmad referenced this pull request