Implement rstest helper call-site collection (8.2.2)#235
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Implemented passive call-site collection for
WalkthroughAdd passive call-site collection to ChangesCall-site collection for
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 18 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideAdds a detailed execution plan document for roadmap item 8.2.2, describing how the File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
changed the title
Mark the 8.2.2 execplan PR step complete after pushing the branch and updating draft PR #235.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Code Duplicationcrates/rstest_helper_should_be_fixture/src/collector_tests.rs: The module contains 2 functions with similar structure: collector_deduplicates_identical_callee_and_source_span,collector_keeps_distinct_source_spans_for_same_callee Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More Excess Number of Function Argumentscrates/rstest_helper_should_be_fixture/src/driver.rs: LateLintPass.check_fn What lead to degradation?LateLintPass.check_fn has 7 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Excess Number of Function Argumentscrates/rstest_helper_should_be_fixture/src/driver.rs: LateLintPass.check_fn What lead to degradation?LateLintPass.check_fn has 7 arguments, max arguments = 4 Why does this problem occur?Functions with many arguments indicate either a) low cohesion where the function has too many responsibilities, or b) a missing abstraction that encapsulates those arguments. How to fix it?Start by investigating the responsibilities of the function. Make sure it doesn't do too many things, in which case it should be split into smaller and more cohesive functions. Consider the refactoring INTRODUCE PARAMETER OBJECT to encapsulate arguments that refer to the same logical concept. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Mark the 8.2.2 execplan PR step complete after pushing the branch and updating draft PR #235.
leynos
force-pushed
the
8-2-2-call-site-collection-in-rstest-tests
branch
from
e4830a2 to
a6ad47c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6ad47c184
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. crates/rstest_helper_should_be_fixture/src/collector.rs Comment on lines +85 to +97 pub(crate) const fn new(
callee_key: String,
file_name: FileName,
lo: BytePos,
hi: BytePos,
) -> Self {
Self {
callee_key,
file_name,
lo,
hi,
}
}❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. crates/rstest_helper_should_be_fixture/src/driver.rs Comment on lines +193 to +203 fn check_fn(
&mut self,
cx: &LateContext<'tcx>,
_kind: hir::intravisit::FnKind<'tcx>,
_decl: &'tcx hir::FnDecl<'tcx>,
body: &'tcx hir::Body<'tcx>,
_span: Span,
def_id: LocalDefId,
) {
self.collect_call_sites(cx, body, def_id);
}❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Add a passive call-site collector for local helper invocations inside strict `#[rstest]` tests. Lower positional arguments into the shared argument fingerprint model, deduplicate recovered source spans, and log collected evidence at crate-post time without emitting diagnostics. Keep the implementation scoped to the lint crate and record the first milestone validation and CodeRabbit review outcome in the ExecPlan.
Add behavioural, property, and trybuild pass coverage for the rstest helper call-site collector. Record the CodeRabbit fixes and validation results in the 8.2.2 execplan.
Add an explicit regression proving macro-only HIR spans do not recover to user-editable source. Record the validation and CodeRabbit review outcome in the 8.2.2 execplan.
Update user, developer, design, roadmap, and execplan docs for the diagnostic-silent rstest helper call-site collection phase. Record the validation and review outcomes for 8.2.2.
Mark the 8.2.2 execplan PR step complete after pushing the branch and updating draft PR #235.
Extract shared duplicate call setup in the collector tests and behaviour scenarios while preserving the existing assertions. Move the `check_fn` call-site collection body into a private helper so the lint pass method delegates cleanly.
Flatten wrapped node labels in the consumer adoption flow so `nixie` can parse the Mermaid flowchart while preserving the documented decision path.
Record the completed 8.2.2 collector behaviour, the follow-on test and driver refactor, and the validation gates that now cover the current implementation state.
Mark the 8.2.2 ExecPlan complete and record the final refactor, validation, roadmap update, and retrospective notes so the plan matches the implemented branch state.
Include method receivers in helper-call fingerprints and read provider attributes from rustc parameter nodes so case inputs are not mistaken for fixture locals. Add debug logging for collection drop paths, remove redundant callee-key allocation during deduplication, expand collector test-module documentation, and move the collection fixture into a Cargo-backed Dylint example harness.
Use CallSiteLocation as the collector deduplication key instead of a separate DedupKey wrapper. This keeps the duplicate check deterministic while avoiding an extra callee-key allocation on each record insertion.
Clear `RUSTC_WRAPPER` in the Windows CI test step and in the shared UI harness when running on Windows. `dylint_testing::Test::example` expects bare `rustc` invocations in Cargo's JSON build output, so a wrapper such as `sccache` prevents it from discovering the example compiler command. Fold the wrapper cleanup into the existing vcpkg environment guard so both Windows-only mutations share one `env_test_guard()` lock and avoid re-entering the process-wide environment mutex.
Keep the Windows UI-test environment adjustments active while the runner executes, but release `env_test_guard()` before invoking the callback. This prevents nested runner setup from deadlocking on the non-re-entrant environment mutex while preserving the `RUSTC_WRAPPER` workaround needed by `dylint_testing::Test::example`.
Remove the Windows CI-wide `RUSTC_WRAPPER` override so sccache remains active for normal test work. Keep bare `rustc` handling scoped to the UI harness, where `dylint_testing::Test::example` needs it. Use `camino::Utf8Path` for the Windows vcpkg probe and convert it with `as_std_path()` at the environment boundary.
Ignore `.memdb/` so local daemon state and pid files do not appear as untracked repository changes after tool runs.
Move the `RUSTC_WRAPPER` inspection in `windows_env_guard` under `env_test_guard` so the early no-op path observes serialized environment state. Keep the mutex scoped to setup inspection and mutation so it is not held across the UI runner callback.
Drive `rstest_helper_should_be_fixture` through the Dylint example harness and assert the pass-owned collector records real call-site evidence. Treat rstest companion functions discovered from macro-expanded HIR as eligible collection roots so the boundary test exercises the actual driver path. Restore trybuild compile-time smoke coverage for the rstest fixtures and add a Windows-only harness test for scoped `RUSTC_WRAPPER` clearing. Document the internal collection-summary hook as test-only so it is not mistaken for a supported user-facing interface.
Redact call-site fingerprint values from normal driver debug output while keeping shape-only summaries for test harness assertions. Share the environment variable guard across UI tests, make collection summary paths unique per call, and restore Windows wrapper state for both previously set and absent `RUSTC_WRAPPER` values. Keep companion HIR exclusion explicit while preserving macro-aware rstest collection roots for generated test bodies.
Remove the fixed retry budget from `ExampleHarnessLock::acquire` so peer nextest processes wait for the cross-process directory lock instead of failing during long Windows CI example runs. Keep the existing directory-lock design and continue releasing the lock from `Drop` with `std::fs::remove_dir`.
Add a compile-checked `EnvVarGuard::set` example so the public test helper shows both usage and observable effect. Leave `RUSTC_WRAPPER` untouched when the Windows UI environment guard only performed VCPKG setup and did not clear a wrapper value itself.
Add focused collector coverage for literal argument lowering and unrecoverable-span handling without constructing a full rustc lint context. Recover abandoned example-harness lock directories after a conservative stale threshold so crashed test processes cannot block future runs indefinitely while legitimate Windows CI waits still succeed. Add shared env-var removal support and a Windows-only regression test for the VCPKG setup path with no `RUSTC_WRAPPER`.
Make helper-call collection walk closure bodies instead of returning early, while keeping closure call spans recoverable from the surrounding closure span. Return I/O errors from the example harness lock path, recover stale lock races, and bound stuck-lock waits with explicit timeout errors. Move Windows env guard assertions into the runner callback and document the shared UI harness env and lock conventions for future tests.
lodyai
Bot
force-pushed
the
8-2-2-call-site-collection-in-rstest-tests
branch
from
b81b889 to
a37bc88
Compare
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Update the stale `CallSiteCollector::record` documentation example to use `CallSiteLocation`, collapse duplicate stale-lock policy tests into a single parameterized case, and serialize Windows environment guard tests across their full bodies.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rstest_helper_should_be_fixture/src/driver.rs (1)
1-462: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSplit
driver.rs— it looks like it exceeds the 400-line-per-file limit.Last annotated line number in the diff context is 462, which is over the workspace's own limit ("Files must not exceed 400 lines in length — Large modules must be decomposed").
CallSiteVisitorplus the rstest-parameter/attribute conversion helpers (Lines 316-462) are a self-contained unit and a natural candidate to extract into a sibling module (e.g.visitor.rs), leavingdriver.rsfocused on lint-pass wiring and reporting.As per path instructions for
**/*.rs: "Files must not exceed 400 lines in length - Large modules must be decomposed."🤖 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 `@crates/rstest_helper_should_be_fixture/src/driver.rs` around lines 1 - 462, `driver.rs` is over the 400-line module limit, so split out the self-contained call-site collection and rstest helper logic into a sibling Rust module. Move `CallSiteVisitor` and the related helpers (`rstest_parameters`, `parameter_binding`, `parameter_attributes`, `attribute_from_hir`, `attribute_path`, `attribute_kind`, `attribute_style`, and `redacted_*` if they stay coupled) into a new module such as `visitor.rs`, then update `RstestHelperShouldBeFixture` to import and use them so `driver.rs` only handles lint wiring, config, and reporting.Source: Path instructions
🤖 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 `@crates/rstest_helper_should_be_fixture/src/collector.rs`:
- Around line 100-129: The doctest on CallSiteCollector::record is stale because
it still passes four arguments even though record now accepts a CallSiteRecord
and a CallSiteLocation. Update the example to construct a CallSiteLocation and
pass it as the second argument to CallSiteCollector::record, keeping the ignored
doctest aligned with the current API and using the existing CallSiteCollector,
CallSiteRecord, and CallSiteLocation symbols.
In `@crates/rstest_helper_should_be_fixture/src/driver.rs`:
- Around line 174-213: The dead companion-test guard in collect_call_sites is
never exercised because rstest_companion_tests is never populated, so remove
that early return or populate the set in check_crate and keep the existing
rstest_collection_roots gating as the source of truth. Also split driver.rs into
smaller modules to satisfy the 400-line limit, keeping collect_call_sites,
CallSiteVisitor, and related helper logic grouped by responsibility so the
module stays under the cap.
---
Outside diff comments:
In `@crates/rstest_helper_should_be_fixture/src/driver.rs`:
- Around line 1-462: `driver.rs` is over the 400-line module limit, so split out
the self-contained call-site collection and rstest helper logic into a sibling
Rust module. Move `CallSiteVisitor` and the related helpers
(`rstest_parameters`, `parameter_binding`, `parameter_attributes`,
`attribute_from_hir`, `attribute_path`, `attribute_kind`, `attribute_style`, and
`redacted_*` if they stay coupled) into a new module such as `visitor.rs`, then
update `RstestHelperShouldBeFixture` to import and use them so `driver.rs` only
handles lint wiring, config, and reporting.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4b7b9e7f-8ce0-4b23-abb6-2b07f5ce31cf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.gitignorecommon/src/test_support/mod.rscrates/rstest_helper_should_be_fixture/Cargo.tomlcrates/rstest_helper_should_be_fixture/examples/bootstrap_zero_diagnostic.rscrates/rstest_helper_should_be_fixture/examples/collection_zero_diagnostic.rscrates/rstest_helper_should_be_fixture/src/collector.rscrates/rstest_helper_should_be_fixture/src/collector_behaviour.rscrates/rstest_helper_should_be_fixture/src/collector_tests.rscrates/rstest_helper_should_be_fixture/src/driver.rscrates/rstest_helper_should_be_fixture/src/lib.rscrates/rstest_helper_should_be_fixture/tests/features/collection.featurecrates/rstest_helper_should_be_fixture/tests/ui.rscrates/rstest_helper_should_be_fixture/tests/ui/collection_zero_diagnostic.rsdocs/developers-guide.mddocs/execplans/8-2-2-call-site-collection-in-rstest-tests.mddocs/lints-for-rstest-fixtures-and-test-hygiene.mddocs/roadmap.mddocs/users-guide.mddocs/whitaker-dylint-suite-design.mdsrc/hir/tests.rssrc/testing/ui/mod.rssrc/testing/ui/tests.rs
| fn collect_call_sites<'tcx>( | ||
| &mut self, | ||
| cx: &LateContext<'tcx>, | ||
| body: &'tcx hir::Body<'tcx>, | ||
| def_id: LocalDefId, | ||
| ) { | ||
| let hir_id = cx.tcx.local_def_id_to_hir_id(def_id); | ||
| if self.rstest_companion_tests.contains(&hir_id) { | ||
| debug!( | ||
| target: LINT_NAME, | ||
| "skipping helper call-site collection for non-rstest function: def_id={:?}", | ||
| def_id, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| let attrs = cx | ||
| .tcx | ||
| .hir_attrs(hir_id) | ||
| .iter() | ||
| .filter_map(attribute_from_hir) | ||
| .collect::<Vec<_>>(); | ||
| if !is_rstest_test_with(&attrs, None, &self.detection_options) | ||
| && !self.rstest_collection_roots.contains(&hir_id) | ||
| { | ||
| debug!( | ||
| target: LINT_NAME, | ||
| "skipping helper call-site collection for non-rstest function: def_id={:?}", | ||
| def_id, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| let parameters = rstest_parameters(cx, body); | ||
| let fixture_locals = fixture_local_names(¶meters, &self.detection_options); | ||
| let mut visitor = | ||
| CallSiteVisitor::new(cx, &mut self.collector, def_id.to_def_id(), &fixture_locals); | ||
| visitor.visit_expr(body.value); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all references to rstest_companion_tests =="
rg -n --type=rust 'rstest_companion_tests' crates/rstest_helper_should_be_fixture
echo
echo "== does a harness-companion collector for SYNTHESIZED functions exist? =="
rg -n --type=rust 'fn collect_harness_test_functions|fn collect_rstest_companion_test_functions' src
echo
echo "== driver.rs total line count (400-line policy check) =="
wc -l crates/rstest_helper_should_be_fixture/src/driver.rsRepository: leynos/whitaker
Length of output: 1040
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== driver.rs around the set and the call-site collection =="
sed -n '90,230p' crates/rstest_helper_should_be_fixture/src/driver.rs
echo
echo "== src/hir/mod.rs around both collectors =="
sed -n '140,240p' src/hir/mod.rs
echo
echo "== all write sites for the companion-test set across the repo =="
rg -n --type=rust 'rstest_companion_tests\s*[.=]' .Repository: leynos/whitaker
Length of output: 8938
Remove the dead companion-test guard and split this module
- Populate
rstest_companion_testsincheck_crateor delete the field and early
return; nothing writes to the set, so the branch never runs and the existing
rstest_collection_rootscheck already excludes the same inputs. - Split
crates/rstest_helper_should_be_fixture/src/driver.rs; it is 522 lines,
which breaks the 400-line module limit.
🤖 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 `@crates/rstest_helper_should_be_fixture/src/driver.rs` around lines 174 - 213,
The dead companion-test guard in collect_call_sites is never exercised because
rstest_companion_tests is never populated, so remove that early return or
populate the set in check_crate and keep the existing rstest_collection_roots
gating as the source of truth. Also split driver.rs into smaller modules to
satisfy the 400-line limit, keeping collect_call_sites, CallSiteVisitor, and
related helper logic grouped by responsibility so the module stays under the
cap.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/rstest_helper_should_be_fixture/src/collector.rs`:
- Around line 108-114: The doctest example in collector.rs is using a
non-existent CallSiteRecord field, since record.callee_key is not available on
the record type. Update the example around CallSiteLocation::new and
collector.record to use a plain callee key string/value instead of pulling
callee_key from record, so the snippet matches the actual CallSiteRecord fields
(callee_def_id, fingerprint, test_source_def_id, span) and stays consistent with
CallSiteLocation.
In `@src/testing/ui/tests.rs`:
- Around line 124-132: The Windows test serialization lock in
windows_env_guard_test_lock currently panics on poison, which causes later tests
to fail after the first panic. Update the lock acquisition in this helper to
recover from a poisoned Mutex by using poisoned.into_inner() instead of
unwrap_or_else panic, so the test suite can continue and surface the original
failure in the serial UI tests.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da69928b-8e3d-4a0b-aafa-f5c0baa4189b
📒 Files selected for processing (3)
crates/rstest_helper_should_be_fixture/src/collector.rscrates/rstest_helper_should_be_fixture/tests/ui.rssrc/testing/ui/tests.rs
| /// let location = CallSiteLocation::new( | ||
| /// record.callee_key.clone(), | ||
| /// rustc_span::FileName::Custom("src/lib.rs".to_string()), | ||
| /// rustc_span::BytePos(0), | ||
| /// rustc_span::BytePos(4), | ||
| /// ); | ||
| /// let inserted = collector.record(record, location); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the doctest — record.callee_key does not exist.
The updated example reads record.callee_key.clone(), but CallSiteRecord carries only callee_def_id, fingerprint, test_source_def_id, and span. callee_key lives on CallSiteLocation, not on the record. Because the doctest is ignored, this drift compiles cleanly and slips past cargo test, just like the argument-count issue before it. Use a plain callee key string instead.
📝 Proposed fix
/// let location = CallSiteLocation::new(
- /// record.callee_key.clone(),
+ /// "crate::helper".to_string(),
/// rustc_span::FileName::Custom("src/lib.rs".to_string()),
/// rustc_span::BytePos(0),
/// rustc_span::BytePos(4),
/// );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// let location = CallSiteLocation::new( | |
| /// record.callee_key.clone(), | |
| /// rustc_span::FileName::Custom("src/lib.rs".to_string()), | |
| /// rustc_span::BytePos(0), | |
| /// rustc_span::BytePos(4), | |
| /// ); | |
| /// let inserted = collector.record(record, location); | |
| /// let location = CallSiteLocation::new( | |
| /// "crate::helper".to_string(), | |
| /// rustc_span::FileName::Custom("src/lib.rs".to_string()), | |
| /// rustc_span::BytePos(0), | |
| /// rustc_span::BytePos(4), | |
| /// ); | |
| /// let inserted = collector.record(record, location); |
🤖 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 `@crates/rstest_helper_should_be_fixture/src/collector.rs` around lines 108 -
114, The doctest example in collector.rs is using a non-existent CallSiteRecord
field, since record.callee_key is not available on the record type. Update the
example around CallSiteLocation::new and collector.record to use a plain callee
key string/value instead of pulling callee_key from record, so the snippet
matches the actual CallSiteRecord fields (callee_def_id, fingerprint,
test_source_def_id, span) and stays consistent with CallSiteLocation.
| #[cfg(windows)] | ||
| fn windows_env_guard_test_lock() -> MutexGuard<'static, ()> { | ||
| // These tests inspect process-global environment state after callbacks, | ||
| // so their whole bodies must run serially rather than only each mutation. | ||
| static LOCK: OnceLock<Mutex<()>> = OnceLock::new(); | ||
| LOCK.get_or_init(|| Mutex::new(())) | ||
| .lock() | ||
| .unwrap_or_else(|error| panic!("expected Windows env guard test lock: {error}")) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Inspecting the Windows test lock helper and nearby test harness code...\n'
git ls-files 'src/testing/ui/tests.rs' 'src/testing/ui/*' 'docs/*' | sed -n '1,120p'
printf '\n--- src/testing/ui/tests.rs (around the helper) ---\n'
sed -n '100,170p' src/testing/ui/tests.rs
printf '\n--- Search for similar serialisation / poison handling patterns ---\n'
rg -n "into_inner\\(|poison|Mutex<\\(\\)>|OnceLock<Mutex" src/testing -SRepository: leynos/whitaker
Length of output: 5361
Recover the poisoned lock in src/testing/ui/tests.rs. A panic in one serial test poisons this mutex and makes later acquisitions fail in a cascade that hides the first breakage; return poisoned.into_inner() instead of panicking.
🤖 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 `@src/testing/ui/tests.rs` around lines 124 - 132, The Windows test
serialization lock in windows_env_guard_test_lock currently panics on poison,
which causes later tests to fail after the first panic. Update the lock
acquisition in this helper to recover from a poisoned Mutex by using
poisoned.into_inner() instead of unwrap_or_else panic, so the test suite can
continue and surface the original failure in the serial UI tests.
2 participants
Summary
This branch implements roadmap task (8.2.2) for
rstest_helper_should_be_fixture: passive helper call-site collection inside strict#[rstest]tests, fixture-local and constant-aware argument fingerprinting, deterministic per-callee storage, and diagnostic-silent crate-post observability.Review feedback now addressed:
#[case]/#[values]provider markers are read from the HIR parameter node.Roadmap task: (8.2.2)
Execplan: docs/execplans/8-2-2-call-site-collection-in-rstest-tests.md
Review walkthrough
ArgAtomlowering.#[rstest]filtering, visitor traversal, span recovery, receiver-aware fingerprints, and debug-only crate-post summary.Validation
cargo check -p rstest_helper_should_be_fixture --all-targets --all-features: passed.cargo nextest run -p rstest_helper_should_be_fixture --all-targets --all-features: passed, 28 tests.RUSTFLAGS="-C prefer-dynamic -Z force-unstable-if-unmarked -D warnings" cargo nextest run -p rstest_helper_should_be_fixture --all-features example_compiles_without_diagnostics: passed, 2 tests.make check-fmt: passed.make lint: passed.make typecheck: passed.make test: passed, 1464 tests, 3 skipped.make markdownlint: passed before this review-fix commit; no Markdown files changed in this commit.make nixie: passed before this review-fix commit; no Mermaid files changed in this commit.coderabbit review --agent --dir crates/rstest_helper_should_be_fixture: completed with 0 findings after earlier fixes.coderabbit review --agent --dir src: completed with 0 findings.Notes
The lint remains diagnostic-silent in this branch. Threshold aggregation and user-facing diagnostics remain assigned to roadmap items 8.2.3 and 8.2.4.
Full-scope CodeRabbit reviews repeatedly stalled after sandbox preparation or tool completion in CLI v0.5.3. Each stalled process was checked against this worktree before termination. Scoped reviews for the changed implementation areas completed cleanly; a docs-scoped review also stalled before producing stored findings.
References