-
Notifications
You must be signed in to change notification settings - Fork 101
test: add unit tests for cargo wdk new subcommand action flow #551
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?
test: add unit tests for cargo wdk new subcommand action flow #551
Conversation
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
Adds unit tests for get_cargo_verbose_flags and extensive tests for the NewAction flow, covering success and multiple failure scenarios.
- Introduces verbosity mapping test in trace.rs.
- Adds comprehensive mocked tests for new project creation and error paths in actions/new/mod.rs.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/cargo-wdk/src/trace.rs | Adds a table-driven test validating Verbosity to cargo flag mapping. |
| crates/cargo-wdk/src/actions/new/mod.rs | Adds a large test module covering success and distinct failure scenarios for NewAction with mock exec/fs. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Apply copilot suggestion Co-authored-by: Copilot <[email protected]> Signed-off-by: Krishna Kumar Thokala <[email protected]>
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
set ExitStatus explicitly using from_raw instead of default Co-authored-by: Copilot <[email protected]> Signed-off-by: Krishna Kumar Thokala <[email protected]>
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| #[test] | ||
| fn map_input_clap_verbosity_flags_to_cargo_flags() { | ||
| // (incoming verbosity, expected cargo flag Option) | ||
| let cases = vec![ |
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.
What happens if someone instantiated Verbosity with illegal values e.g. 15 for the first argument or 3 for the second argument?
Do we want to add tests for some of these values?
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 first argument represents the count of verbose flags, and the second indicates the count of quiet flags. An internal offset is calculated as first - second. When this value exceeds 3, it sets the trace level. In our case, we have one such scenario that covers that i.e (4, 0).
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.
Okay, can you add another one which does not have a 0 in it because 0 is an edge condition as well?
| let driver_type = DriverType::Kmdf; | ||
| let verbosity_level = Verbosity::default(); | ||
|
|
||
| let cases: [UpdateCargoTomlCaseSetupHelper; 3] = [ |
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.
In addition to Gurinder's comment above, storing each of these as booleans would make more sense if we wanted to test every single combination of the failures and their potential outputs. either we should choose a better way to represent this or test all 8 possible true/false combos
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.
I'm reverting to using unnamed tuples as discussed here #551 (comment)
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…nd maintainability
…roved readability
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
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
… handling in expect_create_inx_file
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
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Test improvements:
crates/cargo-wdk/src/trace.rsthat verifies the correct mapping ofVerbositylevels to Cargo flag options, covering default, quiet, and multiple verbose levels.