Refactor cargo wdk action unit tests#690
Conversation
|
@copilot resolve the merge conflicts in this pull request |
There was a problem hiding this comment.
Pull request overview
This PR refactors cargo-wdk’s build/package orchestration to improve unit-testability by introducing injectable runner types for the build and package tasks, and rewrites the BuildAction unit tests to use those runners and more targeted expectations.
Changes:
- Add
BuildTaskRunner/BuildTaskRunParamsandPackageTaskRunnerwrappers to enable dependency injection and mocking. - Update
BuildActionto accept runner instances (vianew_with_runners) and route build/package work through the runners. - Replace large, end-to-end style
BuildActiontests with a smaller orchestrator-focused suite + richerPackageTaskunit tests.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/cargo-wdk/src/actions/build/build_task.rs | Introduces BuildTaskRunner and parameter struct to decouple BuildTask execution for mocking. |
| crates/cargo-wdk/src/actions/build/mod.rs | Refactors BuildAction to use injected runners for build/package phases. |
| crates/cargo-wdk/src/actions/build/package_task.rs | Adds PackageTaskRunner and significantly expands/reshapes PackageTask unit test coverage. |
| crates/cargo-wdk/src/actions/build/tests.rs | Refactors BuildAction tests to validate orchestration via mocked runners and targeted expectations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2e2b890 to
7bc3ee6
Compare
7bc3ee6 to
402e170
Compare
402e170 to
e5eea7d
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
==========================================
+ Coverage 79.93% 81.84% +1.90%
==========================================
Files 26 26
Lines 5633 6290 +657
Branches 5633 6290 +657
==========================================
+ Hits 4503 5148 +645
Misses 1002 1002
- Partials 128 140 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
e5eea7d to
b285c05
Compare
7f6d686 to
9954a35
Compare
Introduce BuildTaskRunner and PackageTaskRunner with #[automock] to decouple BuildAction orchestration tests from the full provider stack. BuildAction tests now mock runners instead of every individual provider, reducing tests.rs from ~3400 lines to ~700 lines while maintaining equivalent coverage through dedicated unit tests in build_task.rs and package_task.rs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa978dd1-9d61-4cea-a03a-7ebc914228fa
…due to expectation overrides
Group top-level orchestration tests into three submodules by scenario type: - standalone_driver_project: single-package driver builds - driver_workspace: multi-package Cargo workspace builds - emulated_workspace: filesystem-based emulated workspace builds No test logic changes; only structural reorganization for clarity. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa978dd1-9d61-4cea-a03a-7ebc914228fa
- Rename TestBuildAction to BuildActionHarness for consistency with PackageTaskHarness naming convention in the task layer. - Shorten test function names since submodule names already provide the 'given' context (e.g. standalone_driver_project::run_skips_probe...). - Rename test_build_action parameter to harness throughout. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa978dd1-9d61-4cea-a03a-7ebc914228fa
BuildTaskRunner::run previously collected the entire cargo message stream into a Vec, which forced every message to be parsed and allocated up front and prevented get_target_dir_from_output from short-circuiting once the matching cdylib artifact is found. Return Box<dyn Iterator<..>> instead so the runner forwards BuildTask::run's lazy stream while still being mockable (mockall cannot mock an opaque impl Trait return, but can mock a boxed trait object). BuildTask::run gains + use<> precise capturing so its owned, 'static iterator can move out of the temporary BuildTask and be boxed. Addresses PR review comment r3581534373. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: fa978dd1-9d61-4cea-a03a-7ebc914228fa
9954a35 to
c22da52
Compare
…in build action tests
… off. This replaces the earlier removed test "given_a_driver_project_when_sign_mode_is_off_then_signing_and_verification_steps_are_skipped"
…ce metadata handling
…d enhancing readability
…esses in PackageTaskRunner
This pull request refactors the build and package execution flow to improve testability and code structure. The main change is introducing
BuildTaskRunnerandPackageTaskRunneras injectable dependencies, allowing for easier mocking in tests. TheBuildActionstruct now accepts these runners, and their usage replaces direct instantiation ofBuildTaskandPackageTask.Here are the overall test coverage stats
actions/build/tests.rsactions/build/package_task.rsactions/build/build_task.rsstandalone_driver_project,driver_workspaceandemulated_workspace.package_taskmodule which were originally being covered in build action unit tests. A module level mutexTEST_MUTEXwas added to serialize execution of certain tests as one of the teststampinf_version_overrides_with_env_varmutates process-globalSTAMPINF_VERSIONenv varbuild_taskmodule.