feat(text-injection): add unit tests for enigo and kdotool injectors #312
+355
−61
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
|||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||
ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||||
![kiloconnect[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Comment on lines
+153
to
+163
| struct PathGuard { | ||
| original_path: String, | ||
| } | ||
|
|
||
| impl PathGuard { | ||
| fn new(temp_dir: &PathBuf) -> Self { | ||
| let original_path = env::var("PATH").unwrap_or_default(); | ||
| let new_path = format!("{}:{}", temp_dir.to_str().unwrap(), original_path); | ||
| env::set_var("PATH", new_path); | ||
| Self { original_path } | ||
| } |
There was a problem hiding this comment.
🔴 CRITICAL: Concurrent PATH manipulation
The PathGuard struct modifies the global PATH environment variable during tests, which can cause race conditions and unpredictable behavior when tests run concurrently, potentially affecting other tests or system operations.
Consider using a more isolated approach for testing, such as:
- Using a separate test harness that doesn't modify global state
- Using a mock filesystem or containerization for tests
- Running these tests sequentially with proper synchronization
This issue was also flagged by the Qodo compliance check.
Comment on lines
+35
to
+42
| // Type each character with a small delay | ||
| for c in text.chars() { | ||
| match c { | ||
| ' ' => enigo | ||
| .key(Key::Space, Direction::Click) | ||
| .map_err(|e| InjectionError::MethodFailed(format!("Failed to type space: {}", e)))?, | ||
| '\n' => enigo | ||
| .key(Key::Return, Direction::Click) |
There was a problem hiding this comment.
New error mapping includes underlying backend error details in InjectionError::MethodFailed(format!(...{}...)), which may be user-visible depending on how InjectionError is surfaced.
Consider whether exposing detailed system error messages to end users is appropriate from a security standpoint. You might want to log the detailed error internally and return a more generic message to users.
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive unit tests for the EnigoInjector and KdotoolInjector text injection backends, closing the work started in draft PR #268.
Key Changes:
- Refactored
EnigoInjectorto extract testable logic into separate functions (type_text_logicandtrigger_paste_logic) - Added mock-based unit tests for
EnigoInjectorwith a customMockEnigotest harness - Added integration-style tests for
KdotoolInjectorusing mock shell scripts and PATH manipulation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
crates/coldvox-text-injection/src/enigo_injector.rs |
Refactored typing and paste logic into testable functions; added comprehensive unit tests with platform-specific testing for macOS and non-macOS paste shortcuts |
crates/coldvox-text-injection/src/kdotool_injector.rs |
Added integration tests using mock kdotool scripts with PATH manipulation to test window management functionality |
Comment on lines
+312
to
+313
| let injector = EnigoInjector::new(config); | ||
| assert_eq!(injector.config, config); |
There was a problem hiding this comment.
The test compares injector.config with config, but InjectionConfig does not derive PartialEq. This comparison will fail to compile. Consider removing this assertion or deriving PartialEq for InjectionConfig.
Suggested change
| let injector = EnigoInjector::new(config); | |
| assert_eq!(injector.config, config); | |
| let expected_allow_enigo = config.allow_enigo; | |
| let injector = EnigoInjector::new(config); | |
| assert_eq!(injector.config.allow_enigo, expected_allow_enigo); |
Comment on lines
+184
to
+191
| let original_path = env::var("PATH").unwrap_or_default(); | ||
| env::set_var("PATH", "/tmp/non-existent-dir"); | ||
|
|
||
| let config = InjectionConfig::default(); | ||
| let injector = KdotoolInjector::new(config); | ||
| assert!(!injector.is_available); | ||
|
|
||
| env::set_var("PATH", original_path); |
There was a problem hiding this comment.
The test manually restores PATH in the cleanup code. However, tests can run concurrently in Rust by default, which means modifying the global PATH environment variable can cause race conditions between tests. Consider using a mutex or running these tests serially with #[serial] (from the serial_test crate), or structuring tests to avoid PATH modification entirely.
Comment on lines
+152
to
+170
| // RAII guard to modify PATH for the duration of a test | ||
| struct PathGuard { | ||
| original_path: String, | ||
| } | ||
|
|
||
| impl PathGuard { | ||
| fn new(temp_dir: &PathBuf) -> Self { | ||
| let original_path = env::var("PATH").unwrap_or_default(); | ||
| let new_path = format!("{}:{}", temp_dir.to_str().unwrap(), original_path); | ||
| env::set_var("PATH", new_path); | ||
| Self { original_path } | ||
| } | ||
| } | ||
|
|
||
| impl Drop for PathGuard { | ||
| fn drop(&mut self) { | ||
| env::set_var("PATH", &self.original_path); | ||
| } | ||
| } |
There was a problem hiding this comment.
The PathGuard RAII pattern is used to manage PATH modifications, but tests can run concurrently which means multiple tests could be modifying PATH simultaneously. Consider using a mutex or running these tests serially with #[serial] (from the serial_test crate) to prevent race conditions from concurrent PATH modifications.
Comment on lines
+172
to
+180
| #[tokio::test] | ||
| async fn test_kdotool_injector_new_available() { | ||
| let (_script, dir) = create_mock_kdotool("exit 0"); | ||
| let _guard = PathGuard::new(&dir.path().to_path_buf()); | ||
|
|
||
| let config = InjectionConfig::default(); | ||
| let injector = KdotoolInjector::new(config); | ||
| assert!(injector.is_available); | ||
| } |
There was a problem hiding this comment.
The test creates shell scripts but only sets execute permissions on Unix systems. However, the tests themselves don't have platform-specific guards and will fail on non-Unix platforms (e.g., Windows) because shell scripts cannot be executed. Consider adding #[cfg(unix)] to these tests or using a cross-platform mock approach.
| impl PathGuard { | ||
| fn new(temp_dir: &PathBuf) -> Self { | ||
| let original_path = env::var("PATH").unwrap_or_default(); | ||
| let new_path = format!("{}:{}", temp_dir.to_str().unwrap(), original_path); |
There was a problem hiding this comment.
The PATH separator is hardcoded as ":" which is Unix-specific. On Windows, the PATH separator is ";". Since kdotool is likely Unix-only, consider adding #[cfg(unix)] to all these tests or using std::env::consts::PATH_SEPARATOR for cross-platform compatibility.
Suggested change
| let new_path = format!("{}:{}", temp_dir.to_str().unwrap(), original_path); | |
| let temp_str = temp_dir.to_str().unwrap(); | |
| let new_path = if original_path.is_empty() { | |
| temp_str.to_string() | |
| } else { | |
| format!( | |
| "{}{}{}", | |
| temp_str, | |
| std::env::consts::PATH_SEPARATOR, | |
| original_path | |
| ) | |
| }; |
CI Feedback 🧐(Feedback updated until commit a1353d3)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Adds comprehensive unit tests for the `enigo_injector` and `kdotool_injector` modules. For the `enigo_injector`, the code was refactored to extract the core keyboard manipulation logic into synchronous, generic functions. This allows for the use of a mock `Enigo` object, enabling isolated unit tests without side effects. For the `kdotool_injector`, the tests mock the `kdotool` command-line tool by creating a temporary script and manipulating the `PATH` environment variable. This ensures that the tests are reliable and do not depend on the actual `kdotool` binary being installed.
auto-merge was automatically disabled
December 25, 2025 00:00
Head branch was modified
Coldaine
force-pushed
the
integrate/268
branch
from
a1353d3 to
1812bf0
Compare
|
| Severity | Issue | Location |
|---|---|---|
| CRITICAL | Concurrent PATH manipulation | crates/coldvox-text-injection/src/kdotool_injector.rs:153-170 |
| WARNING | Modifier key not released on error | crates/coldvox-text-injection/src/enigo_injector.rs:82-105 |
| WARNING | Unix-only PATH separator | crates/coldvox-text-injection/src/kdotool_injector.rs:160-161 |
| WARNING | Manual PATH cleanup | crates/coldvox-text-injection/src/kdotool_injector.rs:182-192 |
| WARNING | Shell scripts Unix-only | crates/coldvox-text-injection/src/kdotool_injector.rs:180 |
Recommendation: Address critical issues before merge
Review Details (2 files)
Files: enigo_injector.rs (2 issues), kdotool_injector.rs (3 issues)
Summary: This PR adds comprehensive unit tests for text injectors. The tests are well-structured and use good mocking patterns. However, there are several issues with test isolation and cross-platform compatibility that need to be addressed.
Critical Issues:
- The kdotool tests modify the global PATH environment variable without proper synchronization, which will cause race conditions when tests run in parallel.
Warning Issues:
- Modifier keys (Ctrl/Cmd) may not be released if paste operations fail in the middle
- PATH separator is hardcoded as ":" which doesn't work on Windows
- One test manually restores PATH instead of using the PathGuard RAII helper
- Shell scripts are only made executable on Unix systems, causing Windows failures
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User description
Summary
Rebased and integrated from Jules draft PR #268.
Changes
EnigoInjectorwith test harnessKdotoolInjectorwith test harnessTesting
cargo test -p coldvox-text-injectionCloses #268
PR Type
Tests, Enhancement
Description
Refactored
EnigoInjectorto extract testable logic into generic functionstype_text_logic()andtrigger_paste_logic()methods acceptingKeyboardtraitAdded comprehensive unit tests for
EnigoInjectorwithMockEnigoimplementationAdded comprehensive unit tests for
KdotoolInjectorwith mock script approachImplemented
PathGuardRAII pattern for safe temporary PATH manipulation in testsDiagram Walkthrough
File Walkthrough
enigo_injector.rs
Refactor enigo logic and add comprehensive unit testscrates/coldvox-text-injection/src/enigo_injector.rs
type_text_logic()andtrigger_paste_logic()that accept any typeimplementing the
Keyboardtraittype_text()andtrigger_paste()async methods to call thenew generic logic functions
MockEnigostruct implementing theKeyboardtrait for isolated testingnewline, tab), error scenarios, and platform-specific paste operations
(macOS vs non-macOS)
kdotool_injector.rs
Add comprehensive unit tests for kdotool injectorcrates/coldvox-text-injection/src/kdotool_injector.rs
mock kdotool scripts
PathGuardRAII struct to safely manage temporary PATHenvironment variable modifications during tests
available/unavailable kdotool, window retrieval, window activation,
window focusing, and ensure_focus operations
kdotool behavior without requiring the actual binary