-
Notifications
You must be signed in to change notification settings - Fork 2.1k
ci: Fix leaky tests #11081
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
ci: Fix leaky tests #11081
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Additional Suggestion:
The handles_kill_dead_server test spawns a process without .kill_on_drop(true), while the similar handles_kill_dead_server_wrong_process test includes this setting to fix the process leak. Both tests should be fixed consistently.
View Details
📝 Patch Details
diff --git a/crates/turborepo-lib/src/daemon/connector.rs b/crates/turborepo-lib/src/daemon/connector.rs
index a418cd41d..88a198432 100644
--- a/crates/turborepo-lib/src/daemon/connector.rs
+++ b/crates/turborepo-lib/src/daemon/connector.rs
@@ -562,6 +562,7 @@ mod test {
.stderr(Stdio::null())
.arg("-e")
.arg("setInterval(() => {}, 1000)")
+ .kill_on_drop(true)
.spawn()
.unwrap();
Analysis
Process leak in handles_kill_dead_server test due to missing kill_on_drop(true)
What fails: The handles_kill_dead_server test spawns a Node.js child process without setting .kill_on_drop(true), causing the process to remain alive after the test completes while handles_kill_dead_server_wrong_process properly cleans up with .kill_on_drop(true).
How to reproduce:
- Run the test:
cargo test --lib daemon::connector::handles_kill_dead_server - The spawned Node.js process with
setInterval(() => {}, 1000)will not be terminated when the test ends sincetokio::process::Childdefaults tokill_on_drop=false - Without explicit cleanup, the process persists after test completion
What happens vs expected: Without .kill_on_drop(true), when proc goes out of scope at the end of the test, the underlying Node.js process continues running. The similar test handles_kill_dead_server_wrong_process (line 522) includes .kill_on_drop(true) for proper cleanup. Per Tokio documentation, the default value is false, meaning "spawned processes will not be killed on drop, similar to the behavior of the standard library."
Both tests spawn long-running Node processes for testing daemon process management. Both should ensure these test processes are properly cleaned up when the test scope exits. The kill_on_drop(true) setting ensures SIGKILL is sent when the Child handle is dropped, guaranteeing process termination regardless of whether the process received SIGTERM earlier.
Fix: Added .kill_on_drop(true) to the handles_kill_dead_server test to match the pattern in handles_kill_dead_server_wrong_process and ensure proper process cleanup.
…ill_on_drop(true)`\, while the similar `handles_kill_dead_server_wrong_process` test includes this setting to fix the process leak\. Both tests should be fixed consistently\.
Description
nextest was showing how these tests could leak, since
procwouldn't get killed on drop. Now they will.Testing Instructions
CI
CLOSES TURBO-4958