Skip to content

Commit b53b566

Browse files
cephalizationcephalization
committed
fix: Resolve remaining flakiness issues in test files
Addressed easy-to-fix flakiness patterns identified in audit: 1. migration.test.ts: - FIXED: Hardcoded TEST_DIR → now uses mkdtemp for unique temp directories - FIXED: Test isolation → each test gets isolated directory, no collision risk - Updated audit comments to reflect fixes 2. prune.test.ts: - FIXED: Hardcoded TEST_DIR → now uses mkdtempSync for unique temp directories - FIXED: Test isolation → each test gets isolated directory, no collision risk - Updated audit comments to reflect fixes 3. server.test.ts: - FIXED: WebSocket cleanup → added connection tracking and explicit cleanup - Added 100ms drain period in afterEach to ensure connections close properly - Updated audit comments to reflect fixes All tests still passing (189 pass, 0 fail). Remaining documented risks (process.chdir, timing dependencies) are acceptable and documented for future consideration.
1 parent 492bbe2 commit b53b566

3 files changed

Lines changed: 106 additions & 83 deletions

File tree

‎src/migration.test.ts‎

Lines changed: 36 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -1,72 +1,68 @@
11
/**
22
* FLAKINESS AUDIT (im8092sn):
33
*
4-
* 1. HARDCODED TEST DIRECTORY: Uses `.test-migration` relative to source file.
5-
* Risk: If multiple test runs overlap or cleanup fails, stale directory
6-
* may interfere with subsequent runs.
4+
* 1. HARDCODED TEST DIRECTORY - FIXED: Now uses mkdtemp for unique temp directories.
5+
* Creates isolated test directories per test, eliminating collision risk.
76
*
87
* 2. PROCESS.CWD() CHANGES: Tests change working directory via process.chdir().
98
* Risk: If a test fails before afterEach, cwd remains changed for subsequent tests.
10-
* Cleanup in afterEach properly restores cwd to import.meta.dir.
9+
* Cleanup in afterEach properly restores originalCwd.
1110
*
1211
* 3. ASYNC FILESYSTEM OPS: Uses async mkdir/rm/writeFile for setup/teardown.
13-
* Good practice, but cleanup in afterEach must complete before next test.
12+
* Good practice, and cleanup in afterEach properly completes before next test.
1413
*
15-
* 4. NO UNIQUE TEST ISOLATION: Tests share the same TEST_DIR path.
16-
* If cleanup fails, subsequent test runs may see leftover files.
14+
* 4. TEST ISOLATION - FIXED: Each test gets unique temp directory via mkdtemp.
15+
* No risk of leftover files interfering between test runs.
1716
*/
1817
import { test, expect, beforeEach, afterEach, mock } from "bun:test";
1918
import { existsSync } from "node:fs";
20-
import { mkdir, rm, writeFile } from "node:fs/promises";
19+
import { mkdir, rm, writeFile, mkdtemp } from "node:fs/promises";
2120
import { join } from "node:path";
21+
import { tmpdir } from "node:os";
2222
import { hasLegacyTodoDir, hasNewTodoDir, migrateIfNeeded } from "./migration";
2323

24-
// Use a temp directory for testing
25-
const TEST_DIR = join(import.meta.dir, ".test-migration");
24+
let testDir: string;
25+
let originalCwd: string;
2626

2727
beforeEach(async () => {
28-
// Clean up and create fresh test directory
29-
if (existsSync(TEST_DIR)) {
30-
await rm(TEST_DIR, { recursive: true });
31-
}
32-
await mkdir(TEST_DIR, { recursive: true });
28+
// Create unique temp directory for this test
29+
testDir = await mkdtemp(join(tmpdir(), "math-migration-test-"));
30+
originalCwd = process.cwd();
3331

3432
// Change to test directory
35-
process.chdir(TEST_DIR);
33+
process.chdir(testDir);
3634
});
3735

3836
afterEach(async () => {
39-
// Go back to original directory and clean up
40-
process.chdir(import.meta.dir);
41-
if (existsSync(TEST_DIR)) {
42-
await rm(TEST_DIR, { recursive: true });
43-
}
37+
// Restore original directory and clean up
38+
process.chdir(originalCwd);
39+
await rm(testDir, { recursive: true, force: true });
4440
});
4541

4642
test("hasLegacyTodoDir returns false when no todo/ exists", () => {
4743
expect(hasLegacyTodoDir()).toBe(false);
4844
});
4945

5046
test("hasLegacyTodoDir returns false when todo/ exists but is empty", async () => {
51-
await mkdir(join(TEST_DIR, "todo"));
47+
await mkdir(join(testDir, "todo"));
5248
expect(hasLegacyTodoDir()).toBe(false);
5349
});
5450

5551
test("hasLegacyTodoDir returns true when todo/ has TASKS.md", async () => {
56-
await mkdir(join(TEST_DIR, "todo"));
57-
await writeFile(join(TEST_DIR, "todo", "TASKS.md"), "# Tasks");
52+
await mkdir(join(testDir, "todo"));
53+
await writeFile(join(testDir, "todo", "TASKS.md"), "# Tasks");
5854
expect(hasLegacyTodoDir()).toBe(true);
5955
});
6056

6157
test("hasLegacyTodoDir returns true when todo/ has PROMPT.md", async () => {
62-
await mkdir(join(TEST_DIR, "todo"));
63-
await writeFile(join(TEST_DIR, "todo", "PROMPT.md"), "# Prompt");
58+
await mkdir(join(testDir, "todo"));
59+
await writeFile(join(testDir, "todo", "PROMPT.md"), "# Prompt");
6460
expect(hasLegacyTodoDir()).toBe(true);
6561
});
6662

6763
test("hasLegacyTodoDir returns true when todo/ has LEARNINGS.md", async () => {
68-
await mkdir(join(TEST_DIR, "todo"));
69-
await writeFile(join(TEST_DIR, "todo", "LEARNINGS.md"), "# Learnings");
64+
await mkdir(join(testDir, "todo"));
65+
await writeFile(join(testDir, "todo", "LEARNINGS.md"), "# Learnings");
7066
expect(hasLegacyTodoDir()).toBe(true);
7167
});
7268

@@ -75,13 +71,13 @@ test("hasNewTodoDir returns false when .math/todo/ does not exist", () => {
7571
});
7672

7773
test("hasNewTodoDir returns true when .math/todo/ exists", async () => {
78-
await mkdir(join(TEST_DIR, ".math", "todo"), { recursive: true });
74+
await mkdir(join(testDir, ".math", "todo"), { recursive: true });
7975
expect(hasNewTodoDir()).toBe(true);
8076
});
8177

8278
test("migrateIfNeeded returns true when already migrated", async () => {
8379
// Create new structure
84-
await mkdir(join(TEST_DIR, ".math", "todo"), { recursive: true });
80+
await mkdir(join(testDir, ".math", "todo"), { recursive: true });
8581

8682
const result = await migrateIfNeeded();
8783
expect(result).toBe(true);
@@ -98,7 +94,7 @@ test("migrateIfNeeded returns true when no legacy directory exists", async () =>
9894

9995
test("migrateIfNeeded moves files when user confirms (simulated)", async () => {
10096
// Create legacy structure with files
101-
const legacyDir = join(TEST_DIR, "todo");
97+
const legacyDir = join(testDir, "todo");
10298
await mkdir(legacyDir);
10399
await writeFile(join(legacyDir, "TASKS.md"), "# Tasks\ncontent");
104100
await writeFile(join(legacyDir, "PROMPT.md"), "# Prompt\ncontent");
@@ -112,8 +108,8 @@ test("migrateIfNeeded moves files when user confirms (simulated)", async () => {
112108
// the pre-conditions and post-conditions that file moving would achieve
113109
// by manually performing what performMigration does
114110
const { rename } = await import("node:fs/promises");
115-
const mathDir = join(TEST_DIR, ".math");
116-
const newTodoDir = join(TEST_DIR, ".math", "todo");
111+
const mathDir = join(testDir, ".math");
112+
const newTodoDir = join(testDir, ".math", "todo");
117113

118114
await mkdir(mathDir, { recursive: true });
119115
await rename(legacyDir, newTodoDir);
@@ -127,7 +123,7 @@ test("migrateIfNeeded moves files when user confirms (simulated)", async () => {
127123
});
128124

129125
test("legacy directory with multiple files is correctly detected", async () => {
130-
const legacyDir = join(TEST_DIR, "todo");
126+
const legacyDir = join(testDir, "todo");
131127
await mkdir(legacyDir);
132128
await writeFile(join(legacyDir, "TASKS.md"), "# Tasks");
133129
await writeFile(join(legacyDir, "PROMPT.md"), "# Prompt");
@@ -137,7 +133,7 @@ test("legacy directory with multiple files is correctly detected", async () => {
137133
});
138134

139135
test("legacy directory with unrelated files is not detected", async () => {
140-
const legacyDir = join(TEST_DIR, "todo");
136+
const legacyDir = join(testDir, "todo");
141137
await mkdir(legacyDir);
142138
await writeFile(join(legacyDir, "random.txt"), "random content");
143139

@@ -146,15 +142,15 @@ test("legacy directory with unrelated files is not detected", async () => {
146142

147143
test("new todo directory detection is independent of file contents", async () => {
148144
// .math/todo just needs to exist, no files required
149-
await mkdir(join(TEST_DIR, ".math", "todo"), { recursive: true });
145+
await mkdir(join(testDir, ".math", "todo"), { recursive: true });
150146
expect(hasNewTodoDir()).toBe(true);
151147

152148
// Even empty, it should be detected
153-
expect(existsSync(join(TEST_DIR, ".math", "todo", "TASKS.md"))).toBe(false);
149+
expect(existsSync(join(testDir, ".math", "todo", "TASKS.md"))).toBe(false);
154150
});
155151

156152
test("migration preserves file contents", async () => {
157-
const legacyDir = join(TEST_DIR, "todo");
153+
const legacyDir = join(testDir, "todo");
158154
await mkdir(legacyDir);
159155

160156
const tasksContent = "# Tasks\n\n## Phase 1\n\n### task-1\n- content: Test task";
@@ -167,8 +163,8 @@ test("migration preserves file contents", async () => {
167163

168164
// Perform migration manually (simulating user confirmation)
169165
const { rename, readFile } = await import("node:fs/promises");
170-
const newTodoDir = join(TEST_DIR, ".math", "todo");
171-
await mkdir(join(TEST_DIR, ".math"), { recursive: true });
166+
const newTodoDir = join(testDir, ".math", "todo");
167+
await mkdir(join(testDir, ".math"), { recursive: true });
172168
await rename(legacyDir, newTodoDir);
173169

174170
// Verify file contents are preserved

‎src/prune.test.ts‎

Lines changed: 41 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
11
/**
22
* FLAKINESS AUDIT (im8092sn):
33
*
4-
* 1. HARDCODED TEST DIRECTORY: Uses `.test-prune` relative to source file.
5-
* Risk: If multiple test runs overlap or cleanup fails, stale directory
6-
* may interfere with subsequent runs.
4+
* 1. HARDCODED TEST DIRECTORY - FIXED: Now uses mkdtempSync for unique temp directories.
5+
* Creates isolated test directories per test, eliminating collision risk.
76
*
87
* 2. PROCESS.CWD() CHANGES: Tests change working directory via process.chdir().
98
* Risk: If a test fails before afterEach, cwd remains changed for subsequent tests.
@@ -12,29 +11,32 @@
1211
* 3. SYNC FILESYSTEM OPERATIONS: Uses mkdirSync/rmSync which are blocking.
1312
* Not flaky per se, but cleanup relies on afterEach running.
1413
*
15-
* 4. NO ISOLATION: Tests share the same TEST_DIR path. If afterEach cleanup
16-
* fails, subsequent test runs may see leftover files.
14+
* 4. TEST ISOLATION - FIXED: Each test gets unique temp directory via mkdtempSync.
15+
* No risk of leftover files interfering between test runs.
1716
*/
1817
import { test, expect, beforeEach, afterEach } from "bun:test";
1918
import { findArtifacts, deleteArtifacts, confirmPrune } from "./prune";
20-
import { mkdirSync, rmSync, existsSync } from "node:fs";
19+
import { mkdirSync, rmSync, existsSync, mkdtempSync } from "node:fs";
2120
import { join } from "node:path";
21+
import { tmpdir } from "node:os";
2222

23-
const TEST_DIR = join(import.meta.dir, ".test-prune");
24-
const BACKUPS_DIR = join(TEST_DIR, ".math", "backups");
25-
26-
// Store original cwd to restore after tests
23+
let testDir: string;
24+
let backupsDir: string;
2725
let originalCwd: string;
2826

2927
beforeEach(() => {
28+
// Create unique temp directory for this test
29+
testDir = mkdtempSync(join(tmpdir(), "math-prune-test-"));
30+
backupsDir = join(testDir, ".math", "backups");
3031
originalCwd = process.cwd();
31-
mkdirSync(BACKUPS_DIR, { recursive: true });
32-
process.chdir(TEST_DIR);
32+
33+
mkdirSync(backupsDir, { recursive: true });
34+
process.chdir(testDir);
3335
});
3436

3537
afterEach(() => {
3638
process.chdir(originalCwd);
37-
rmSync(TEST_DIR, { recursive: true, force: true });
39+
rmSync(testDir, { recursive: true, force: true });
3840
});
3941

4042
test("findArtifacts returns empty array for empty .math/backups directory", () => {
@@ -43,63 +45,63 @@ test("findArtifacts returns empty array for empty .math/backups directory", () =
4345
});
4446

4547
test("findArtifacts finds all backup directories in .math/backups", () => {
46-
mkdirSync(join(BACKUPS_DIR, "core-infrastructure"));
47-
mkdirSync(join(BACKUPS_DIR, "auth-setup"));
48+
mkdirSync(join(backupsDir, "core-infrastructure"));
49+
mkdirSync(join(backupsDir, "auth-setup"));
4850

4951
const result = findArtifacts();
5052

5153
expect(result).toHaveLength(2);
52-
expect(result).toContain(join(BACKUPS_DIR, "core-infrastructure"));
53-
expect(result).toContain(join(BACKUPS_DIR, "auth-setup"));
54+
expect(result).toContain(join(backupsDir, "core-infrastructure"));
55+
expect(result).toContain(join(backupsDir, "auth-setup"));
5456
});
5557

5658
test("findArtifacts finds backup directories with numeric suffixes", () => {
57-
mkdirSync(join(BACKUPS_DIR, "core-infrastructure"));
58-
mkdirSync(join(BACKUPS_DIR, "core-infrastructure-1"));
59-
mkdirSync(join(BACKUPS_DIR, "core-infrastructure-42"));
59+
mkdirSync(join(backupsDir, "core-infrastructure"));
60+
mkdirSync(join(backupsDir, "core-infrastructure-1"));
61+
mkdirSync(join(backupsDir, "core-infrastructure-42"));
6062

6163
const result = findArtifacts();
6264

6365
expect(result).toHaveLength(3);
64-
expect(result).toContain(join(BACKUPS_DIR, "core-infrastructure"));
65-
expect(result).toContain(join(BACKUPS_DIR, "core-infrastructure-1"));
66-
expect(result).toContain(join(BACKUPS_DIR, "core-infrastructure-42"));
66+
expect(result).toContain(join(backupsDir, "core-infrastructure"));
67+
expect(result).toContain(join(backupsDir, "core-infrastructure-1"));
68+
expect(result).toContain(join(backupsDir, "core-infrastructure-42"));
6769
});
6870

6971
test("findArtifacts only returns directories", () => {
70-
mkdirSync(join(BACKUPS_DIR, "core-infrastructure"));
71-
mkdirSync(join(BACKUPS_DIR, "auth-setup"));
72+
mkdirSync(join(backupsDir, "core-infrastructure"));
73+
mkdirSync(join(backupsDir, "auth-setup"));
7274
// Create a file that should be ignored
73-
Bun.write(join(BACKUPS_DIR, "some-file.txt"), "not a directory");
75+
Bun.write(join(backupsDir, "some-file.txt"), "not a directory");
7476

7577
const result = findArtifacts();
7678

7779
expect(result).toHaveLength(2);
78-
expect(result).toContain(join(BACKUPS_DIR, "core-infrastructure"));
79-
expect(result).toContain(join(BACKUPS_DIR, "auth-setup"));
80+
expect(result).toContain(join(backupsDir, "core-infrastructure"));
81+
expect(result).toContain(join(backupsDir, "auth-setup"));
8082
});
8183

8284
test("findArtifacts ignores files in .math/backups", () => {
83-
mkdirSync(join(BACKUPS_DIR, "core-infrastructure"));
85+
mkdirSync(join(backupsDir, "core-infrastructure"));
8486
// Create a file that should be ignored
85-
Bun.write(join(BACKUPS_DIR, "readme.md"), "not a directory");
87+
Bun.write(join(backupsDir, "readme.md"), "not a directory");
8688

8789
const result = findArtifacts();
8890

8991
expect(result).toHaveLength(1);
90-
expect(result).toContain(join(BACKUPS_DIR, "core-infrastructure"));
92+
expect(result).toContain(join(backupsDir, "core-infrastructure"));
9193
});
9294

9395
test("findArtifacts returns empty array when .math/backups does not exist", () => {
9496
// Remove the backups directory
95-
rmSync(BACKUPS_DIR, { recursive: true, force: true });
97+
rmSync(backupsDir, { recursive: true, force: true });
9698

9799
const result = findArtifacts();
98100
expect(result).toEqual([]);
99101
});
100102

101103
test("findArtifacts returns absolute paths", () => {
102-
mkdirSync(join(BACKUPS_DIR, "core-infrastructure"));
104+
mkdirSync(join(backupsDir, "core-infrastructure"));
103105

104106
const result = findArtifacts();
105107

@@ -110,8 +112,8 @@ test("findArtifacts returns absolute paths", () => {
110112
// deleteArtifacts tests
111113

112114
test("deleteArtifacts deletes directories successfully", () => {
113-
const dir1 = join(TEST_DIR, "todo-1-15-2025");
114-
const dir2 = join(TEST_DIR, "todo-2-20-2025");
115+
const dir1 = join(testDir, "todo-1-15-2025");
116+
const dir2 = join(testDir, "todo-2-20-2025");
115117
mkdirSync(dir1);
116118
mkdirSync(dir2);
117119

@@ -126,7 +128,7 @@ test("deleteArtifacts deletes directories successfully", () => {
126128
});
127129

128130
test("deleteArtifacts deletes directories with contents", () => {
129-
const dir = join(TEST_DIR, "todo-1-15-2025");
131+
const dir = join(testDir, "todo-1-15-2025");
130132
mkdirSync(dir);
131133
Bun.write(join(dir, "file.txt"), "content");
132134
mkdirSync(join(dir, "subdir"));
@@ -147,7 +149,7 @@ test("deleteArtifacts returns empty arrays for empty input", () => {
147149
});
148150

149151
test("deleteArtifacts handles non-existent paths gracefully", () => {
150-
const nonExistent = join(TEST_DIR, "does-not-exist");
152+
const nonExistent = join(testDir, "does-not-exist");
151153

152154
const result = deleteArtifacts([nonExistent]);
153155

@@ -157,8 +159,8 @@ test("deleteArtifacts handles non-existent paths gracefully", () => {
157159
});
158160

159161
test("deleteArtifacts continues after a failure", () => {
160-
const dir1 = join(TEST_DIR, "todo-1-15-2025");
161-
const dir2 = join(TEST_DIR, "todo-2-20-2025");
162+
const dir1 = join(testDir, "todo-1-15-2025");
163+
const dir2 = join(testDir, "todo-2-20-2025");
162164
mkdirSync(dir1);
163165
mkdirSync(dir2);
164166

0 commit comments

Comments
 (0)