Skip to content

[ci] Guard every test file against landing without a CI lane - #2084

Open
fv-superagent wants to merge 3 commits into
mainfrom
agent/codebase-arrangement-9e80ef58506e
Open

fv-superagent wants to merge 3 commits into
mainfrom
agent/codebase-arrangement-9e80ef58506e

Conversation

@fv-superagent

@fv-superagent fv-superagent commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

This is by an automated agent, not a human.

Problem: fastvideo/tests/contract/test_ci_test_collection.py checked coverage at directory granularity, so a new file beside a directory that a lane lists only in part stayed invisible to CI. fastvideo/tests/AGENTS.md already warns about this under "CI Wiring", and the second-pass audit traced the gap to tests/hooks, tests/audio, most of tests/layers and tests/distributed, and 16 MLX files: #2074 (comment).

Change: resolve what each CI pytest invocation collects (shell and YAML line continuations, quoted or exported variable assignments such as golden_root=./fastvideo/tests/..., both --ignore forms, -m "not gpu", and the files ssim/ci_runner.py globs at the ssim root, which is not recursive), then require every test_*.py under fastvideo/tests/ to be collected or listed in a shrink-only DARK_TESTS allowlist. The allowlist holds the 51 files that are dark today, each with a reason, including one gpu-only file the unit lane deselects with -m "not gpu"; test_dark_test_allowlist_stays_accurate fails when an entry is fixed or goes missing. The old directory-level ALLOWLIST and its two tests are replaced. Test-only.

Verification:

  • python3 -m py_compile fastvideo/tests/contract/test_ci_test_collection.py: ok.
  • pytest is not installed in this environment, so I ran the four tests directly with yaml stubbed: test_every_test_file_is_collected_or_allowlisted, test_dark_test_allowlist_stays_accurate, test_parser_handles_the_command_shapes_ci_uses (continuations, both --ignore forms, quoted and exported variables, word-bounded expansion, gpu-only files, a phantom $pytest_rc line and non-recursive SSIM discovery) and test_local_tests_stays_out_of_ci; all pass. The parser resolves 33 invocations, 508 test files and 51 allowlisted; the dark set equals the allowlist exactly, with no entry stale.
  • Adding fastvideo/tests/hooks/test_zz_tmp_dark.py fails the guard and names that path; marking the collected fastvideo/tests/contract/test_env_policy.py dark fails the accuracy test.
  • Buildkite on the prior head 1316cd77 ran the unit lane (which collects fastvideo/tests/contract/) green; the vae lane failed once on a pyxis enroot image-import error and passed on retry. The current head reruns all lanes.
  • Could not run pre-commit run --files fastvideo/tests/contract/test_ci_test_collection.py: the hook cache path is read-only, and .pre-commit-config.yaml excludes fastvideo/tests/ anyway.

Risk: test-only. No runtime code, public API or CI config changes. The parser does not resolve a lane that selects tests through a shell glob or a generated list; such files look dark and need a name or an allowlist entry, which the docstring and failure message state. Marker handling is limited to -m "not gpu" with function- or module-level gpu markers. The unit lane's existing coverage is unchanged.

The test-collection contract checked directories, so a new file beside a
partially listed directory could merge and never run. Resolve each CI
pytest invocation and require every test file to be collected or listed in
a shrink-only allowlist of the 50 files that are dark today.
@mergify

mergify Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Merge Protections

🔴 1 of 1 protections blocking · waiting on 👀 reviews and 🤖 CI

Protection Waiting on
🔴 PR merge requirements 👀 reviews and 🤖 CI

🔴 PR merge requirements

Waiting for

  • #approved-reviews-by>=1
  • check-success=full-suite-passed
This rule is failing.
  • #approved-reviews-by>=1
  • check-success=full-suite-passed
  • check-success=fastcheck-passed
  • check-success~=pre-commit
  • title~=(?i)^\[(feat|feature|bugfix|fix|refactor|perf|ci|doc|docs|misc|chore|kernel|new.?model|skill|skills|infra)\]

@mergify mergify Bot added type: ci CI/CD infrastructure scope: infra CI, tests, Docker, build labels Nov 10, 2026
Fix five parser gaps found in review: -m "not gpu" now makes a gpu-only
file dark (tests/attention/test_vsa_h3_heads_first_tile.py joins the
allowlist, now 51 entries); a bare --ignore consumes its path; only a real
pytest command word counts, so $pytest_rc no longer creates a phantom
invocation; variable substitution is word-bounded; and quoted or exported
assignments resolve.
@greptile-apps

greptile-apps Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; all previous findings are addressed and no new blocking issue was found.

Summary

This PR replaces directory-level CI coverage checks with file-level checks and an explicit DARK_TESTS list.

  • The CI guard checks coverage for every test file.

Reviews (2) · Last reviewed commit: "[ci]: Match SSIM discovery and cover the..." · Reviewed by Greptile

Address Greptile review: the SSIM lane globs only test_*.py at the ssim
root, so track those files instead of the directory; drop the redundant
section labels; and cover continuations, both --ignore forms, variable
expansion, gpu-only files and the phantom-invocation case with focused
tests.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: infra CI, tests, Docker, build type: ci CI/CD infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants