chore(cli): separate unit tests from integration tests and fix test cleanup (#8562)

* fix(cli): separate unit tests from integration tests and fix test cleanup

- Rename 14 non-backend test files to *_unit.test.ts convention
- Add UNIT_ONLY env var guard in setup.ts to skip cargo build/backend startup
- Add test:unit and test:integration scripts to package.json
- Use setsid on Linux for process group management so stop() kills both
  cargo and the windmill child process
- Fix exit handler to kill process group instead of just the direct child
- Add cleanupStaleTestResources() to drop orphaned windmill_test_* databases
  and kill orphaned backend processes on startup
- Rewrite TESTING.md with current bun-based instructions

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(cli): fix process group approach - kill by db name instead of setsid

The setsid approach didn't work because setsid forks, making the PID
we get from Bun.spawn ephemeral. Instead, kill orphaned windmill child
processes by matching our unique database name in /proc/pid/environ.

Also add afterAll hook in setup.ts so full async cleanup (process kill
+ database drop) runs when all tests complete normally, not just on
SIGINT/SIGTERM.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(cli): address PR review feedback

- Remove duplicate cleanupStaleTestResources() call in getTestBackend()
  (already called in setup.ts)
- Add regex guard on database names before SQL interpolation
- Extract shared killWindmillProcessesByEnvMatch() helper to deduplicate
  process-killing logic
- Remove redundant test:integration script (test already runs everything)
- Flip setup.ts to if/else pattern for readability

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

cli/TESTING.md

@@ -3,57 +3,57 @@
 ## Running Tests
 
 ```bash
-# Run all tests
-deno test -A --no-check test/
+# Run unit tests only (fast — no backend, no database, no cargo build)
+bun run test:unit
+
+# Run all tests (unit + integration — requires PostgreSQL + cargo)
+DATABASE_URL=postgres://postgres:changeme@localhost:5432 bun run test
 
 # Run specific test files
-deno test -A --no-check test/gitsync_settings_features.test.ts
-deno test -A --no-check test/init_no_git_sync.test.ts
-deno test -A --no-check test/multi_instance_workspace.test.ts
-deno test -A --no-check test/override_settings_behavior.test.ts
-deno test -A --no-check test/sync_config_resolution.test.ts
-deno test -A --no-check test/workspace_conflicts.test.ts
-
-# Run with specific test patterns
-deno test -A --no-check test/ --filter "workspace"
-deno test -A --no-check test/ --filter "sync"
+bun test test/sync_pull_push.test.ts
+bun test test/workspace_conflicts_unit.test.ts
 ```
 
-## Test Files
+## Test Categories
 
-- **`gitsync_settings_features.test.ts`** - Git sync settings functionality
-- **`init_no_git_sync.test.ts`** - Init without git sync
-- **`multi_instance_workspace.test.ts`** - Multi-instance workspace handling
-- **`override_settings_behavior.test.ts`** - Settings override behavior
-- **`sync_config_resolution.test.ts`** - Sync configuration resolution
-- **`workspace_conflicts.test.ts`** - Workspace conflict detection
+### Unit tests (`*_unit.test.ts`)
 
-## Docker Requirements
+Pure local tests — no backend, no database. Uses `bunfig.unit.toml` (no preload).
+
+Examples: `git_unit`, `lint_command_unit`, `tar_creation_unit`, `workspace_conflicts_unit`
+
+### Integration tests
+
+Require a running backend and PostgreSQL. The `setup.ts` preload builds the backend
+binary and starts a shared backend instance.
+
+Examples: `sync_pull_push`, `dev_server`, `standalone_commands`
+
+## Environment Variables
+
+| Variable | Purpose | Default |
+|----------|---------|---------|
+| `DATABASE_URL` | PostgreSQL connection string (without database name) | `postgres://postgres:changeme@localhost:5432` |
+| `TEST_BACKEND` | `cargo` or `docker` | `cargo` |
+| `CI_MINIMAL_FEATURES` | `true` for CI mode (zip-only features) | unset |
+| `EE_LICENSE_KEY` | Enterprise license for EE feature tests | unset |
+| `TEST_FEATURES` | Additional cargo features (comma-separated) | unset |
+| `TEST_CLI_RUNTIME` | `node` to test npm package | unset |
+| `UNIT_ONLY` | `1` to skip backend setup in preload (used by `test:unit`) | unset |
+| `VERBOSE` | `1` for backend process output | unset |
+
+## Cleanup
+
+Stale test databases (`windmill_test_*`) and orphaned backend processes from
+previous crashed runs are automatically cleaned up when starting a new test run.
+
+To manually check for leftovers:
 
 ```bash
-# Ensure Docker is running
-docker --version
-docker-compose --version
+# Check for stale test databases
+psql postgres://postgres:changeme@localhost:5432/postgres -c \
+  "SELECT datname FROM pg_database WHERE datname LIKE 'windmill_test_%';"
 
-# Ensure EE license key is available
-echo $EE_LICENSE_KEY
+# Check for orphaned backend processes
+ps aux | grep "target/debug/windmill" | grep -v grep
 ```
-
-## Debugging Failed Tests
-
-```bash
-# Run with verbose output
-deno test -A --no-check test/ --reporter=verbose
-
-# Check container status
-docker ps
-
-# View backend logs
-docker logs test-test_windmill_server-1
-
-# Manual container management
-cd test
-docker compose -f docker-compose.test.yml up -d
-docker compose -f docker-compose.test.yml down
-docker compose -f docker-compose.test.yml down -v
-```

cli/package.json

@@ -9,6 +9,7 @@
     "dev": "bun run src/main.ts",
     "build": "./build.sh",
     "test": "bun test test/",
+    "test:unit": "UNIT_ONLY=1 bun test test/*_unit*",
     "check": "bunx tsc --noEmit",
     "gen-client": "./gen_wm_client.sh && ./windmill-utils-internal/gen_wm_client.sh"
   },

cli/test/cargo_backend.ts

@@ -14,11 +14,13 @@
 import { resolve, dirname, join } from "node:path";
 import { fileURLToPath } from "node:url";
 import { statSync } from "node:fs";
-import { mkdtemp, rm } from "node:fs/promises";
+import { mkdtemp, rm, readFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { createServer } from "node:net";
 import { Subprocess } from "bun";
 
+const IS_LINUX = process.platform === "linux";
+
 export interface CargoBackendConfig {
   /** PostgreSQL connection string (without database name) */
   postgresUrl?: string;
@@ -193,6 +195,10 @@ export class CargoBackend {
       this.process = null;
     }
 
+    // Kill any child processes (e.g. the windmill binary spawned by cargo)
+    // by matching our unique database name in their environment
+    await this.killProcessesByDbName();
+
     // Drop the test database
     await this.dropDatabase();
 
@@ -304,6 +310,15 @@ export class CargoBackend {
     }
   }
 
+  /**
+   * Kill any processes whose environment contains our unique database name.
+   * This catches child processes (e.g. the windmill binary spawned by cargo run)
+   * that survive after the direct child is killed.
+   */
+  private async killProcessesByDbName(): Promise<void> {
+    await killWindmillProcessesByEnvMatch(this.dbName);
+  }
+
   /**
    * Start the backend process using cargo run
    */
@@ -762,6 +777,90 @@ export class CargoBackend {
   }
 }
 
+/**
+ * Kill windmill processes whose /proc/pid/environ contains the given pattern.
+ * Used by both per-test cleanup (match specific DB name) and stale cleanup (match any test DB).
+ */
+async function killWindmillProcessesByEnvMatch(pattern: string): Promise<void> {
+  if (!IS_LINUX) return;
+  try {
+    const pgrepProc = Bun.spawn(["pgrep", "-f", "target/(debug|release)/windmill"], {
+      stdout: "pipe", stderr: "pipe",
+    });
+    const output = await new Response(pgrepProc.stdout).text();
+    await new Response(pgrepProc.stderr).text();
+    await pgrepProc.exited;
+
+    for (const pidStr of output.trim().split("\n").filter(Boolean)) {
+      const pid = Number(pidStr);
+      if (isNaN(pid)) continue;
+      try {
+        const environ = await readFile(`/proc/${pid}/environ`, "utf-8");
+        if (environ.includes(pattern)) {
+          console.log(`Killing orphaned test backend process: ${pid}`);
+          process.kill(pid, "SIGKILL");
+        }
+      } catch {
+        // Process exited or we lack permissions
+      }
+    }
+  } catch {
+    // pgrep not available or no matches
+  }
+}
+
+/**
+ * Clean up stale test databases and orphaned backend processes from previous
+ * test runs that crashed or were killed without proper cleanup.
+ *
+ * Should be called before starting a new test backend.
+ */
+export async function cleanupStaleTestResources(postgresUrl?: string): Promise<void> {
+  const baseUrl = postgresUrl || process.env["DATABASE_URL"] || "postgres://postgres:changeme@localhost:5432";
+  const url = new URL(baseUrl);
+  url.pathname = "";
+  url.search = "";
+  const cleanBaseUrl = url.toString().replace(/\/$/, "");
+
+  // 1. Find and drop stale windmill_test_* databases
+  try {
+    const listProc = Bun.spawn(["psql", `${cleanBaseUrl}/postgres`, "-t", "-c",
+      `SELECT datname FROM pg_database WHERE datname LIKE 'windmill_test_%';`
+    ], { stdout: "pipe", stderr: "pipe" });
+    const output = await new Response(listProc.stdout).text();
+    await new Response(listProc.stderr).text();
+    await listProc.exited;
+
+    const staleDBs = output.trim().split("\n").map(s => s.trim()).filter(Boolean);
+    for (const db of staleDBs) {
+      // Only touch databases matching the expected naming pattern
+      if (!/^windmill_test_[a-z0-9_]+$/.test(db)) continue;
+      console.log(`Cleaning up stale test database: ${db}`);
+      const termProc = Bun.spawn(["psql", `${cleanBaseUrl}/postgres`, "-c",
+        `SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE datname = '${db}' AND pid <> pg_backend_pid();`
+      ], { stdout: "pipe", stderr: "pipe" });
+      await new Response(termProc.stdout).text();
+      await new Response(termProc.stderr).text();
+      await termProc.exited;
+
+      const dropProc = Bun.spawn(["psql", `${cleanBaseUrl}/postgres`, "-c",
+        `DROP DATABASE IF EXISTS "${db}";`
+      ], { stdout: "pipe", stderr: "pipe" });
+      await new Response(dropProc.stdout).text();
+      await new Response(dropProc.stderr).text();
+      await dropProc.exited;
+    }
+    if (staleDBs.length > 0) {
+      console.log(`Cleaned up ${staleDBs.length} stale test database(s)`);
+    }
+  } catch (err) {
+    console.warn(`Warning: Failed to clean up stale databases: ${err}`);
+  }
+
+  // 2. Find and kill orphaned windmill processes from test runs
+  await killWindmillProcessesByEnvMatch("windmill_test_");
+}
+
 // Global backend instance
 let globalCargoBackend: CargoBackend | null = null;
 

cli/test/setup.ts

@@ -1,14 +1,22 @@
 /**
  * Global test setup — preloaded before all test files.
  *
+ * When UNIT_ONLY=1, skips all backend setup (cargo build, database, etc.)
+ * so that unit tests can run instantly without any external dependencies.
+ *
+ * Otherwise:
  * 1. Builds the backend binary so `cargo run` starts instantly.
  * 2. Starts a shared backend instance so integration tests don't
  *    bear the startup cost inside their per-test timeout window.
  */
 
-import { resolve } from "node:path";
-import { fileURLToPath } from "node:url";
-import { statSync } from "node:fs";
+if (process.env["UNIT_ONLY"]) {
+  // Nothing to do — unit tests don't need backend setup
+} else {
+
+const { resolve } = await import("node:path");
+const { fileURLToPath } = await import("node:url");
+const { statSync } = await import("node:fs");
 
 const __dirname = resolve(fileURLToPath(import.meta.url), "..");
 
@@ -69,10 +77,22 @@ console.log("Backend build complete.");
 // This avoids the first integration test timing out while the backend
 // creates its database, starts the process, and waits for the health check.
 if (process.env["DATABASE_URL"]) {
-  const { getTestBackend } = await import("./test_backend.ts");
+  // Clean up any stale databases/processes from previous crashed test runs
+  const { cleanupStaleTestResources } = await import("./cargo_backend.ts");
+  await cleanupStaleTestResources();
+
+  const { getTestBackend, cleanupTestBackend } = await import("./test_backend.ts");
   console.log("Pre-starting test backend...");
   await getTestBackend();
   console.log("Test backend is ready for all tests.");
+
+  // Register afterAll to do full async cleanup (kill processes + drop DB)
+  // when all tests complete. The synchronous "exit" handler alone can't
+  // drop databases or scan /proc for orphaned child processes.
+  const { afterAll } = await import("bun:test");
+  afterAll(async () => {
+    await cleanupTestBackend();
+  });
 }
 
 // When TEST_CLI_RUNTIME=node, also build the npm package so tests
@@ -92,3 +112,5 @@ if (process.env["TEST_CLI_RUNTIME"] === "node") {
   }
   console.log("npm package built — tests will use Node runtime.");
 }
+
+}

cli/test/test_backend.ts

@@ -590,11 +590,16 @@ function registerCleanup() {
   cleanupRegistered = true;
   process.on("exit", () => {
     if (globalBackend) {
-      // Synchronous kill — can't await in exit handler
-      try {
-        (globalBackend as any).backend?.process?.kill();
-      } catch {
-        // Best effort
+      // Synchronous kill — can't await in exit handler.
+      // Kill the direct child (cargo); any orphaned windmill child processes
+      // will be cleaned up by cleanupStaleTestResources() on next startup.
+      const pid = (globalBackend as any).backend?.process?.pid;
+      if (pid) {
+        try {
+          process.kill(pid, "SIGKILL");
+        } catch {
+          // Best effort — process may already be dead
+        }
       }
     }
   });