Implement dynamic test fixtures for lint exclusions (#111)#214
Conversation
Reviewer's GuideImplements dynamic, runtime-generated integration test fixtures for lint exclusion behavior in the no_std_fs_operations crate, replacing on-disk fixture projects with temporary crates and updating tests and dependencies accordingly. Sequence diagram for dynamic lint exclusion integration testssequenceDiagram
actor Tester
participant Test as excluded_crate_suppresses_diagnostics
participant Fixture as create_fixture_project
participant Config as fixture_dylint_config
participant Source as fixture_source
participant Assert as assert_fixture_behaviour
participant CargoDylint as run_cargo_dylint
participant Diagnostics as diagnostic_count
Tester->>Test: invoke
Test->>Fixture: create_fixture_project(crate_name, is_excluded=true)
Fixture->>Fixture: create TempDir
Fixture->>Config: fixture_dylint_config(crate_name, is_excluded=true)
Config-->>Fixture: dylint_toml_contents
Fixture->>Source: fixture_source(crate_name)
Source-->>Fixture: lib_rs_contents
Fixture->>Fixture: write Cargo.toml, dylint.toml, src/lib.rs
Fixture-->>Test: FixtureProject
Test->>Assert: assert_fixture_behaviour(FixtureProject, expect_diagnostics=false)
Assert->>CargoDylint: run_cargo_dylint(FixtureProject)
CargoDylint-->>Assert: output
Assert->>Diagnostics: diagnostic_count(output)
Diagnostics-->>Assert: count
Assert->>Assert: assert count == 0
Assert-->>Test: success
Tester->>Test: invoke non_excluded_crate_emits_diagnostics
Test->>Fixture: create_fixture_project(crate_name, is_excluded=false)
Fixture->>Config: fixture_dylint_config(crate_name, is_excluded=false)
Config-->>Fixture: dylint_toml_contents
Fixture-->>Test: FixtureProject
Test->>Assert: assert_fixture_behaviour(FixtureProject, expect_diagnostics=true)
Assert->>CargoDylint: run_cargo_dylint(FixtureProject)
CargoDylint-->>Assert: output
Assert->>Diagnostics: diagnostic_count(output)
Diagnostics-->>Assert: count
Assert->>Assert: assert count > 0 and test fails as expected
Updated class diagram for dynamic fixture helpers in integration_exclusion testsclassDiagram
class FixtureProject {
+TempDir temp_dir
+PathBuf root_path
+PathBuf root()
+PathBuf src_dir()
+PathBuf cargo_toml_path()
+PathBuf dylint_toml_path()
+PathBuf lib_rs_path()
}
class TempDir {
+path() PathBuf
+close() Result
}
class IntegrationExclusionTests {
+excluded_crate_suppresses_diagnostics()
+non_excluded_crate_emits_diagnostics()
}
class FixtureFunctions {
+create_fixture_project(crate_name, is_excluded) FixtureProject
+fixture_dylint_config(crate_name, is_excluded) String
+fixture_source(crate_name) String
+assert_fixture_behaviour(fixture_project, expect_diagnostics)
}
class CargoDylintRunner {
+run_cargo_dylint(fixture_project) String
+diagnostic_count(output) usize
}
FixtureProject --> TempDir : owns
IntegrationExclusionTests --> FixtureFunctions : uses
FixtureFunctions --> FixtureProject : creates
FixtureFunctions --> CargoDylintRunner : uses
CargoDylintRunner --> FixtureProject : reads
File-Level Changes
Assessment against linked issues
Possibly linked issues
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughRefactor integration tests to generate TempDir-backed fixture projects at runtime, centralise assertions via a fallible harness using FixtureProject and OnceLock-cached lint builds, remove static fixtures, add workspace/dev-deps ( ChangesIntegration Testing Infrastructure Refactor
Sequence DiagramsequenceDiagram
autonumber
participant TestHarness as TestHarness
participant Fixture as FixtureProject
participant FS as Filesystem (TempDir)
participant Cargo as Cargo/Dylint subprocess
participant Parser as JSONParser
participant Assert as Assertions
TestHarness->>Fixture: create_fixture_project(name, excluded?)
Fixture->>FS: write `Cargo.toml`, `dylint.toml`, `src/lib.rs`
TestHarness->>Cargo: run `cargo dylint` in TempDir
Cargo-->>TestHarness: exit code + stdout (JSON)
TestHarness->>Parser: parse JSON diagnostics
Parser-->>TestHarness: diagnostic count for no_std_fs_operations
TestHarness->>Assert: assert expected count (0 or >0) and exit status
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (15 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@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. crates/no_std_fs_operations/tests/integration_exclusion.rs Comment on lines +293 to +305 fn excluded_crate_suppresses_diagnostics() {
let lint_library_path = lint_library_path();
let fixture = create_fixture_project("excluded_test_crate", true);
assert_fixture_behaviour(
fixture.root(),
&lint_library_path,
"excluded_test_crate",
Expectation {
should_emit_diagnostics: false,
should_succeed: true,
},
);
}❌ New issue: Code Duplication |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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.
…mic temp dirs for integration tests - Removed static fixture crates under tests/fixtures in favor of ephemeral projects created in temporary directories - Added FixtureProject utility to create per-test temporary crates with configurable exclusion - Updated integration_exclusion.rs tests to use dynamic fixture projects instead of static - Added tempfile dependency for temporary directory management - Improves test isolation and flexibility for exclusion behavior tests Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
|
@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. Code Duplicationcrates/no_std_fs_operations/tests/integration_exclusion.rs: What lead to degradation?The module contains 2 functions with similar structure: excluded_crate_suppresses_diagnostics,non_excluded_crate_emits_diagnostics 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 |
Add a shared helper for the exclusion integration tests so the excluded and non-excluded cases differ only in their fixture inputs and expected outcome. Prefer the rustup-managed Cargo path in the Makefile when it exists so hook environments with a minimal `PATH` can still run the standard gates.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 10825
Use the following message in CodeScene's diagnostic suppression UI:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lodyai
Bot
force-pushed
the
test-fixtures-for-exclusion-lint-t0egzl
branch
from
a8ae630 to
1367e2c
Compare
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/no_std_fs_operations/tests/integration_exclusion.rs`:
- Around line 297-323: Replace the two duplicated test functions
excluded_crate_suppresses_diagnostics and non_excluded_crate_emits_diagnostics
with a single parameterized rstest that calls run_exclusion_test for each case;
create a single test function (keep #[ignore = "..."] and #[serial]) annotated
with #[rstest(crate_name, is_excluded, expected => case("excluded_test_crate",
true, Expectation { should_emit_diagnostics: false, should_succeed: true }),
case("non_excluded_crate", false, Expectation { should_emit_diagnostics: true,
should_succeed: false })) and inside invoke run_exclusion_test(crate_name,
is_excluded, expected) so the duplicated bodies are collapsed into one
matrix-driven test while preserving the same run_exclusion_test and Expectation
usage.
🪄 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: 8ac30d8b-5250-4bef-9d4f-ecfc785c2ba4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
crates/no_std_fs_operations/Cargo.tomlcrates/no_std_fs_operations/tests/fixtures/excluded_project/Cargo.tomlcrates/no_std_fs_operations/tests/fixtures/excluded_project/dylint.tomlcrates/no_std_fs_operations/tests/fixtures/excluded_project/src/lib.rscrates/no_std_fs_operations/tests/fixtures/non_excluded_project/Cargo.tomlcrates/no_std_fs_operations/tests/fixtures/non_excluded_project/dylint.tomlcrates/no_std_fs_operations/tests/fixtures/non_excluded_project/src/lib.rscrates/no_std_fs_operations/tests/integration_exclusion.rs
💤 Files with no reviewable changes (6)
- crates/no_std_fs_operations/tests/fixtures/excluded_project/dylint.toml
- crates/no_std_fs_operations/tests/fixtures/excluded_project/Cargo.toml
- crates/no_std_fs_operations/tests/fixtures/non_excluded_project/Cargo.toml
- crates/no_std_fs_operations/tests/fixtures/non_excluded_project/dylint.toml
- crates/no_std_fs_operations/tests/fixtures/excluded_project/src/lib.rs
- crates/no_std_fs_operations/tests/fixtures/non_excluded_project/src/lib.rs
Refactor the integration exclusion tests in no_std_fs_operations from duplicate #[test] functions into a single #[rstest] matrix. Keep ignore/serial attributes and shared run_exclusion_test helper usage.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/no_std_fs_operations/tests/integration_exclusion.rs (1)
264-267:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStop panicking on malformed cargo JSON without fixture context.
Return a
Resultfromdiagnostic_countand surface parse failures with crate name and stderr inassert_fixture_behaviour. Keep failure reporting actionable whencargo dylintemits non-JSON stdout.Patch
-fn diagnostic_count(output: &[u8]) -> usize { - Message::parse_stream(Cursor::new(output)) - .map(|message| message.expect("cargo dylint should emit valid JSON messages")) - .filter_map(|message| match message { - Message::CompilerMessage(message) => Some(message.message), - _ => None, - }) - .filter(|diagnostic| { - diagnostic - .code - .as_ref() - .is_some_and(|code| code.code == LINT_CRATE_NAME) - }) - .count() +fn diagnostic_count(output: &[u8]) -> Result<usize, String> { + let mut count = 0usize; + + for message in Message::parse_stream(Cursor::new(output)) { + let message = message.map_err(|err| format!("invalid cargo JSON message: {err}"))?; + if let Message::CompilerMessage(message) = message { + if message + .message + .code + .as_ref() + .is_some_and(|code| code.code == LINT_CRATE_NAME) + { + count += 1; + } + } + } + + Ok(count) } @@ - let result = run_cargo_dylint(fixture_dir, lint_library_path); - let count = diagnostic_count(&result.stdout); + let result = run_cargo_dylint(fixture_dir, lint_library_path); @@ - if expectation.should_emit_diagnostics { + let count = diagnostic_count(&result.stdout).unwrap_or_else(|err| { + panic!( + "crate `{crate_name}` produced non-JSON cargo output: {err}\nstderr was:\n{}", + result.stderr + ) + }); + + if expectation.should_emit_diagnostics {Also applies to: 325-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/no_std_fs_operations/tests/integration_exclusion.rs` around lines 264 - 267, Change diagnostic_count to return Result<usize, E> (e.g., Result<usize, anyhow::Error>) instead of panicking: have it attempt to parse messages with Message::parse_stream and propagate any parse errors (including context of the raw stdout) rather than calling expect(). Update callers (notably assert_fixture_behaviour) to handle the Result: on Err, produce an actionable failure that includes the crate name and the captured stderr/stdout so users can see the malformed cargo output; on Ok, continue using the usize count. Also update the other affected call sites (the block around the other occurrence at the 325-333 region) to use the new Result-returning diagnostic_count and handle errors similarly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/no_std_fs_operations/tests/integration_exclusion.rs`:
- Around line 264-267: Change diagnostic_count to return Result<usize, E> (e.g.,
Result<usize, anyhow::Error>) instead of panicking: have it attempt to parse
messages with Message::parse_stream and propagate any parse errors (including
context of the raw stdout) rather than calling expect(). Update callers (notably
assert_fixture_behaviour) to handle the Result: on Err, produce an actionable
failure that includes the crate name and the captured stderr/stdout so users can
see the malformed cargo output; on Ok, continue using the usize count. Also
update the other affected call sites (the block around the other occurrence at
the 325-333 region) to use the new Result-returning diagnostic_count and handle
errors similarly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f34bf15-818f-47a8-b179-2853eab463ce
📒 Files selected for processing (1)
crates/no_std_fs_operations/tests/integration_exclusion.rs
`diagnostic_count` in the exclusion integration tests previously called `.expect()` on each `Message::parse_stream` result, panicking on the first malformed JSON line. This produced unhelpful diagnostics when cargo dylint emitted unexpected output. Change `diagnostic_count` to collect parse results with `collect::<Result<Vec<_>, _>>()` and propagate parse errors via `anyhow::Error`, including the raw stdout in the error message so callers can inspect the malformed output. Update `assert_fixture_behaviour` to handle the `Result` by producing an actionable panic that includes the crate name and captured stderr alongside the parse error.
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.
This comment was marked as resolved.
This comment was marked as resolved.
Convert all fallible build-layer and fixture-creation functions to return `anyhow::Result` instead of panicking with `.expect()`. Separate the subprocess-execution portion of `assert_fixture_behaviour` into `evaluate_fixture`, which returns `Result<(bool, usize)>` so callers can inspect results programmatically. Store `lint_library_path` as `OnceLock<anyhow::Result<PathBuf>>` so a build failure propagates to callers rather than being silently cached. Add `use anyhow::Context as _;` and use `.context()` / `.with_context()` throughout for actionable error messages.
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.
Sorry, something went wrong.
|
@coderabbitai these should both be dealt with as part of the current PR, not as separate issues. The first is a simple documentation change, the second is introduction of standard tests. Insta is a standard part of the testing toolkit used at df12, and is not some weird outlier.
Please suggest a fix for these concerns and provide an AI coding agent prompt for the fixes. |
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.
Sorry, something went wrong.
Update the developer guide and roadmap to describe the runtime-generated `TempDir` fixture projects used by the `no_std_fs_operations` exclusion tests. Add an insta JSON snapshot for the non-excluded diagnostic path so the structured `cargo dylint` output is covered without relying on absolute fixture paths.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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)
docs/developers-guide.md (1)
58-85:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse Oxford spellings and fix the missing comma in this new section.
Replace
parametrised/serialisedwithparametrized/serialized, and add the comma afterFor each casein the final paragraph.Triage:
[type:spelling][type:grammar][type:docstyle]Suggested fix
-The shared helper `run_exclusion_test(crate_name, is_excluded, expectation)` +The shared helper `run_exclusion_test(crate_name, is_excluded, expectation)` resolves the lint library path via a `OnceLock`-cached `build_lint_library` call, creates the fixture project, and delegates to `assert_fixture_behaviour`. -Both parametrised cases in `exclusion_crates_behaviour_test` delegate to this +Both parametrized cases in `exclusion_crates_behaviour_test` delegate to this helper. @@ -for correct serialised execution because nextest runs each test in a separate +for correct serialized execution because nextest runs each test in a separate process, so the in-process `#[serial]` mutex alone is not sufficient. @@ -The parametrised `#[rstest]` case `exclusion_crates_behaviour_test` covers both -fixture configurations. For each case it asserts the subprocess exit status and +The parametrized `#[rstest]` case `exclusion_crates_behaviour_test` covers both +fixture configurations. For each case, it asserts the subprocess exit status and the `no_std_fs_operations` diagnostic count, so the test verifies both the success path for excluded crates (zero diagnostics, exit 0) and the failure path for non-excluded crates (one or more diagnostics, non-zero exit).As per coding guidelines, "Documentation must use en-GB-oxendict spelling and grammar" and "Use the Oxford comma: 'ships, planes, and hovercraft' where it aids comprehension".
🤖 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 `@docs/developers-guide.md` around lines 58 - 85, Replace the British "-ised" spellings with Oxford "-ized" forms by changing "parametrised" to "parametrized" and "serialised" to "serialized" (maintaining en-GB-oxendict style), and add the missing comma after the phrase "For each case" in the final paragraph so it reads "For each case, it asserts..."; locate these tokens in the section describing run_exclusion_test and the parametrised #[rstest] paragraph to apply the edits.
🤖 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/no_std_fs_operations/tests/integration_exclusion.rs`:
- Around line 174-186: The generated Cargo.toml written by the fs::write call
for root.join("Cargo.toml") needs an empty workspace table to prevent Cargo from
inheriting an outer workspace; modify the format! argument (the concat! block)
used in that fs::write invocation to include a line for "[workspace]\n" (e.g.,
insert "\n[workspace]\n" or add "[workspace]\n" into the concatenated string) so
the written manifest contains an explicit empty workspace entry while still
using the same crate_name interpolation.
- Around line 454-468: The pipeline silently drops parse/serialize errors by
using filter_map(Result::ok) and .ok(); change it so
Message::parse_stream(Cursor::new(&result.stdout)) is collected as a Result
sequence and any Err from parsing or any Err from serde_json::to_value on the
CompilerMessage case fails the test immediately (use collect::<Result<Vec<_>,
_>>() or map + expect with a clear message) instead of filtering them
out—specifically replace filter_map(Result::ok) and the .ok() after
serde_json::to_value with error-propagating handling so malformed parse results
or serialization failures surface as test failures.
---
Outside diff comments:
In `@docs/developers-guide.md`:
- Around line 58-85: Replace the British "-ised" spellings with Oxford "-ized"
forms by changing "parametrised" to "parametrized" and "serialised" to
"serialized" (maintaining en-GB-oxendict style), and add the missing comma after
the phrase "For each case" in the final paragraph so it reads "For each case, it
asserts..."; locate these tokens in the section describing run_exclusion_test
and the parametrised #[rstest] paragraph to apply the edits.
🪄 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: 40cc9bac-6400-4419-bd5b-81995b309ef7
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockcrates/no_std_fs_operations/tests/snapshots/integration_exclusion__non_excluded_crate_diagnostics.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
Cargo.tomlcrates/no_std_fs_operations/Cargo.tomlcrates/no_std_fs_operations/tests/integration_exclusion.rsdocs/developers-guide.mddocs/roadmap.md
| fs::write( | ||
| root.join("Cargo.toml"), | ||
| format!( | ||
| concat!( | ||
| "[package]\n", | ||
| "name = \"{crate_name}\"\n", | ||
| "version = \"0.1.0\"\n", | ||
| "edition = \"2024\"\n", | ||
| "\n", | ||
| "[dependencies]\n", | ||
| ), | ||
| crate_name = crate_name | ||
| ), |
There was a problem hiding this comment.
Add an empty [workspace] to the generated fixture manifest.
Emit the empty workspace table here. Without it, Cargo can walk upwards from the temp directory and attach the fixture to an enclosing workspace when TMPDIR points inside a checkout, which makes this integration test environment-dependent.
Suggested fix
fs::write(
root.join("Cargo.toml"),
format!(
concat!(
"[package]\n",
"name = \"{crate_name}\"\n",
"version = \"0.1.0\"\n",
"edition = \"2024\"\n",
"\n",
+ "[workspace]\n",
+ "\n",
"[dependencies]\n",
),
crate_name = crate_name
),
)📝 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.
| fs::write( | |
| root.join("Cargo.toml"), | |
| format!( | |
| concat!( | |
| "[package]\n", | |
| "name = \"{crate_name}\"\n", | |
| "version = \"0.1.0\"\n", | |
| "edition = \"2024\"\n", | |
| "\n", | |
| "[dependencies]\n", | |
| ), | |
| crate_name = crate_name | |
| ), | |
| fs::write( | |
| root.join("Cargo.toml"), | |
| format!( | |
| concat!( | |
| "[package]\n", | |
| "name = \"{crate_name}\"\n", | |
| "version = \"0.1.0\"\n", | |
| "edition = \"2024\"\n", | |
| "\n", | |
| "[workspace]\n", | |
| "\n", | |
| "[dependencies]\n", | |
| ), | |
| crate_name = crate_name | |
| ), |
🤖 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/no_std_fs_operations/tests/integration_exclusion.rs` around lines 174
- 186, The generated Cargo.toml written by the fs::write call for
root.join("Cargo.toml") needs an empty workspace table to prevent Cargo from
inheriting an outer workspace; modify the format! argument (the concat! block)
used in that fs::write invocation to include a line for "[workspace]\n" (e.g.,
insert "\n[workspace]\n" or add "[workspace]\n" into the concatenated string) so
the written manifest contains an explicit empty workspace entry while still
using the same crate_name interpolation.
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd 'mod.rs' --full-path --glob '*test_support*' | xargs cat
Length of output: 102
@leynos — Not resolved. The [workspace] table is still absent from the generated manifest. crates/no_std_fs_operations/tests/test_support/mod.rs currently writes:
[package]
name = "{crate_name}"
version = "0.1.0"
edition = "2024"
[dependencies]
The [workspace] table must be inserted before [dependencies].
Apply the fix below:
In `crates/no_std_fs_operations/tests/test_support/mod.rs`, locate the `fs::write` call for `root.join("Cargo.toml")` inside `create_fixture_project`. The `concat!` block currently contains:
```
"[package]\n",
"name = \"{crate_name}\"\n",
"version = \"0.1.0\"\n",
"edition = \"2024\"\n",
"\n",
"[dependencies]\n",
```
Insert two lines immediately after the blank line (`"\n"`) and before `"[dependencies]\n"`:
```
"[workspace]\n",
"\n",
```
so the manifest written to disk becomes:
```
[package]
name = "<crate_name>"
version = "0.1.0"
edition = "2024"
[workspace]
[dependencies]
```
This prevents Cargo from walking upward and attaching the temporary fixture to an enclosing workspace when `TMPDIR` is located inside a checkout tree.
🧬 Code Graph Analysis Results
- File: crates/no_std_fs_operations/src/driver.rs, lines 89-91
pub fn is_excluded(&self, crate_name: &str) -> bool {
self.excluded_crates.contains(crate_name)
}
- File: crates/no_std_fs_operations/tests/test_support/mod.rs, lines 32-70
pub(super) fn create_fixture_project(
crate_name: &str,
is_excluded: bool,
) -> anyhow::Result<FixtureProject> {
let temp_dir = TempDir::new().context("failed to create temporary fixture directory")?;
let root = temp_dir.path().to_path_buf();
fs::write(
root.join("Cargo.toml"),
format!(
concat!(
"[package]\n",
"name = \"{crate_name}\"\n",
"version = \"0.1.0\"\n",
"edition = \"2024\"\n",
"\n",
"[dependencies]\n",
),
crate_name = crate_name
),
)
.context("failed to write fixture Cargo.toml")?;
fs::write(
root.join("dylint.toml"),
fixture_dylint_config(crate_name, is_excluded),
)
.context("failed to write fixture dylint.toml")?;
let source_dir = root.join("src");
fs::create_dir(&source_dir).context("failed to create fixture src directory")?;
fs::write(source_dir.join("lib.rs"), fixture_source(crate_name))
.context("failed to write fixture source")?;
Ok(FixtureProject {
_temp_dir: temp_dir,
root,
})
}
🧠 Learnings used
Learnt from: leynos
Repo: leynos/whitaker PR: 130
File: crates/module_must_have_inner_docs/src/driver/inner_attr.rs:66-66
Timestamp: 2026-02-01T03:25:03.269Z
Learning: In Rust, when a variable is initialized with an integer literal and the only subsequent usage is a method like saturating_sub that exists on multiple integer types, an explicit type annotation is required. Do not rely on the compiler to infer the concrete integer type from the method alone; if the code could apply to multiple integer types, specify the type (e.g., 'let mut depth: usize = 1;') to ensure the intended type is used and avoid type-ambiguity during compilation.
Learnt from: leynos
Repo: leynos/whitaker PR: 140
File: installer/tests/behaviour_toolchain.rs:0-0
Timestamp: 2026-02-04T00:00:24.433Z
Learning: In Rust, use 'use path as _;' to bring trait methods into scope without binding the trait name. This pattern does not work for functions; importing a function with 'as _' discards the name and makes the function uncallable. Apply this guidance to Rust source files (not just tests) by ensuring underscore imports are used only for traits, not for functions.
leynos
changed the title
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 145412
|
Route exclusion fixture assertions through the fallible evaluation helper so subprocess execution and diagnostic parsing stay outside the assertion logic. Keep the ignored integration test attributes unchanged while preserving the existing fixture behaviour checks for excluded and non-excluded crates.
Move the temporary fixture generator into test support and keep its exposed surface narrow for the integration exclusion tests. Return `anyhow::Result` from the exclusion test driver so setup failures carry context through the test harness instead of panicking via `expect`.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Replace the blanket Clippy allow on `lint_library_path` with an explicit `expect` that records why the `Result` conversion needs the current shape.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/no_std_fs_operations/tests/integration_exclusion.rs (1)
353-367:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail the snapshot test on malformed output instead of silently dropping errors.
The pipeline uses
filter_map(Result::ok)at line 354 and.ok()at line 363, which silently discard parse and serialisation failures. This contradicts the approach indiagnostic_count(lines 187-195) which correctly propagates errors. A broken harness or unexpected cargo output format change would produce a partial or empty snapshot rather than a failing test.Collect parse results fallibly and propagate serialisation errors so the test fails loudly on malformed output.
Proposed fix
- let diagnostics: Vec<serde_json::Value> = Message::parse_stream(Cursor::new(&result.stdout)) - .filter_map(Result::ok) + let messages: Vec<Message> = Message::parse_stream(Cursor::new(&result.stdout)) + .collect::<Result<Vec<_>, _>>() + .unwrap_or_else(|e| { + panic!( + "non_excluded_crate_snap produced malformed cargo output: {e}\nstderr:\n{}", + result.stderr + ) + }); + + let diagnostics: Vec<serde_json::Value> = messages + .into_iter() .filter_map(|message| match message { Message::CompilerMessage(message) if message .message .code .as_ref() .is_some_and(|code| code.code == LINT_CRATE_NAME) => { - serde_json::to_value(message.message).ok() + Some( + serde_json::to_value(&message.message) + .expect("failed to serialise diagnostic for snapshot"), + ) } _ => None, }) .collect();🤖 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/no_std_fs_operations/tests/integration_exclusion.rs` around lines 353 - 367, The current test silently drops parse and serialization failures by using filter_map(Result::ok) and .ok(); change the pipeline to collect and propagate errors so the test fails on malformed output: replace the filter_map(Result::ok) and the .ok() call in the chain that starts from Message::parse_stream(Cursor::new(&result.stdout)) with a fallible mapping that returns Result<serde_json::Value, E> for each message (map parse errors through and convert serde_json::to_value errors into Err), then collect::<Result<Vec<serde_json::Value>, _>>() (or use try_collect) to get a Result<Vec<_>, _> and unwrap or propagate the error in the test; reference Message::parse_stream and the conversion via serde_json::to_value when making the change.
🤖 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.
Duplicate comments:
In `@crates/no_std_fs_operations/tests/integration_exclusion.rs`:
- Around line 353-367: The current test silently drops parse and serialization
failures by using filter_map(Result::ok) and .ok(); change the pipeline to
collect and propagate errors so the test fails on malformed output: replace the
filter_map(Result::ok) and the .ok() call in the chain that starts from
Message::parse_stream(Cursor::new(&result.stdout)) with a fallible mapping that
returns Result<serde_json::Value, E> for each message (map parse errors through
and convert serde_json::to_value errors into Err), then
collect::<Result<Vec<serde_json::Value>, _>>() (or use try_collect) to get a
Result<Vec<_>, _> and unwrap or propagate the error in the test; reference
Message::parse_stream and the conversion via serde_json::to_value when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3f7ea3a9-f6d3-4ae2-9d7d-92e1f1e4fd26
📒 Files selected for processing (1)
crates/no_std_fs_operations/tests/integration_exclusion.rs
|
@coderabbitai Have the following now been resolved? crates/no_std_fs_operations/tests/integration_exclusion.rs (1)
Patchfn find_cdylib_in_artifacts(
stdout: &[u8],
package_id: &cargo_metadata::PackageId,
) -> anyhow::Result<PathBuf> {
for message in Message::parse_stream(Cursor::new(stdout)) {
- let Ok(Message::CompilerArtifact(artifact)) = message else {
+ let message = message.context("failed to parse cargo build JSON output")?;
+ let Message::CompilerArtifact(artifact) = message else {
continue;
};
if artifact.package_id != *package_id {
continue;🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In
crates/no_std_fs_operations/tests/integration_exclusion.rs (2)
Patchlet diagnostics: Vec<serde_json::Value> = Message::parse_stream(Cursor::new(&result.stdout))
- .filter_map(Result::ok)
+ .collect::<Result<Vec<_>, _>>()
+ .unwrap_or_else(|e| {
+ panic!(
+ "non_excluded_crate_diagnostics_match_snapshot produced malformed cargo output: {e}\nstderr:\n{}",
+ result.stderr
+ )
+ })
+ .into_iter()
.filter_map(|message| match message {
Message::CompilerMessage(message)
if message
.message
.code
.as_ref()
.is_some_and(|code| code.code == LINT_CRATE_NAME) =>
{
- serde_json::to_value(message.message).ok()
+ Some(
+ serde_json::to_value(message.message)
+ .expect("failed to serialise diagnostic for snapshot"),
+ )
}
_ => None,
})
.collect();🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In
Patch-#[allow(clippy::useless_asref, clippy::redundant_closure)]
+#[expect(
+ clippy::useless_asref,
+ reason = "anyhow::Error is not Clone; .as_ref().map(Clone::clone) converts &Result<PathBuf, Error> into Result<PathBuf, Error>"
+)]
+#[expect(
+ clippy::redundant_closure,
+ reason = "closure form is required for the non-Clone anyhow::Error conversion pattern"
+)]
fn lint_library_path() -> anyhow::Result<PathBuf> {As per coding guidelines, " 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In
crates/no_std_fs_operations/tests/test_support/mod.rs (1)
Patchconcat!(
"[package]\n",
"name = \"{crate_name}\"\n",
"version = \"0.1.0\"\n",
"edition = \"2024\"\n",
"\n",
+ "[workspace]\n",
+ "\n",
"[dependencies]\n",
),🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
|
@coderabbitai Have the following now been resolved? crates/no_std_fs_operations/tests/integration_exclusion.rs (1)
Suggested structure//! Helpers for building and locating the lint library in integration tests.
use std::env;
use std::io::Cursor;
use std::path::{Path, PathBuf};
use std::process::Command;
use std::sync::OnceLock;
use anyhow::Context as _;
use cargo_metadata::{Message, Metadata, MetadataCommand};
pub const LINT_CRATE_NAME: &str = "no_std_fs_operations";
pub fn lint_library_path() -> anyhow::Result<PathBuf> { /* ... */ }
fn build_lint_library() -> anyhow::Result<PathBuf> { /* ... */ }
// ... remaining build helpersAs per coding guidelines, "Files must not exceed 400 lines in length" and "Large modules must be decomposed." 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7251
Below are the agent prompts for the three remaining issues. 1 —
|
|
@coderabbitai Have the following now been resolved? crates/no_std_fs_operations/tests/integration_exclusion.rs (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 15932
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 20002
Note that three findings from the previous round remain unresolved in the current codegraph. Supply the following prompt to your AI coding agent to address them all in one pass: |
Propagate integration fixture evaluation and snapshot setup errors through `anyhow::Result` instead of converting them into hidden panics. Encode generated fixture TOML values through `toml::Value` so crate names cannot inject manifest or `dylint.toml` keys, and add regression coverage for escaped names. Serialise the ignored exclusion integration binary under nextest's `serial-dylint-ui` group because it builds and stages the lint library.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Propagate cargo JSON parse errors while locating the built lint cdylib so malformed build output reports the parsing failure directly. Make the snapshot test fail on malformed cargo output or diagnostic serialisation failures instead of silently dropping bad records. Add an empty `[workspace]` table to generated fixture manifests so each temporary crate is resolved as its own workspace root.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Align the exclusion snapshot test panic paths with the crate-specific fixture name. Keep malformed cargo JSON and diagnostic serialisation failures explicit so the snapshot cannot be built from partial output.
1 participant
Summary
Changes
FixtureProjectstruct to hold aTempDirand root pathcreate_fixture_project(crate_name, is_excluded)to build a temporary crate withCargo.toml,dylint.toml, andsrc/lib.rsfixture_dylint_config(crate_name, is_excluded)to configureexcluded_cratesaccording to the testfixture_source(crate_name)to generate a small crate source implementingopen_fileassert_fixture_behaviour(...)helper to run cargo-dylint and validate resultsexcluded_crate_suppresses_diagnosticstestnon_excluded_crate_emits_diagnosticstesttempfilefor temporary directories and updated relevant crates to reference ittests/fixtures(e.g.,excluded_projectandnon_excluded_project)run_cargo_dylint,diagnostic_countusage)Why
Migration / Compatibility
#[ignore]by default because they requirecargo-dylintand a built lint library.Test plan
cargo test -p whitaker --test integration_exclusion -- --ignoredDiffs touched
tempfiledependency for teststempfile(dependency graph reflects changes)◳ Generated by DevBoxer ◰
ℹ️ Tag @devboxerhub to ask questions and address PR feedback
📎 Task: https://www.devboxer.com/task/a2b8d62b-4c8f-4dc0-b582-42de342d2760
📝 Closes #111
Summary by Sourcery
Replace static on-disk lint exclusion fixtures with dynamically generated temporary projects in integration tests.
New Features:
Enhancements:
Build:
Tests: