refactor: make cargo wdk integration tests faster #553
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the cargo-wdk integration test suite to significantly improve performance by replacing coarse-grained file-based locking with fine-grained Windows named mutexes. The test execution time is reduced from ~24 minutes to ~10 minutes by:
- Moving certificate store synchronization into cargo-wdk itself using a dedicated named mutex
- Using per-project mutexes instead of a single global lock for build operations
- Replacing file locks with Windows API named mutexes to avoid file system artifacts
Reviewed Changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/cargo-wdk/tests/test_utils/mod.rs | Implements NamedMutex wrapper and replaces with_file_lock with with_mutex helper function |
| crates/cargo-wdk/tests/new_command_test.rs | Removes unnecessary lock acquisition from test command invocation |
| crates/cargo-wdk/tests/build_command_test.rs | Replaces global locks with per-project mutex locks in build tests |
| crates/cargo-wdk/src/actions/build/tests.rs | Removes .once() constraint to allow multiple mock invocations during parallel testing |
| crates/cargo-wdk/src/actions/build/package_task.rs | Adds certificate store mutex to prevent race conditions when generating certificates |
| crates/cargo-wdk/Cargo.toml | Adds windows crate dependency with required Win32 API features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
gurry
force-pushed
the
making-cargo-wdk-tests-faster
branch
from
ed8a5a8 to
2ef4c2d
Compare
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 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.
Pull Request Overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 5 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.
Pull Request Overview
Copilot reviewed 7 out of 9 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.
| } | ||
|
|
||
| /// An RAII wrapper over a Win API named mutex | ||
| pub struct NamedMutex { |
There was a problem hiding this comment.
Unfortunately NamedMutex is duplicated because we don't have a common crate to put it in.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 7 out of 9 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.
| // create a cert in the store. It is not a correctness problem. We | ||
| // just don't want to litter the store with certs especially during | ||
| // tests when there are lots of parallel runs | ||
| let mutex_name = CString::new("WDRCertStoreMutex_bd345cf9330") // Unique enough |
There was a problem hiding this comment.
Can we move this name to a constant and use it here?
There was a problem hiding this comment.
It's used at only one place so didn't feel the need to do that
There was a problem hiding this comment.
If not for reusability, can we consider doing it for consistency, so that all constants are defined in one place?
| _not_send: PhantomData<*const ()>, | ||
| } | ||
|
|
||
| impl NamedMutex { |
There was a problem hiding this comment.
It would be good to add unit tests for this custom Mutex implementation.
2 participants
This PR improves the performance of cargo-wdk integration tests by making the locks they take more fine grained. On my laptop the test execution time has reduced from ~13 minutes to ~5 minutes.
We need locks here for two reasons:
cargo buildcommands on the same driver projectEarlier we used a single lock to fulfil these requirements. That serialized all the tests with respect to each other and hurt performance. In this PR we still use locks but we do it differently.
To address requirement 1. we have made cargo-wdk itself take a lock on the cert store instead of relying on the test for synchronization. This lock is super fine-grained compared to before. For 2. we take a separate lock per driver project instead of having every test block on the same lock. Together these changes allow parallelism that improves performance.
Another thing: we now use a named mutex as the locking primitive instead of a file lock because mutexes are cleaner as they create no files. This also fixes #489.
While the performance wins here are significant, more can be done in this area. For example we could copy test projects out into their own isolated directories and run tests on those copies. That will eliminate requirement 2 entirely and remove all locks from the cargo-wdk test code giving us even bigger wins. Something to look into in the future.