fix(ci): recognize namespaced test attributes in pr-triage's needs-tests check#202
fix(ci): recognize namespaced test attributes in pr-triage's needs-tests check#202euxaristia wants to merge 5 commits into
Conversation
…sts check The needs-tests detection regex only matched a literal #[test] or #[cfg(test) immediately after #[, so it missed namespaced test attributes like #[tokio::test] or #[async_std::test]. Any PR whose only new tests were async (the common pattern in this codebase, e.g. crates/gl/src/agent.rs) got falsely labeled needs-tests even when tests were added in the same commit. Reproduced against PR Gitlawb#198: it added two #[tokio::test]-only tests in the same commit as the source change, and the triage bot still commented needs-tests. Widens the regex to also match any attribute ending in ::test], verified against representative patches including PR Gitlawb#198's actual diff. Closes Gitlawb#201
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR triage workflow replaces patch-text matching with source-aware Rust test detection that filters comments and literals, maps added lines, fetches head files, and recognizes ChangesRust test detection
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PRDiff
participant TriageWorkflow
participant GitHubContentsAPI
PRDiff->>TriageWorkflow: provide Rust patch
TriageWorkflow->>TriageWorkflow: identify added line numbers
TriageWorkflow->>GitHubContentsAPI: fetch head-version Rust file
GitHubContentsAPI-->>TriageWorkflow: return file content
TriageWorkflow->>TriageWorkflow: strip comments and literals
TriageWorkflow->>TriageWorkflow: detect added test attributes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
beardthelion
left a comment
There was a problem hiding this comment.
Verified by running both the old and new regex over the full addsInlineTest predicate (filename guard, f.patch guard, ^\+ anchor, .some(), and the alternation), 20+ inputs each way. The direction is right and the dominant #[tokio::test] form now resolves with zero regressions. Two things to fold in before merge.
Findings
-
[P2] Cover the parametrized
#[tokio::test(flavor = ...)]form
.github/workflows/pr-triage.yml:78
The::test]alternative needs]immediately aftertest, but the args form is::test(...), sotest]never appears and it's missed. This is the PR's own target class and it's live in the tree atcrates/gitlawb-node/src/git/smart_http.rs:753and:818. A PR whose only new tests use the flavor form still gets falsely labeledneeds-tests. The bare form (248 hits) is fixed, so this is a partial fix rather than a wrong one. -
[P3] Anchor the match to an attribute so
::test]stops over-matching non-test code
.github/workflows/pr-triage.yml:78
::test]is unanchored, so it also fires on ordinary added lines likelet r = registry[Region::test];and on the macro appearing in a comment or string literal. Effect is bounded (it only suppresses the advisory label, which isn't a merge gate), but it's a new false-positive class the old regex didn't have.
Both close with a single regex. I ran this against the same matrix: 0 regressions, 0 false-positives, and it catches the flavor form:
/^\+\s*#\[\s*(cfg\(\s*test|[\w-]+(::[\w-]+)*::test|test)\b/m
Anchoring to a leading #[ removes the over-match, and ::test\b (instead of ::test]) also clears #[tokio::test(...)].
Net: strict improvement as it stands, and the above makes it complete against the title. Non-gating label, so nothing here is severe, but the flavor form defeats the stated purpose on code that already exists in the repo.
…cover parametrized tokio::test
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P3] Keep the detector on an added diff line
.github/workflows/pr-triage.yml:80
\s*after the diff+also consumes newlines. As a result, a Rust PR can add only a blank line immediately before an existing context-line#[tokio::test]; the expression then treats that unchanged test as newly added and suppressesneeds-tests. Use horizontal whitespace (for example,[ \t]*) at this position so the attribute itself must be on an added line. -
[P3] Complete the
cfg(test)predicate before accepting it as a test
.github/workflows/pr-triage.yml:80
The widenedcfg\(\s*test\bbranch also matches valid non-test predicates such as#[cfg(test = "triage_only")]. Adding only that attribute to an otherwise testless Rust PR makesaddsInlineTesttrue and bypasses the advisory test signal; the former expression did not do so. Require the predicate to close (for example,cfg\(\s*test\s*\)) before classifying it as a test gate. -
[P3] Handle valid whitespace in the namespaced attribute syntax
.github/workflows/pr-triage.yml:80
Rust accepts token whitespace in attributes, including#[tokio :: test]and#[cfg (test)], but the new matcher requires both::andcfg(to be adjacent. A PR whose only added test uses either valid form is still labeledneeds-tests, so the claimed namespaced/cfg recognition remains incomplete. Allow horizontal token whitespace around these separators while keeping the match confined to the added line. -
[P3] Do not treat raw-string contents as executable test attributes
.github/workflows/pr-triage.yml:80
Anchoring at#[prevents ordinary comment and string-expression lines from matching, but it does not distinguish a multiline raw string: an added fixture containing a line#[tokio::test]makes this expression true despite adding no runnable test. This is a new false positive for namespaced attributes and lets a testless Rust change suppressneeds-tests; account for Rust string state (or otherwise avoid presenting the leading-attribute check as excluding strings).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In @.github/workflows/pr-triage.yml:
- Around line 83-87: Update the patch-scanning logic around the lexical state
object and f.patch iteration so state is not carried across incomplete hunks.
Reconstruct lexical context from the complete head-version file using added-line
coordinates, or reset and initialize state from each hunk’s surrounding content
before evaluating additions, ensuring #[test] text is classified correctly when
a patch begins inside a comment or multiline literal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d71643b0-2e39-45be-b0ae-f0e11deb6339
📒 Files selected for processing (1)
.github/workflows/pr-triage.yml
A diff hunk carries no lexical context from lines outside it, so scanning patch text alone could misclassify an added line that sits inside a comment or raw string opened earlier in the file (or outside the hunk's context window). Fetch the head-version file content via the contents API and lex it in file order, checking only the lines the diff actually added.
|
Pushed a fixup addressing jatmn's four findings and this morning's CodeRabbit finding:
For the lexer's cross-hunk state gap CodeRabbit flagged: instead of scanning the diff patch text directly, the workflow now fetches the head-version file content via the contents API and lexes it in file order, then checks only the line numbers the diff actually added (computed from the hunk headers). This means lexical state (open raw string, block comment, etc.) is always accurate regardless of where a hunk's context window starts. Verified against 12 cases covering all of the above, including the specific scenario CodeRabbit described (an added line landing inside a raw string opened outside the hunk's visible context). |
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Preserve nesting while skipping block comments
.github/workflows/pr-triage.yml:89
Rust block comments nest, but this lexer represents comment state as a boolean and clears it at the first*/. A source-only PR can add#[tokio::test]after an inner comment closes but before the enclosing comment closes (/* outer,/* inner */,#[tokio::test],*/); Rust still treats the attribute as comment text, while this code treats it as live and suppressesneeds-tests. Track block-comment depth (and cover nested comments) before accepting an attribute. -
[P2] Bound the per-file Contents API scan and do not silently classify failures as no test
.github/workflows/pr-triage.yml:170
Every changed Rust file with a patch now causes a serialrepos.getContentcall, while every failure is silently skipped. A large PR can therefore exhaust the workflow's ten-minute/API budget, and rate-limit or transient failures on the only file containing a new test make the triage job incorrectly addneeds-tests(or prevent label reconciliation altogether). The previous implementation only examined the already-fetched file patches. Put a bounded strategy/fallback around this scan and surface a failure rather than treating an unavailable file as testless. -
[P3] Require
testto be the terminal attribute segment
.github/workflows/pr-triage.yml:80
Thetest\balternatives stop at a word boundary rather than a valid attribute terminator, so non-test attributes such as#[foo::test::helper],#[test::helper], and#[foo::test = "x"]match. Adding one of those to a Rust-only PR suppresses the advisory test signal despite adding no test. Require optional horizontal space followed by]or(after the bare/namespacedtestsegment. -
[P3] Recognize valid multiline test attributes
.github/workflows/pr-triage.yml:80
Rust permits whitespace, including newlines, between attribute tokens. A valid added test formatted as#\n[test]or#[tokio\n::test]is scanned one line at a time and neither line matches this horizontal-whitespace-only expression, so the PR still receivesneeds-tests. Carry enough attribute-token state across added lines (or parse the added attribute span) to cover the valid forms the workflow is intended to recognize.
…ing, and bound Contents API scans
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Do not abort triage when the bounded scan is exhausted
.github/workflows/pr-triage.yml:199
The cap is reached using the broadtest/cfgsubstring prefilter rather than a real attribute match. A normal Rust PR that edits 16 files with (for example)#[cfg(feature = ...)]or even comments containing those words reaches thethrowon the sixteenth file. That exits before the label reconciliation below, so every managed label (including stale labels that should be removed) and the guidance comment are left untouched. Keep the API work bounded, but use a non-fatal conservative fallback so an otherwise valid multi-file PR is still triaged. -
[P2] Handle Contents API responses that do not carry Base64 text
.github/workflows/pr-triage.yml:206
getContentis decoded unconditionally, but GitHub returns an emptycontentwithencoding: "none"for the 1–100 MB range unless the request uses the raw representation. An added#[tokio::test]in such a Rust file therefore gets lexed as an empty file and is incorrectly labeledneeds-tests(and an absentcontentresponse would instead fail the job). Check the response encoding/size and fetch a supported raw/blob representation or take an explicit conservative fallback. -
[P3] Require an added attribute token, not merely added whitespace inside one
.github/workflows/pr-triage.yml:226
The expression permits newlines between#and[, then marks a match as new when any character in its span is on an added line. Thus a source-only PR can insert just a blank line into an existing multiline attribute (#, added blank line,[test]) and makeaddsInlineTesttrue without adding a test. Rust accepts that formatting, so this suppressesneeds-testsfor the rest of an otherwise testless change. Tie the match to an added attribute token/span rather than any whitespace it crosses. -
[P3] Preserve token boundaries while stripping literals
.github/workflows/pr-triage.yml:136
Removing a literal without a placeholder can join unrelated Rust tokens into a synthetic test attribute. For example,consume!(# "not an attribute" [test]);is valid input to amacro_rules! consume { ($($tt:tt)*) => {}; }matcher, but the stripper produces#[test]; the regex then treats the added invocation as a test. Preserve a separator for skipped comments/literals (or match lexed attribute tokens) so testless macro changes cannot bypass the advisory label. -
[P3] Recognize absolute namespaced test attributes
.github/workflows/pr-triage.yml:80
Rust accepts an attribute path beginning with::, but the namespaced branch requires an identifier before its first::. Consequently#[::tokio::test]is not detected even though it is a valid async-test attribute, and a PR that adds only that form still receivesneeds-tests. Allow the optional leading path separator when matching a namespacedtestattribute.
Summary
The
needs-testscheck in.github/workflows/pr-triage.ymluses:/^\+.*#\[(test\]|cfg\(test\))/mto detect whether a PR added an inline test. This only matches a literal
#[test]or#[cfg(test)immediately after#[, so it misses namespaced test attributes like#[tokio::test]or#[async_std::test]. Any PR whose only new tests are async gets falsely labeledneeds-tests, even when tests were added in the same commit.Closes #201
Reproduction
PR #198 added two
#[tokio::test]-only tests tocrates/gl/src/doctor.rsin the same commit as the source fix, and the triage bot still commentedneeds-tests.Fix
/^\+.*(#\[test\]|#\[cfg\(test\)|::test\])/mMatches any attribute ending in
::test], covering namespaced test macros at any depth, while still matching the original#[test]/#[cfg(test)]forms.How a reviewer can verify
I don't see any existing test harness for the inline
github-scriptworkflows in this repo, so I verified the regex directly against representative diff patches (a standalone Node script, not committed — just the verification, shown here):All six cases behave as expected under the new regex, including the real PR #198 diff that triggered this.
Kind of change
Summary by CodeRabbit
#[tokio::test]/#[async_std::test]andcfg(test)(with optional arguments).