Skip to content

Commit 8ee1833

Browse files
authored
Refactor workdir setup tests and guard fixture cleanup (#5303)
* Initial plan * refactor(tests): extract shared fixture in workdir-setup.test.ts Removes ~45 duplicated lines across three describe blocks by introducing setupWorkdirFixture(). Suites opt out of chroot-home cleanup via the cleanupChrootHome option. * test: guard temp dir cleanup in workdir fixture --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 parent 6707484 commit 8ee1833

1 file changed

Lines changed: 39 additions & 75 deletions

File tree

src/workdir-setup.test.ts

Lines changed: 39 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,8 @@ import { prepareWorkDirectories, workdirSetupTestHelpers } from './workdir-setup
1515
import { resolveLogPaths } from './log-paths';
1616
import { getRealUserHome } from './host-identity';
1717

18-
describe('prepareWorkDirectories', () => {
19-
let tempDir: string;
18+
function setupWorkdirFixture({ cleanupChrootHome = true } = {}) {
19+
let tempDir = '';
2020

2121
const buildConfig = (overrides: Record<string, unknown> = {}) => ({
2222
workDir: tempDir,
@@ -39,10 +39,28 @@ describe('prepareWorkDirectories', () => {
3939
});
4040

4141
afterEach(() => {
42+
if (!tempDir) {
43+
return;
44+
}
45+
4246
fs.rmSync(tempDir, { recursive: true, force: true });
43-
fs.rmSync(`${tempDir}-chroot-home`, { recursive: true, force: true });
47+
if (cleanupChrootHome) {
48+
fs.rmSync(`${tempDir}-chroot-home`, { recursive: true, force: true });
49+
}
4450
});
4551

52+
return {
53+
buildConfig,
54+
get tempDir() {
55+
return tempDir;
56+
},
57+
};
58+
}
59+
60+
describe('prepareWorkDirectories', () => {
61+
const fixture = setupWorkdirFixture();
62+
const { buildConfig } = fixture;
63+
4664
describe('log/state directory setup', () => {
4765
it('creates agent logs directory', () => {
4866
const config = buildConfig();
@@ -121,7 +139,7 @@ describe('prepareWorkDirectories', () => {
121139
});
122140

123141
it('falls back to world-writable squid logs when squid chown fails', () => {
124-
const proxyLogsDir = path.join(tempDir, 'proxy-logs');
142+
const proxyLogsDir = path.join(fixture.tempDir, 'proxy-logs');
125143
(fs.chownSync as unknown as jest.Mock).mockImplementation((targetPath: fs.PathLike) => {
126144
if (String(targetPath) === proxyLogsDir) {
127145
throw new Error('chown failed');
@@ -140,7 +158,7 @@ describe('prepareWorkDirectories', () => {
140158

141159
describe('chroot home bind-mount preparation', () => {
142160
it('creates chroot home directory when it does not exist', () => {
143-
const emptyHomeDir = `${tempDir}-chroot-home`;
161+
const emptyHomeDir = `${fixture.tempDir}-chroot-home`;
144162
expect(fs.existsSync(emptyHomeDir)).toBe(false);
145163

146164
const config = buildConfig();
@@ -153,7 +171,7 @@ describe('prepareWorkDirectories', () => {
153171
});
154172

155173
it('uses existing chroot home directory if already present', () => {
156-
const emptyHomeDir = `${tempDir}-chroot-home`;
174+
const emptyHomeDir = `${fixture.tempDir}-chroot-home`;
157175
fs.mkdirSync(emptyHomeDir, { recursive: true });
158176
const statBefore = fs.statSync(emptyHomeDir);
159177

@@ -167,9 +185,7 @@ describe('prepareWorkDirectories', () => {
167185
});
168186

169187
it('creates missing home subdirectories with correct ownership', () => {
170-
(getRealUserHome as jest.Mock).mockReturnValue(tempDir);
171-
172-
const copilotDir = path.join(tempDir, '.copilot');
188+
const copilotDir = path.join(fixture.tempDir, '.copilot');
173189
if (fs.existsSync(copilotDir)) {
174190
fs.rmSync(copilotDir, { recursive: true, force: true });
175191
}
@@ -184,9 +200,7 @@ describe('prepareWorkDirectories', () => {
184200
});
185201

186202
it('creates .gemini directory when geminiApiKey is provided', () => {
187-
(getRealUserHome as jest.Mock).mockReturnValue(tempDir);
188-
189-
const geminiDir = path.join(tempDir, '.gemini');
203+
const geminiDir = path.join(fixture.tempDir, '.gemini');
190204
if (fs.existsSync(geminiDir)) {
191205
fs.rmSync(geminiDir, { recursive: true, force: true });
192206
}
@@ -200,9 +214,7 @@ describe('prepareWorkDirectories', () => {
200214
});
201215

202216
it('does not create .gemini directory when geminiApiKey is not provided', () => {
203-
(getRealUserHome as jest.Mock).mockReturnValue(tempDir);
204-
205-
const geminiDir = path.join(tempDir, '.gemini');
217+
const geminiDir = path.join(fixture.tempDir, '.gemini');
206218
if (fs.existsSync(geminiDir)) {
207219
fs.rmSync(geminiDir, { recursive: true, force: true });
208220
}
@@ -216,7 +228,7 @@ describe('prepareWorkDirectories', () => {
216228
});
217229

218230
it('creates configured runner tool cache directory segments with correct ownership', () => {
219-
const runnerToolCacheParent = path.join(tempDir, 'runner-work');
231+
const runnerToolCacheParent = path.join(fixture.tempDir, 'runner-work');
220232
const runnerToolCachePath = path.join(runnerToolCacheParent, '_tool');
221233
expect(fs.existsSync(runnerToolCachePath)).toBe(false);
222234

@@ -234,12 +246,11 @@ describe('prepareWorkDirectories', () => {
234246
});
235247

236248
it('throws when runnerToolCachePath contains a pre-existing non-root-owned intermediate symlink', () => {
237-
const realDir = path.join(tempDir, 'real-dir');
238-
const symlinkDir = path.join(tempDir, 'link-to-real');
249+
const realDir = path.join(fixture.tempDir, 'real-dir');
250+
const symlinkDir = path.join(fixture.tempDir, 'link-to-real');
239251
fs.mkdirSync(realDir, { recursive: true });
240252
fs.symlinkSync(realDir, symlinkDir);
241253
const runnerToolCachePath = path.join(symlinkDir, 'child');
242-
(getRealUserHome as jest.Mock).mockReturnValue(tempDir);
243254

244255
const config = buildConfig({ runnerToolCachePath });
245256
const logPaths = resolveLogPaths(config);
@@ -250,7 +261,7 @@ describe('prepareWorkDirectories', () => {
250261
});
251262

252263
it('prepares chroot mountpoint for fallback runner tool cache under home', () => {
253-
const runnerToolCachePath = path.join(tempDir, 'work', '_tool');
264+
const runnerToolCachePath = path.join(fixture.tempDir, 'work', '_tool');
254265
fs.mkdirSync(runnerToolCachePath, { recursive: true });
255266

256267
const savedRunnerToolCache = process.env.RUNNER_TOOL_CACHE;
@@ -266,7 +277,7 @@ describe('prepareWorkDirectories', () => {
266277
}
267278
}
268279

269-
const chrootWorkDir = path.join(`${tempDir}-chroot-home`, 'work');
280+
const chrootWorkDir = path.join(`${fixture.tempDir}-chroot-home`, 'work');
270281
const chrootToolCacheDir = path.join(chrootWorkDir, '_tool');
271282
expect(fs.existsSync(chrootToolCacheDir)).toBe(true);
272283
expect(fs.statSync(chrootToolCacheDir).isDirectory()).toBe(true);
@@ -279,31 +290,8 @@ describe('prepareWorkDirectories', () => {
279290
});
280291

281292
describe('prepareLogDirectories (sub-function)', () => {
282-
let tempDir: string;
283-
284-
const buildConfig = (overrides: Record<string, unknown> = {}) => ({
285-
workDir: tempDir,
286-
sslBump: false,
287-
allowedDomains: [] as string[],
288-
agentCommand: 'echo test',
289-
logLevel: 'info' as const,
290-
keepContainers: false,
291-
buildLocal: false,
292-
imageRegistry: 'ghcr.io/github/gh-aw-firewall',
293-
imageTag: 'latest',
294-
...overrides,
295-
});
296-
297-
beforeEach(() => {
298-
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'workdir-setup-test-'));
299-
jest.clearAllMocks();
300-
(fs.chownSync as unknown as jest.Mock).mockImplementation(() => undefined);
301-
(getRealUserHome as jest.Mock).mockReturnValue(tempDir);
302-
});
303-
304-
afterEach(() => {
305-
fs.rmSync(tempDir, { recursive: true, force: true });
306-
});
293+
const fixture = setupWorkdirFixture({ cleanupChrootHome: false });
294+
const { buildConfig } = fixture;
307295

308296
it('creates all log directories without touching chroot home', () => {
309297
const config = buildConfig();
@@ -316,44 +304,20 @@ describe('prepareLogDirectories (sub-function)', () => {
316304
expect(fs.existsSync(logPaths.apiProxyLogs)).toBe(true);
317305
expect(fs.existsSync(logPaths.cliProxyLogs)).toBe(true);
318306
// chroot home must NOT have been created
319-
expect(fs.existsSync(`${tempDir}-chroot-home`)).toBe(false);
307+
expect(fs.existsSync(`${fixture.tempDir}-chroot-home`)).toBe(false);
320308
});
321309
});
322310

323311
describe('prepareChrootHomeMounts (sub-function)', () => {
324-
let tempDir: string;
325-
326-
const buildConfig = (overrides: Record<string, unknown> = {}) => ({
327-
workDir: tempDir,
328-
sslBump: false,
329-
allowedDomains: [] as string[],
330-
agentCommand: 'echo test',
331-
logLevel: 'info' as const,
332-
keepContainers: false,
333-
buildLocal: false,
334-
imageRegistry: 'ghcr.io/github/gh-aw-firewall',
335-
imageTag: 'latest',
336-
...overrides,
337-
});
338-
339-
beforeEach(() => {
340-
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), 'workdir-setup-test-'));
341-
jest.clearAllMocks();
342-
(fs.chownSync as unknown as jest.Mock).mockImplementation(() => undefined);
343-
(getRealUserHome as jest.Mock).mockReturnValue(tempDir);
344-
});
345-
346-
afterEach(() => {
347-
fs.rmSync(tempDir, { recursive: true, force: true });
348-
fs.rmSync(`${tempDir}-chroot-home`, { recursive: true, force: true });
349-
});
312+
const fixture = setupWorkdirFixture();
313+
const { buildConfig } = fixture;
350314

351315
it('creates chroot home directory without touching log directories', () => {
352316
const config = buildConfig();
353317
const logPaths = resolveLogPaths(config);
354318
workdirSetupTestHelpers.prepareChrootHomeMounts(config);
355319

356-
expect(fs.existsSync(`${tempDir}-chroot-home`)).toBe(true);
320+
expect(fs.existsSync(`${fixture.tempDir}-chroot-home`)).toBe(true);
357321
// log directories must NOT have been created
358322
expect(fs.existsSync(logPaths.agentLogs)).toBe(false);
359323
expect(fs.existsSync(logPaths.sessionState)).toBe(false);
@@ -363,7 +327,7 @@ describe('prepareChrootHomeMounts (sub-function)', () => {
363327
});
364328

365329
it('creates .gemini directory only when geminiApiKey is provided', () => {
366-
const geminiDir = path.join(tempDir, '.gemini');
330+
const geminiDir = path.join(fixture.tempDir, '.gemini');
367331

368332
workdirSetupTestHelpers.prepareChrootHomeMounts(buildConfig({ geminiApiKey: 'key' }));
369333
expect(fs.existsSync(geminiDir)).toBe(true);

0 commit comments

Comments
 (0)