-
Notifications
You must be signed in to change notification settings - Fork 110
fix: replace direct call to std::fs::create_dir_all with fs provider fs::create_dir_all
#602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…e_dir_all` method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a testing issue where direct calls to std::fs::create_dir_all bypassed the Fs provider mock during test execution, causing unwanted temporary directory creation. The solution introduces a new create_dir_all method to the Fs provider and replaces all direct calls with this mockable version.
- Added
create_dir_allmethod to theFsprovider with proper error handling - Replaced direct
std::fs::create_dir_allcall inNewAction::copy_cargo_configwith the new provider method - Updated test mocks to properly expect
create_dir_allcalls
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/cargo-wdk/src/providers/fs.rs | Added create_dir_all method to the Fs provider and updated imports to include create_dir_all from std::fs |
| crates/cargo-wdk/src/actions/new/mod.rs | Removed direct std::fs::create_dir_all import and replaced usage with self.fs.create_dir_all, updated test mock expectations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn create_dir_all(&self, path: &Path) -> Result<(), FileError> { | ||
| create_dir_all(path).map_err(|e| FileError::CreateDirError(path.to_owned(), e)) | ||
| } |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error variant CreateDirError is used for both create_dir and create_dir_all operations. While this works, it would be clearer to introduce a separate error variant like CreateDirAllError to distinguish between single-level directory creation failures and recursive directory creation failures. This would make error messages more precise and help with debugging.
| let expected_cargo_dir = cargo_dir; | ||
| self.mock_fs | ||
| .expect_create_dir_all() | ||
| .withf(move |path| path == expected_cargo_dir) |
Copilot
AI
Dec 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable expected_cargo_dir is redundantly assigned from cargo_dir. This intermediate assignment doesn't add value and can be simplified by using cargo_dir.clone() directly in the closure or cloning only when needed.
| let expected_cargo_dir = cargo_dir; | |
| self.mock_fs | |
| .expect_create_dir_all() | |
| .withf(move |path| path == expected_cargo_dir) | |
| self.mock_fs | |
| .expect_create_dir_all() | |
| .withf(move |path| path == cargo_dir) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #602 +/- ##
==========================================
+ Coverage 82.66% 82.67% +0.01%
==========================================
Files 25 25
Lines 7137 7147 +10
Branches 7137 7147 +10
==========================================
+ Hits 5900 5909 +9
Misses 1108 1108
- Partials 129 130 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Calling
std::fs::create_dir_allfrom theNewAction::copy_cargo_configmethod led to bypassing theFsprovider mock during test execution. This led to creation of temporary directories:In this PR, a
create_dir_allmethod is added toFsprovider and all direct calls tostd::fs::create_dir_allhave been replaced by this method. This allows the mocks to work as expected during test execution.