ci: enforce no ignored tests across the org - #253
Conversation
A composite action fails CI on statically ignored tests: Rust #[ignore], JS/TS .skip/.only and x-prefixed suites, Solidity xtest-renamed functions and unconditional vm.skip(true). Conditional skips (an if within the preceding lines or a non-literal argument) stay allowed, which covers the existing env-gated fork-test skips across the org. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a new composite GitHub Action, ChangesNo-ignored-tests action and CI wiring
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CI as CI Workflow
participant Action as no-ignored-tests Action
participant Repo as Repository Files
CI->>Action: Invoke no-ignored-tests step
Action->>Repo: Search for Rust #[ignore], JS .skip/.only, Solidity xtest
Action->>Repo: Search for vm.skip(true) occurrences
Action->>Action: Run awk guard check on vm.skip matches
alt Violations found
Action-->>CI: Print matches, exit 1
else No violations
Action-->>CI: Print "No ignored tests found."
end
Related Issues: None referenced in provided data. Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/actions/no-ignored-tests/action.yml (1)
36-49: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueGuard heuristic can be defeated by unrelated
if (nearby.The conditional-skip check only looks for the literal text
if (on the same line or within the preceding 5 lines — it doesn't verify that theifactually wraps thevm.skip(true)call. An unrelatedif (from a prior, already-closed block could cause a genuinely unconditionalvm.skip(true)to pass unflagged.Given this is documented as a known, validated tradeoff (not full AST parsing), this is a soft heuristic gap rather than a blocking bug — worth keeping in mind, since it slightly weakens the org-wide ban it purports to enforce.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/actions/no-ignored-tests/action.yml around lines 36 - 49, The conditional-skip heuristic in the awk check is too loose because it treats any nearby `if (` as proof that `vm.skip(true)` is conditional. Update the logic in the no-ignored-tests action so the `vm.skip(true)` match is only exempted when the same line or the surrounding context actually indicates the call is inside an `if` block, rather than relying on unrelated prior `if (` text; keep the check anchored around the awk rule that scans for `vm.skip(true)`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/actions/no-ignored-tests/action.yml:
- Around line 36-49: The conditional-skip heuristic in the awk check is too
loose because it treats any nearby `if (` as proof that `vm.skip(true)` is
conditional. Update the logic in the no-ignored-tests action so the
`vm.skip(true)` match is only exempted when the same line or the surrounding
context actually indicates the call is inside an `if` block, rather than relying
on unrelated prior `if (` text; keep the check anchored around the awk rule that
scans for `vm.skip(true)`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba826b92-782b-446e-a2da-4c440807c5a3
📒 Files selected for processing (3)
.github/actions/no-ignored-tests/action.yml.github/workflows/rainix-rs-static.yaml.github/workflows/rainix-sol-static.yaml
Conditional skips are banned along with unconditional ones - skips hide undone work. Vendor exclusions (dependencies/, lib/) now match only at the repo root so first-party test/lib/ trees are checked; node_modules, target and .git stay excluded by name at any depth. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
|
Tip For best results, initiate chat on the files or code changes. SIZE=M |
What
Adds a
no-ignored-testscomposite action and runs it from bothrainix-sol-static.yamlandrainix-rs-static.yaml(referenced@main, so consumers inherit it on their next run).What it bans — no exceptions
#[ignore](zero current usage org-wide).it/test/describe.skip/.onlyandxit/xdescribe—.onlyincluded since a focused test silently ignores everything else (zero current usage).function xtest…disabled-by-rename (zero current usage), and anyvm.skip(whatsoever — conditional or not. A test either runs and passes, or it is deleted; skips hide undone work.Parameterized
_test*external helpers (Foundry does not auto-run them; rainlang uses the convention in 9 files) are untouched. Vendor exclusions (dependencies/,lib/) match only at the repo root so first-partytest/lib/trees are checked;node_modules/target/.gitstay excluded by name.Known reds this deliberately creates
Landing this makes the static job red in three repos until their skips are reworked into hard behavior:
test/lib/deploy/: twovm.skip(true)(registry-unreachable guards) and threevm.skip(vm.envOr("CI", false))(tests that never run in CI).LibDecimalFloatDeployTaggedConstants.t.sol.vm.skip(vm.envOr("CI", false))inLibMetaBoardDeploy.t.sol.The rework in each case: remove the skip so registry/RPC unavailability is a hard failure, and CI-skipped fork tests actually run in CI (the fork RPC secrets exist).
Verification
Run against real checkouts: raindex FAILS with all 5 sites listed per file:line, rainlang PASSES (helpers untouched), synthetic repo with all four violation classes FAILS. rain.erc4626.words/rain.flare-style repos with no skips PASS.
🤖 Generated with Claude Code