Skip to content

Commit e2197c0

Browse files
abhishekSavaniabhishekSavani
committed
test-runner: add cleanup tests for event listeners and memory leaks in runner.js
1 parent 4451309 commit e2197c0

File tree

3 files changed

+302
-6
lines changed

3 files changed

+302
-6
lines changed

‎lib/internal/test_runner/runner.js‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -444,6 +444,9 @@ function runTestFile(path, filesWatcher, opts) {
444444
finished(child.stdout, { __proto__: null, signal: t.signal }),
445445
]);
446446

447+
// Close readline interface to prevent memory leak
448+
rl.close();
449+
447450
if (watchMode) {
448451
filesWatcher.runningProcesses.delete(path);
449452
filesWatcher.runningSubtests.delete(path);
@@ -506,7 +509,7 @@ function watchFiles(testFiles, opts) {
506509
}
507510

508511
// Watch for changes in current filtered files
509-
watcher.on('changed', ({ owners, eventType }) => {
512+
const onChanged = ({ owners, eventType }) => {
510513
if (!opts.hasFiles && (eventType === 'rename' || eventType === 'change')) {
511514
const updatedTestFiles = createTestFileList(opts.globPatterns, opts.cwd);
512515
const newFileName = ArrayPrototypeFind(updatedTestFiles, (x) => !ArrayPrototypeIncludes(testFiles, x));
@@ -547,19 +550,29 @@ function watchFiles(testFiles, opts) {
547550
triggerUncaughtException(error, true /* fromPromise */);
548551
}));
549552
}
550-
});
553+
};
554+
555+
watcher.on('changed', onChanged);
556+
557+
// Cleanup function to remove event listener and prevent memory leak
558+
const cleanup = () => {
559+
watcher.removeListener('changed', onChanged);
560+
opts.root.harness.watching = false;
561+
opts.root.postRun();
562+
};
563+
551564
if (opts.signal) {
552565
kResistStopPropagation ??= require('internal/event_target').kResistStopPropagation;
553566
opts.signal.addEventListener(
554567
'abort',
555-
() => {
556-
opts.root.harness.watching = false;
557-
opts.root.postRun();
558-
},
568+
cleanup,
559569
{ __proto__: null, once: true, [kResistStopPropagation]: true },
560570
);
561571
}
562572

573+
// Expose cleanup method for proper resource management
574+
filesWatcher.cleanup = cleanup;
575+
563576
return filesWatcher;
564577
}
565578

‎test/parallel/test-runner-cleanup-listeners.js‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
'use strict';
2+
3+
// Test for proper cleanup of event listeners in runner.js
4+
// This specifically tests the fixes for:
5+
// 1. Readline interface not being closed (line 448)
6+
// 2. Watch mode event listeners not being removed (line 558-574)
7+
8+
require('../common');
9+
const assert = require('assert');
10+
const { spawnSync } = require('child_process');
11+
const { writeFileSync, mkdirSync } = require('fs');
12+
const { join } = require('path');
13+
const tmpdir = require('../common/tmpdir');
14+
15+
tmpdir.refresh();
16+
17+
// Test 1: Verify test runs complete without hanging (readline cleanup)
18+
{
19+
const testFile = join(tmpdir.path, 'simple-test.js');
20+
writeFileSync(
21+
testFile,
22+
`
23+
const test = require('node:test');
24+
test('simple test', () => {
25+
console.log('test output');
26+
console.error('error output');
27+
});
28+
`
29+
);
30+
31+
const args = ['--test', '--test-isolation=process', testFile];
32+
const child = spawnSync(process.execPath, args, {
33+
timeout: 5000,
34+
});
35+
36+
// Should not timeout and should exit cleanly
37+
assert.strictEqual(child.status, 0);
38+
assert.strictEqual(child.signal, null);
39+
assert.ok(child.stdout.toString().includes('test output'));
40+
assert.ok(child.stderr.toString().includes('error output'));
41+
}
42+
43+
// Test 2: Verify multiple test files don't accumulate resources
44+
{
45+
const testDir = join(tmpdir.path, 'multiple-tests');
46+
mkdirSync(testDir, { recursive: true });
47+
48+
// Create multiple test files
49+
for (let i = 0; i < 5; i++) {
50+
const testFile = join(testDir, `test-${i}.js`);
51+
writeFileSync(
52+
testFile,
53+
`
54+
const test = require('node:test');
55+
test('test ${i}', () => {
56+
console.log('Running test ${i}');
57+
});
58+
`
59+
);
60+
}
61+
62+
const args = ['--test', '--test-isolation=process', testDir];
63+
const child = spawnSync(process.execPath, args, {
64+
timeout: 10000,
65+
});
66+
67+
// All tests should pass and not timeout
68+
assert.strictEqual(child.status, 0);
69+
assert.strictEqual(child.signal, null);
70+
71+
const output = child.stdout.toString();
72+
for (let i = 0; i < 5; i++) {
73+
assert.ok(output.includes(`test ${i}`));
74+
}
75+
}
76+
77+
// Test 3: Verify cleanup with test failures
78+
{
79+
const testFile = join(tmpdir.path, 'failing-test.js');
80+
writeFileSync(
81+
testFile,
82+
`
83+
const test = require('node:test');
84+
const assert = require('assert');
85+
86+
test('failing test', () => {
87+
console.error('About to fail');
88+
assert.fail('Expected failure');
89+
});
90+
`
91+
);
92+
93+
const args = ['--test', '--test-isolation=process', testFile];
94+
const child = spawnSync(process.execPath, args, {
95+
timeout: 5000,
96+
});
97+
98+
// Should fail but not hang
99+
assert.strictEqual(child.status, 1);
100+
assert.strictEqual(child.signal, null);
101+
}
102+
103+
// Test 4: Verify isolation=none mode cleanup
104+
{
105+
const testFile = join(tmpdir.path, 'isolation-none-test.js');
106+
writeFileSync(
107+
testFile,
108+
`
109+
const test = require('node:test');
110+
test('isolation none test', () => {
111+
console.log('Running in same process');
112+
});
113+
`
114+
);
115+
116+
const args = ['--test', '--test-isolation=none', testFile];
117+
const child = spawnSync(process.execPath, args, {
118+
timeout: 5000,
119+
});
120+
121+
assert.strictEqual(child.status, 0);
122+
assert.strictEqual(child.signal, null);
123+
}
124+
125+
// Test 5: Verify proper handling of stderr with special characters
126+
{
127+
const testFile = join(tmpdir.path, 'stderr-test.js');
128+
writeFileSync(
129+
testFile,
130+
`
131+
const test = require('node:test');
132+
test('stderr with special chars', () => {
133+
console.error('Error with special chars: \\n\\t\\r');
134+
console.error('Unicode: \u{1F4A9}');
135+
});
136+
`
137+
);
138+
139+
const args = ['--test', '--test-isolation=process', testFile];
140+
const child = spawnSync(process.execPath, args, {
141+
timeout: 5000,
142+
});
143+
144+
assert.strictEqual(child.status, 0);
145+
assert.strictEqual(child.signal, null);
146+
}

‎test/parallel/test-runner-memory-leak-fix.js‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
'use strict';
2+
3+
// Test for memory leak fixes in runner.js
4+
// This test verifies that:
5+
// 1. Readline interface is properly closed after child process exits
6+
// 2. Watch mode event listeners are properly removed
7+
// 3. No event listeners accumulate over multiple test runs
8+
9+
const common = require('../common');
10+
const assert = require('assert');
11+
const { run } = require('internal/test_runner/runner');
12+
const { writeFileSync } = require('fs');
13+
const { join } = require('path');
14+
const tmpdir = require('../common/tmpdir');
15+
16+
tmpdir.refresh();
17+
18+
// Test 1: Verify readline interface cleanup
19+
{
20+
const testFile = join(tmpdir.path, 'test-1.js');
21+
writeFileSync(
22+
testFile,
23+
`
24+
const test = require('node:test');
25+
test('should pass', () => {
26+
// Simple passing test
27+
});
28+
`
29+
);
30+
31+
const reporter = run({
32+
files: [testFile],
33+
isolation: 'process',
34+
});
35+
36+
let testCount = 0;
37+
reporter.on('test:pass', () => {
38+
testCount++;
39+
});
40+
41+
reporter.on(
42+
'test:complete',
43+
common.mustCall(() => {
44+
assert.strictEqual(testCount, 1);
45+
})
46+
);
47+
}
48+
49+
// Test 2: Verify proper cleanup in isolation mode
50+
{
51+
const testFile = join(tmpdir.path, 'test-2.js');
52+
writeFileSync(
53+
testFile,
54+
`
55+
const test = require('node:test');
56+
test('another test', () => {
57+
console.log('Test running');
58+
});
59+
`
60+
);
61+
62+
const reporter = run({
63+
files: [testFile],
64+
isolation: 'process',
65+
});
66+
67+
let completed = false;
68+
reporter.on(
69+
'test:complete',
70+
common.mustCall(() => {
71+
completed = true;
72+
})
73+
);
74+
75+
// Give some time for cleanup to occur
76+
setTimeout(
77+
common.mustCall(() => {
78+
assert.strictEqual(completed, true);
79+
}),
80+
1000
81+
);
82+
}
83+
84+
// Test 3: Verify cleanup with signal abort
85+
{
86+
const testFile = join(tmpdir.path, 'test-3.js');
87+
writeFileSync(
88+
testFile,
89+
`
90+
const test = require('node:test');
91+
test('long running test', async () => {
92+
await new Promise(resolve => setTimeout(resolve, 5000));
93+
});
94+
`
95+
);
96+
97+
const controller = new AbortController();
98+
const reporter = run({
99+
files: [testFile],
100+
signal: controller.signal,
101+
isolation: 'process',
102+
});
103+
104+
let testStarted = false;
105+
reporter.on('test:start', () => {
106+
testStarted = true;
107+
setTimeout(() => controller.abort(), 100);
108+
});
109+
110+
reporter.on(
111+
'test:complete',
112+
common.mustCall(() => {
113+
assert.strictEqual(testStarted, true);
114+
})
115+
);
116+
}
117+
118+
// Test 4: Verify no listener accumulation with multiple runs
119+
{
120+
const testFile = join(tmpdir.path, 'test-4.js');
121+
writeFileSync(
122+
testFile,
123+
`
124+
const test = require('node:test');
125+
test('simple test', () => {});
126+
`
127+
);
128+
129+
for (let i = 0; i < 3; i++) {
130+
const reporter = run({
131+
files: [testFile],
132+
isolation: 'process',
133+
});
134+
135+
reporter.on('test:complete', common.mustCall());
136+
}
137+
}

0 commit comments

Comments
 (0)