Conversation
Reviewer's GuideThe PR enhances test infrastructure by ensuring the process step can auto-generate a missing manifest before executing, and by introducing a reusable helper for creating a minimal manifest in tests. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Summary by CodeRabbit
WalkthroughEnhance the test step for running the Ninja process to ensure the manifest file exists and is correctly written before execution. Add a new utility function for writing a minimal manifest, and update documentation to reflect the expanded test support capabilities. Changes
Sequence Diagram(s)sequenceDiagram
participant TestStep as Test Step
participant Support as Support Utils
participant FileSystem as File System
participant Ninja as Ninja Process
TestStep->>FileSystem: Check if manifest file exists
alt Manifest missing
TestStep->>Support: Call write_manifest
Support->>FileSystem: Write minimal manifest
end
TestStep->>FileSystem: Update CLI manifest path
TestStep->>Ninja: Run Ninja process with manifest
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- The block in run() that resolves the manifest path and creates a missing manifest could be extracted into a dedicated test helper to reduce verbosity and centralize setup logic.
- Instead of using NamedTempFile::persist (which overwrites existing files), consider using fs::write or NamedTempFile::persist_noclobber for more explicit control when creating the manifest.
- The write_manifest helper panics on write errors—consider returning a Result or propagating errors to get clearer diagnostics if the manifest fails to write.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The block in run() that resolves the manifest path and creates a missing manifest could be extracted into a dedicated test helper to reduce verbosity and centralize setup logic.
- Instead of using NamedTempFile::persist (which overwrites existing files), consider using fs::write or NamedTempFile::persist_noclobber for more explicit control when creating the manifest.
- The write_manifest helper panics on write errors—consider returning a Result or propagating errors to get clearer diagnostics if the manifest fails to write.
## Individual Comments
### Comment 1
<location> `tests/steps/process_steps.rs:59` </location>
<code_context>
)]
#[when("the ninja process is run")]
fn run(world: &mut CliWorld) {
+ let dir = world.temp.as_ref().expect("temp dir");
+ let manifest_path = {
+ let cli = world.cli.as_ref().expect("cli");
+ if cli.file.is_absolute() {
+ cli.file.clone()
+ } else {
+ dir.path().join(&cli.file)
+ }
+ };
+
+ if !manifest_path.exists() {
+ let mut file =
+ NamedTempFile::new_in(dir.path()).expect("Failed to create temporary manifest file");
+ support::write_manifest(&mut file);
+ file.persist(&manifest_path)
+ .expect("Failed to persist manifest file");
+ }
+
+ {
+ let cli = world.cli.as_mut().expect("cli");
+ cli.file.clone_from(&manifest_path);
+ }
+
</code_context>
<issue_to_address>
Consider adding tests for absolute vs. relative manifest paths.
Please add tests to verify correct handling and manifest creation for both absolute and relative paths.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
use netsuke::runner;
use std::path::PathBuf;
use tempfile::{NamedTempFile, TempDir};
/// Installs a test-specific ninja binary and updates the `PATH`.
#[expect(
)]
#[when("the ninja process is run")]
fn run(world: &mut CliWorld) {
=======
use netsuke::runner;
use std::path::{Path, PathBuf};
use tempfile::{NamedTempFile, TempDir};
#[cfg(test)]
mod tests {
use super::*;
use std::fs;
use std::env;
struct DummyCli {
file: PathBuf,
}
struct DummyWorld {
temp: Option<TempDir>,
cli: Option<DummyCli>,
}
fn setup_temp_dir() -> TempDir {
tempfile::tempdir().expect("Failed to create temp dir")
}
#[test]
fn test_relative_manifest_path_creates_file() {
let temp_dir = setup_temp_dir();
let rel_path = PathBuf::from("manifest.json");
let mut world = DummyWorld {
temp: Some(temp_dir),
cli: Some(DummyCli { file: rel_path.clone() }),
};
// Simulate the run logic
let dir = world.temp.as_ref().expect("temp dir");
let manifest_path = dir.path().join(&rel_path);
assert!(!manifest_path.exists(), "Manifest should not exist before run");
// Create manifest as in run()
let mut file = NamedTempFile::new_in(dir.path()).expect("Failed to create temporary manifest file");
writeln!(file, "{{\"test\": true}}").unwrap();
file.persist(&manifest_path).expect("Failed to persist manifest file");
assert!(manifest_path.exists(), "Manifest should exist after run");
}
#[test]
fn test_absolute_manifest_path_creates_file() {
let temp_dir = setup_temp_dir();
let abs_path = temp_dir.path().join("abs_manifest.json");
let mut world = DummyWorld {
temp: Some(temp_dir),
cli: Some(DummyCli { file: abs_path.clone() }),
};
// Simulate the run logic
let manifest_path = abs_path.clone();
assert!(!manifest_path.exists(), "Manifest should not exist before run");
// Create manifest as in run()
let mut file = NamedTempFile::new_in(manifest_path.parent().unwrap()).expect("Failed to create temporary manifest file");
writeln!(file, "{{\"test\": true}}").unwrap();
file.persist(&manifest_path).expect("Failed to persist manifest file");
assert!(manifest_path.exists(), "Manifest should exist after run");
}
}
/// Installs a test-specific ninja binary and updates the `PATH`.
#[expect(
)]
#[when("the ninja process is run")]
fn run(world: &mut CliWorld) {
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/steps/process_steps.rs(2 hunks)tests/support/mod.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/support/mod.rstests/steps/process_steps.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/support/mod.rstests/steps/process_steps.rs
🧬 Code Graph Analysis (1)
tests/steps/process_steps.rs (2)
tests/steps/cli_steps.rs (1)
manifest_path(94-97)tests/support/mod.rs (1)
write_manifest(33-46)
🔇 Additional comments (4)
tests/support/mod.rs (2)
2-4: LGTM! Well-structured module documentation.The expanded documentation clearly describes the module's purpose and utility, adhering to the requirement for module-level comments.
29-46: LGTM! Well-implemented utility function.The function follows best practices:
- Proper Rustdoc documentation
- Uses
concat!()for string literals as required- Uses
.expect()over.unwrap()as mandated- Clear panic message for write failures
tests/steps/process_steps.rs (2)
7-7: LGTM! Appropriate import for temporary file handling.The addition of
NamedTempFileimport supports the new manifest creation functionality.
59-80: LGTM! Robust manifest file handling implementation.The implementation correctly:
- Determines absolute vs relative manifest paths
- Creates missing manifest files using the shared utility
- Persists temporary files to the correct location
- Updates the CLI configuration appropriately
- Uses
.expect()with clear error messages as required by guidelinesThe logic ensures tests have the necessary manifest files whilst maintaining clean separation of concerns.
Ensure manifests exist before running ninja and provide helper to generate a default manifest for tests.
leynos
force-pushed
the
codex/fix-mutable-borrow-and-improve-error-handling
branch
from
09115f8 to
3ce4b9b
Compare
|
@coderabbitai Have these now been resolved?
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos No, these issues have not been resolved. Looking at the current implementation: 1. Manifest path resolution block not extracted: 2. Still using 3. The refactoring suggestions from the previous review remain unaddressed. Would you like me to create an issue to track these improvements for a future PR? 🐰💭 🏁 Script executed: Length of output: 4447 🏁 Script executed: Length of output: 3662 @leynos No, these issues have not been resolved. Here's what I found in the current code: 1. Manifest path resolution block not extracted (Lines 58-74 in 2. Still using file.persist(&manifest_path)
.expect("Failed to persist manifest file");The code continues using 3. .expect("write manifest content");The function still uses None of the refactoring suggestions have been implemented. The code retains the same structure and error handling patterns that were flagged in the previous review. 🐰📝 |
|
Closes #57 |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/steps/process_steps.rs(2 hunks)tests/support/mod.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider usingArcto reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//!) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessarymutbindings.
Handle errors with theResulttype instead of panicking where feasible.
Avoidunsafecode unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not usereturnin single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Preferexpectoverallow.
Prefer.expect()over.unwrap().
Useconcat!()to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums: Derivestd::error::Error(via thethiserrorcrate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary: Useeyre::Reportfor human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library: Convert to domain enums at API boundaries, and toeyreonly in the mainmain()entrypoint or top-level async task.
Files:
tests/support/mod.rstests/steps/process_steps.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/support/mod.rstests/steps/process_steps.rs
🧬 Code Graph Analysis (1)
tests/steps/process_steps.rs (2)
tests/steps/cli_steps.rs (1)
manifest_path(94-97)tests/support/mod.rs (1)
write_manifest(101-114)
🪛 GitHub Check: build-test
tests/support/mod.rs
[failure] 100-100:
this lint expectation is unfulfilled
🪛 GitHub Actions: CI
tests/support/mod.rs
[error] 100-100: Clippy lint error: this lint expectation is unfulfilled.
🔇 Additional comments (2)
tests/support/mod.rs (1)
1-4: LGTM!The expanded module documentation clearly explains the dual purpose of providing helpers for both fake executables and manifest generation.
tests/steps/process_steps.rs (1)
7-7: LGTM!The import additions support the new manifest handling functionality.
| let dir = world.temp.as_ref().expect("temp dir"); | ||
| let manifest_path = { | ||
| let cli = world.cli.as_ref().expect("cli"); | ||
| if cli.file.is_absolute() { | ||
| cli.file.clone() | ||
| } else { | ||
| dir.path().join(&cli.file) | ||
| } | ||
| }; | ||
|
|
||
| if !manifest_path.exists() { | ||
| let mut file = | ||
| NamedTempFile::new_in(dir.path()).expect("Failed to create temporary manifest file"); | ||
| support::write_manifest(&mut file); | ||
| file.persist(&manifest_path) | ||
| .expect("Failed to persist manifest file"); | ||
| } | ||
|
|
||
| { | ||
| let cli = world.cli.as_mut().expect("cli"); | ||
| cli.file.clone_from(&manifest_path); | ||
| } | ||
|
|
There was a problem hiding this comment.
Extract manifest path resolution logic to a dedicated helper function.
This block contains verbose setup logic that should be centralised for maintainability, as noted in the PR objectives. The manifest path resolution and creation logic remains inline rather than being extracted to a reusable helper.
Create a helper function in the support module:
pub fn ensure_manifest_exists(
temp_dir: &Path,
cli_file: &Path
) -> std::io::Result<PathBuf> {
let manifest_path = if cli_file.is_absolute() {
cli_file.to_path_buf()
} else {
temp_dir.join(cli_file)
};
if !manifest_path.exists() {
let mut file = NamedTempFile::new_in(temp_dir)?;
write_manifest(&mut file)?;
file.persist(&manifest_path)?;
}
Ok(manifest_path)
}Then simplify the run function:
fn run(world: &mut CliWorld) {
let dir = world.temp.as_ref().expect("temp dir");
- let manifest_path = {
- let cli = world.cli.as_ref().expect("cli");
- if cli.file.is_absolute() {
- cli.file.clone()
- } else {
- dir.path().join(&cli.file)
- }
- };
-
- if !manifest_path.exists() {
- let mut file =
- NamedTempFile::new_in(dir.path()).expect("Failed to create temporary manifest file");
- support::write_manifest(&mut file);
- file.persist(&manifest_path)
- .expect("Failed to persist manifest file");
- }
-
- {
- let cli = world.cli.as_mut().expect("cli");
- cli.file.clone_from(&manifest_path);
- }
+ let cli_file = world.cli.as_ref().expect("cli").file.clone();
+ let manifest_path = support::ensure_manifest_exists(dir.path(), &cli_file)
+ .expect("Failed to ensure manifest exists");
+
+ world.cli.as_mut().expect("cli").file = manifest_path;🤖 Prompt for AI Agents
In tests/steps/process_steps.rs around lines 59 to 81, the manifest path
resolution and creation logic is verbose and inline, reducing maintainability.
Extract this logic into a helper function named ensure_manifest_exists in the
support module that takes the temporary directory path and CLI file path as
parameters, checks if the manifest path is absolute or relative, creates the
manifest file if it does not exist, and returns the manifest path. Then,
simplify the run function by calling this helper and updating the CLI file path
accordingly.
There was a problem hiding this comment.
@coderabbitai Please create a new GitHub issue for this
There was a problem hiding this comment.
🧩 Analysis chain
I'll create a GitHub issue to track this improvement.
@leynos I've created a GitHub issue to track the extraction of the manifest path resolution logic into a helper function. This will help centralise the verbose setup logic and improve maintainability as discussed.
🏁 Script executed:
#!/bin/bash
gh issue create \
--title "Extract manifest path resolution logic into helper function" \
--body "## Description
The manifest path resolution and creation logic in the \`run()\` function within \`tests/steps/process_steps.rs\` is verbose and inline, reducing maintainability. This logic should be extracted into a dedicated helper function for better code organisation.
## Current Problem
The current implementation contains approximately 23 lines of inline logic for:
- Resolving absolute vs relative manifest paths
- Creating temporary manifest files when they don't exist
- Persisting the manifest files
- Updating the CLI configuration
## Proposed Solution
Extract this logic into a helper function \`ensure_manifest_exists\` in the support module that:
- Takes temporary directory path and CLI file path as parameters
- Handles absolute/relative path resolution
- Creates manifest files when missing
- Returns the resolved manifest path
- Allows the main \`run()\` function to be simplified
## References
- Original PR: https://github.com/leynos/netsuke/pull/56
- Discussion: https://github.com/leynos/netsuke/pull/56#discussion_r2255505417
- Requested by: @leynos" \
--assignee leynos
Length of output: 1232
@leynos The GitHub issue has been successfully created at #59. The issue is assigned to you and includes all the relevant details about extracting the manifest path resolution logic into a helper function, along with references to the original PR and discussion.
🧬 Code Graph Analysis Results
Relevant snippets from tests/support/mod.rs (lines 17-30, 41-65, 147-159)
pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) {
let dir = TempDir::new().expect("temp dir");
let path = dir.path().join("ninja");
let mut file = File::create(&path).expect("script");
writeln!(file, "#!/bin/sh\nexit {exit_code}").expect("write script");
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
let mut perms = fs::metadata(&path).expect("meta").permissions();
perms.set_mode(0o755);
fs::set_permissions(&path, perms).expect("perms");
}
(dir, path)
}
pub fn fake_ninja_check_build_file() -> (TempDir, PathBuf) {
let dir = TempDir::new().expect("temp dir");
let path = dir.path().join("ninja");
let mut file = File::create(&path).expect("script");
writeln!(
file,
concat!(
"#!/bin/sh\n",
"if [ \"$1\" = \"-f\" ] && [ ! -f \"$2\" ]; then\n",
" echo 'missing build file: $2' >&2\n",
" exit 1\n",
"fi\n",
"exit 0"
)
)
.expect("write script");
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
let mut perms = fs::metadata(&path).expect("meta").permissions();
perms.set_mode(0o755);
fs::set_permissions(&path, perms).expect("perms");
}
(dir, path)
}
pub fn write_manifest(file: &mut impl Write) -> io::Result<()> {
writeln!(
file,
concat!(
"netsuke_version: \"1.0.0\"\n",
"targets:\n",
" - name: hello\n",
" recipe:\n",
" kind: command\n",
" command: \"echo hi\"\n"
),
)
}Relevant snippet from src/runner.rs (lines 236-309)
pub fn run_ninja(
program: &Path,
cli: &Cli,
build_file: &Path,
targets: &BuildTargets,
) -> io::Result<()> {
let mut cmd = Command::new(program);
if let Some(dir) = &cli.directory {
// Resolve and canonicalise the directory so Ninja receives a stable
// absolute path. Using only `current_dir` avoids combining it with
// Ninja's own `-C` flag which would otherwise double-apply the
// directory and break relative paths.
let dir = fs::canonicalize(dir)?;
cmd.current_dir(dir);
}
if let Some(jobs) = cli.jobs {
cmd.arg("-j").arg(jobs.to_string());
}
// Canonicalise the build file path so Ninja resolves it correctly from the
// working directory. Fall back to the original on failure so Ninja can
// surface a meaningful error.
let build_file_path = build_file
.canonicalize()
.unwrap_or_else(|_| build_file.to_path_buf());
cmd.arg("-f").arg(&build_file_path);
cmd.args(targets.as_slice());
cmd.stdout(Stdio::piped());
cmd.stderr(Stdio::piped());
let program = cmd.get_program().to_string_lossy().into_owned();
let args: Vec<CommandArg> = cmd
.get_args()
.map(|a| CommandArg::new(a.to_string_lossy().into_owned()))
.collect();
let redacted_args = redact_sensitive_args(&args);
let arg_strings: Vec<&str> = redacted_args.iter().map(CommandArg::as_str).collect();
info!("Running command: {} {}", program, arg_strings.join(" "));
let mut child = cmd.spawn()?;
let stdout = child.stdout.take().expect("child stdout");
let stderr = child.stderr.take().expect("child stderr");
let out_handle = thread::spawn(move || {
let reader = BufReader::new(stdout);
let mut handle = io::stdout();
for line in reader.lines().map_while(Result::ok) {
let _ = writeln!(handle, "{line}");
}
});
let err_handle = thread::spawn(move || {
let reader = BufReader::new(stderr);
let mut handle = io::stderr();
for line in reader.lines().map_while(Result::ok) {
let _ = writeln!(handle, "{line}");
}
});
let status = child.wait()?;
let _ = out_handle.join();
let _ = err_handle.join();
if status.success() {
Ok(())
} else {
#[expect(
clippy::io_other_error,
reason = "use explicit error kind for compatibility with older Rust"
)]
Err(io::Error::new(
io::ErrorKind::Other,
format!("ninja exited with {status}"),
))
}
}Relevant snippet from tests/steps/cli_steps.rs (lines 104-107)
fn manifest_path(world: &mut CliWorld, path: String) {
let cli = world.cli.as_ref().expect("cli");
assert_eq!(cli.file, PathBuf::from(&path));
}| file.persist(&manifest_path) | ||
| .expect("Failed to persist manifest file"); |
There was a problem hiding this comment.
Replace NamedTempFile::persist() with safer alternatives.
Using NamedTempFile::persist() overwrites existing files, as noted in the PR objectives. Use fs::write or persist_noclobber for more explicit control.
- file.persist(&manifest_path)
- .expect("Failed to persist manifest file");
+ drop(file);
+ std::fs::write(&manifest_path, &manifest_content)
+ .expect("Failed to write manifest file");Alternatively, use persist_noclobber:
- file.persist(&manifest_path)
- .expect("Failed to persist manifest file");
+ file.persist_noclobber(&manifest_path)
+ .expect("Failed to persist manifest file");Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/steps/process_steps.rs at lines 73 to 74, replace the use of
NamedTempFile::persist() with a safer alternative to avoid overwriting existing
files. Use either fs::write to write the file contents explicitly or use
persist_noclobber to persist the file without overwriting. This change ensures
more explicit control over file persistence and aligns with the PR objectives.
There was a problem hiding this comment.
@coderabbitai Please create a new GitHub issue for this
There was a problem hiding this comment.
🧩 Analysis chain
@leynos I've created a GitHub issue to track the NamedTempFile::persist() safety concern. The issue includes the technical details, suggested solutions, and references back to this PR and comment for proper context.
🏁 Script executed:
#!/bin/bash
# Create a GitHub issue for the NamedTempFile::persist() safety concern
gh issue create \
--title "Replace NamedTempFile::persist() with safer alternatives in process tests" \
--body "## Problem
The current implementation in \`tests/steps/process_steps.rs\` uses \`NamedTempFile::persist()\` which overwrites existing files without explicit control. This poses a potential safety risk and doesn't align with best practices for file handling.
## Location
- File: \`tests/steps/process_steps.rs\`
- Lines: 73-74 (in the \`run()\` function)
## Suggested Solutions
1. **Use \`fs::write\`** for explicit file writing:
\`\`\`rust
drop(file);
std::fs::write(&manifest_path, &manifest_content)
.expect(\"Failed to write manifest file\");
\`\`\`
2. **Use \`persist_noclobber\`** to avoid overwriting:
\`\`\`rust
file.persist_noclobber(&manifest_path)
.expect(\"Failed to persist manifest file\");
\`\`\`
## Context
This issue was identified during code review of PR #56, where the focus was on improving manifest path handling in process tests.
## References
- PR: https://github.com/leynos/netsuke/pull/56
- Original comment: https://github.com/leynos/netsuke/pull/56#discussion_r2255505420" \
--assignee leynos
Length of output: 1191
@leynos Perfect! I've successfully created GitHub issue #58 to track this improvement. The issue is available at: #58
The issue includes:
- Clear problem description about the NamedTempFile::persist() safety concern
- Specific file location and line numbers
- Two alternative solutions (fs::write and persist_noclobber)
- Context linking back to this PR and comment thread
- Assignment to you for future implementation
This will help ensure the improvement doesn't get lost and can be addressed in a future PR.
🧬 Code Graph Analysis Results
Relevant snippets from tests/support/mod.rs (lines 17-30, 41-65, 147-159)
pub fn fake_ninja(exit_code: i32) -> (TempDir, PathBuf) {
let dir = TempDir::new().expect("temp dir");
let path = dir.path().join("ninja");
let mut file = File::create(&path).expect("script");
writeln!(file, "#!/bin/sh\nexit {exit_code}").expect("write script");
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
let mut perms = fs::metadata(&path).expect("meta").permissions();
perms.set_mode(0o755);
fs::set_permissions(&path, perms).expect("perms");
}
(dir, path)
}
pub fn fake_ninja_check_build_file() -> (TempDir, PathBuf) {
let dir = TempDir::new().expect("temp dir");
let path = dir.path().join("ninja");
let mut file = File::create(&path).expect("script");
writeln!(
file,
concat!(
"#!/bin/sh\n",
"if [ \"$1\" = \"-f\" ] && [ ! -f \"$2\" ]; then\n",
" echo 'missing build file: $2' >&2\n",
" exit 1\n",
"fi\n",
"exit 0"
)
)
.expect("write script");
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
let mut perms = fs::metadata(&path).expect("meta").permissions();
perms.set_mode(0o755);
fs::set_permissions(&path, perms).expect("perms");
}
(dir, path)
}
pub fn write_manifest(file: &mut impl Write) -> io::Result<()> {
writeln!(
file,
concat!(
"netsuke_version: \"1.0.0\"\n",
"targets:\n",
" - name: hello\n",
" recipe:\n",
" kind: command\n",
" command: \"echo hi\"\n"
),
)
}Relevant snippet from src/runner.rs (lines 236-309)
pub fn run_ninja(
program: &Path,
cli: &Cli,
build_file: &Path,
targets: &BuildTargets,
) -> io::Result<()> {
let mut cmd = Command::new(program);
if let Some(dir) = &cli.directory {
// Resolve and canonicalise the directory so Ninja receives a stable
// absolute path. Using only `current_dir` avoids combining it with
// Ninja's own `-C` flag which would otherwise double-apply the
// directory and break relative paths.
let dir = fs::canonicalize(dir)?;
cmd.current_dir(dir);
}
if let Some(jobs) = cli.jobs {
cmd.arg("-j").arg(jobs.to_string());
}
// Canonicalise the build file path so Ninja resolves it correctly from the
// working directory. Fall back to the original on failure so Ninja can
// surface a meaningful error.
let build_file_path = build_file
.canonicalize()
.unwrap_or_else(|_| build_file.to_path_buf());
cmd.arg("-f").arg(&build_file_path);
cmd.args(targets.as_slice());
cmd.stdout(Stdio::piped());
cmd.stderr(Stdio::piped());
let program = cmd.get_program().to_string_lossy().into_owned();
let args: Vec<CommandArg> = cmd
.get_args()
.map(|a| CommandArg::new(a.to_string_lossy().into_owned()))
.collect();
let redacted_args = redact_sensitive_args(&args);
let arg_strings: Vec<&str> = redacted_args.iter().map(CommandArg::as_str).collect();
info!("Running command: {} {}", program, arg_strings.join(" "));
let mut child = cmd.spawn()?;
let stdout = child.stdout.take().expect("child stdout");
let stderr = child.stderr.take().expect("child stderr");
let out_handle = thread::spawn(move || {
let reader = BufReader::new(stdout);
let mut handle = io::stdout();
for line in reader.lines().map_while(Result::ok) {
let _ = writeln!(handle, "{line}");
}
});
let err_handle = thread::spawn(move || {
let reader = BufReader::new(stderr);
let mut handle = io::stderr();
for line in reader.lines().map_while(Result::ok) {
let _ = writeln!(handle, "{line}");
}
});
let status = child.wait()?;
let _ = out_handle.join();
let _ = err_handle.join();
if status.success() {
Ok(())
} else {
#[expect(
clippy::io_other_error,
reason = "use explicit error kind for compatibility with older Rust"
)]
Err(io::Error::new(
io::ErrorKind::Other,
format!("ninja exited with {status}"),
))
}
}Relevant snippet from tests/steps/cli_steps.rs (lines 104-107)
fn manifest_path(world: &mut CliWorld, path: String) {
let cli = world.cli.as_ref().expect("cli");
assert_eq!(cli.file, PathBuf::from(&path));
}Switches from #[allow] to #[expect] where appropriate to make unused item warnings explicit. Keeps one #[allow] on a shared test helper whose usage varies by crate, preventing stray "unfulfilled expect" warnings in some builds.
Changes write_manifest from unwrapping to returning io::Result, letting callers decide how to handle failures and simplifying test diagnostics. The step that generates the temporary manifest now wraps the result with an expect message for clearer panic context.
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| mod.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
| @@ -1,4 +1,7 @@ | |||
| //! Test utilities for process management and log capture. | |||
| //! Test utilities for process management. | |||
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 2 functions with similar structure: fake_ninja,fake_ninja_pwd
There was a problem hiding this comment.
@coderabbitai Please create a new GitHub issue for this
There was a problem hiding this comment.
🧩 Analysis chain
I'll create a GitHub issue to track the code duplication identified by the CodeScene analysis.
@leynos I've created a GitHub issue to track the code duplication identified by CodeScene. The issue documents the duplication between fake_ninja and fake_ninja_pwd functions and suggests extracting common functionality into a shared helper.
🏁 Script executed:
#!/bin/bash
gh issue create \
--title "Refactor code duplication in test support utilities" \
--body "## Problem
CodeScene analysis has identified code duplication between two functions in \`tests/support/mod.rs\`:
- \`fake_ninja\`
- \`fake_ninja_pwd\`
Both functions follow a similar pattern for creating fake executables with temporary directories and shell scripts, but with different script content.
## Suggested Solution
Consider extracting the common functionality into a shared helper function that accepts the script content as a parameter, reducing duplication whilst maintaining the specific behaviour of each function.
## Context
This issue was identified during PR review and flagged by automated analysis.
**References:**
- PR: https://github.com/leynos/netsuke/pull/56
- Comment: https://github.com/leynos/netsuke/pull/56#discussion_r2255550299
**Requested by:** @leynos" \
--assignee leynos
Length of output: 935
@leynos Perfect! I've successfully created GitHub issue #63 to track the code duplication in the test support utilities. The issue documents the duplication between fake_ninja and fake_ninja_pwd functions and includes all the relevant context and references.
Issue URL: #63
The issue has been assigned to you and includes a suggested approach for refactoring the common functionality into a shared helper function.
🧬 Code Graph Analysis Results
Possibly relevant snippets from tests/steps/process_steps.rs (lines 32-35)
fn fake_ninja(world: &mut CliWorld, code: i32) {
let (dir, path) = support::fake_ninja(code);
install_test_ninja(world, dir, path);
}- This function calls
support::fake_ninja(fromtests/support/mod.rs) to create a fake Ninja executable with a specified exit code. - It then installs this fake Ninja executable into the test environment represented by
world. - Parameters:
world: mutable reference to aCliWorldtest context.code: integer exit code for the fake Ninja executable.
- No return value.
Summary of tests/support/mod.rs (the provided file)
- Provides test utilities for process management, especially for creating fake Ninja executables with various behaviors.
- Functions include:
fake_ninja(exit_code: i32) -> (TempDir, PathBuf): creates a fake Ninja executable that exits with the given code.fake_ninja_check_build_file() -> (TempDir, PathBuf): creates a fake Ninja that checks for the existence of a build file.fake_ninja_pwd() -> (TempDir, PathBuf): creates a fake Ninja that writes its current directory to a file.write_manifest(file: &mut impl Write) -> io::Result<()>: writes a minimal manifest to a writable stream.
- Also includes a
capture_logsutility to capture logs emitted during a closure execution. - Uses
tempfile::TempDirfor temporary directories and sets executable permissions on Unix.
Possibly relevant snippets from src/runner.rs (lines 23-25, 40-42, 53-55)
pub fn new(content: String) -> Self {
Self(content)
}
pub fn new(arg: String) -> Self {
Self(arg)
}
pub fn new(targets: Vec<String>) -> Self {
Self(targets)
}- These are simple constructor functions for some types wrapping strings or vectors of strings.
- They may be relevant if the test utilities or fake executables interact with these types, but no direct connection is shown here.
No other snippets appear directly related to the tests/support/mod.rs file or its utilities.
@coderabbitai Please create new GitHub issues for these |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll create GitHub issues for each of the three unresolved refactoring suggestions. @leynos I've created three GitHub issues to track these refactoring improvements:
Each issue includes the technical details, code references, and backlinks to this PR for context. 🏁 Script executed: Length of output: 2641 @leynos Successfully created three GitHub issues to track the refactoring improvements:
Each issue includes detailed technical specifications, code references, and backlinks to this PR for future implementation. |
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| mod.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
1 participant
Summary
Testing
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_6891ecebc3648322a0897a620fbcc94a
Summary by Sourcery
Ensure missing manifests are automatically generated in process_steps tests and provide a helper for minimal manifest creation
Tests: