Skip to content

check_test_reachability counts a step disabled with if: false as running its test #652

Closed

Description

@vladimirrott

scripts/check_test_reachability.sh counts a workflow step as running a test even when the step is switched off. Its parser looks only at run (scripts/check_test_reachability.sh:41-44):

run = step.get("run")
if isinstance(run, str):
    match = command.fullmatch(run.strip())

It never reads the step's if, the job's if, or continue-on-error on either. At 90cc24e, on a copy of the tree, I added if: ${{ false }} to the step that runs systemd-directory-modes.test.sh in ci.yml:

pristine   rc=0  test-reachability: every release and E2E test is invoked by a gate.
if: false  rc=0  test-reachability: every release and E2E test is invoked by a gate.

tests/release/npm-audit-off-pr-path.test.sh has the same blind spot: it decides whether the scheduled npm audit step is enforced from step.get("run") and continue-on-error (:86, :97), and does not read if.

Why it matters

The usual way to quiet a flaky test in CI is if: false or continue-on-error: true. Either one leaves the test unrun or non-blocking while the gate whose job is to notice still prints "every release and E2E test is invoked by a gate".

Scope

  • Count a step only when neither the step nor its job has an if that is a literal false (false, ${{ false }}), and neither has continue-on-error: true. For any other if expression, either count it and print which expression it depends on, or refuse; say which you chose in the PR.
  • Apply the same rule in npm-audit-off-pr-path.test.sh.
  • Trap: YAML parses a bare false as a boolean and ${{ false }} as a string. Handle both.

Tests first

  • tests/release/test-reachability.test.sh already builds workflow fixtures. Add three: step if: ${{ false }}, step continue-on-error: true, job if: false. Each must exit non-zero and name the test file. All three pass today.
  • The same three fixtures for the npm-audit guard.
  • Remove the new check and the fixtures must go green again, which proves they bite.

Difficulty

easy: two small Python blocks and fixtures in tests that already have the harness.

Getting started

CONTRIBUTING.md has the build and test commands. No CLA and no copyright waiver. The project is MIT; sign off each commit with git commit -s (the required dco check enforces it since #732).

Activity

  1. added
    bugSomething isn't working
    help wantedExtra attention is needed
    easyDifficulty: self-contained, no deep context needed
    on Oct 24, 2026
  2. vladimirrott commented on Oct 24, 2026

    @vladimirrott
    MemberAuthor

    @be-student this is about your gate. #377 made every release test reach one, and check_test_reachability.sh still counts a step switched off with if: ${{ false }} as reaching it. The fixtures harness you'd extend is already in test-reachability.test.sh.

    I'm holding it for you. Say so here and I'll assign it. If it isn't for you, no reply needed.

  3. vladimirrott commented on Oct 27, 2026

    @vladimirrott
    MemberAuthor

    @be-student Thanks for looking at this one. I'm opening it back up since I haven't heard from you in a few days. If you still want it, say so here and it's yours again.

  4. vladimirrott commented on Oct 27, 2026

    @vladimirrott
    MemberAuthor

    @ITSMERNB This has the same shape as your #570: a CI check that counts something as having run when it never did. scripts/check_test_reachability.sh treats a step disabled with if: false as running its test, so a test can be switched off with the checker still green. The issue has the line and a test that pins it. Want it? Say so here and I'll assign it to you.

  5. added
    claimedSomeone has said in the thread that they are working on this
    on Oct 27, 2026
  6. vladimirrott commented on Nov 3, 2026

    @vladimirrott
    MemberAuthor

    @ITSMERNB I haven't heard back on this in a week, so I'm putting it back in the pool. If you want it later, say so here and it's yours.

  7. added
    twir-listedListed in a This Week in Rust CfP. Do not offer to anyone until that issue publishes.
    and removed
    claimedSomeone has said in the thread that they are working on this
    on Nov 3, 2026
  8. QinXi-ai commented on Nov 5, 2026

    @QinXi-ai
    Contributor

    I'll take this one. I'll cover the job and step conditions in both checks, with regression fixtures and mutation checks.

  9. removed
    twir-listedListed in a This Week in Rust CfP. Do not offer to anyone until that issue publishes.
    on Nov 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingeasyDifficulty: self-contained, no deep context neededhelp wantedExtra attention is needed

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions