Conversation
Add 5 new tests for the --proxy-logs-dir flag covering: - API proxy logs sibling directory volume mount (with/without proxyLogsDir) - Recursive creation of nested proxyLogsDir parents - Logs not moved to /tmp when proxyLogsDir is specified - API proxy logs sibling chmod during cleanup Fixes #499 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
Adds additional unit test coverage around the --proxy-logs-dir flag behavior, ensuring Squid and api-proxy log directory handling is correct across compose generation, config writing, and cleanup.
Changes:
- Add
generateDockerComposetests for api-proxy log volume mounts with/withoutproxyLogsDir. - Add
writeConfigstests for creating the api-proxy sibling logs directory and recursive creation of nestedproxyLogsDirpaths. - Add
cleanuptests to ensure logs aren’t moved whenproxyLogsDiris used and that siblingapi-proxy-logsgets chmod’d.
Comments suppressed due to low confidence (1)
src/docker-manager.test.ts:2607
- Same as above:
Date.now()-based temp directory names can collide and make the test flaky under parallel runs. Usefs.mkdtempSync(...)to create a uniqueexternalDirfor the test.
const externalDir = path.join(os.tmpdir(), `awf-proxy-logs-test-${Date.now()}`);
const proxyLogsDir = path.join(externalDir, 'proxy-logs');
const apiProxyLogsDir = path.join(externalDir, 'api-proxy-logs');
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/docker-manager.test.ts
Outdated
| const externalDir = path.join(os.tmpdir(), `awf-proxy-logs-test-${Date.now()}`); | ||
| const proxyLogsDir = path.join(externalDir, 'proxy-logs'); | ||
| fs.mkdirSync(proxyLogsDir, { recursive: true }); |
There was a problem hiding this comment.
Using Date.now() to generate externalDir can collide when tests run in parallel or when the same millisecond is reused, which can cause flaky failures (EEXIST / unexpected cleanup interactions). Prefer fs.mkdtempSync(path.join(os.tmpdir(), 'awf-proxy-logs-test-')) (or similar) for a guaranteed-unique temp directory, consistent with other tests in this file.
This issue also appears on line 2605 of the same file.
| it('should use sibling api-proxy-logs directory when proxyLogsDir is specified', () => { | ||
| const config: WrapperConfig = { | ||
| ...mockConfig, | ||
| proxyLogsDir: '/custom/proxy/logs', | ||
| enableApiProxy: true, | ||
| openaiApiKey: 'sk-test-key', |
There was a problem hiding this comment.
PR description says this adds 5 new unit tests, but this diff appears to introduce 6 new it(...) cases (2 in generateDockerCompose, 2 in writeConfigs, 2 in cleanup). Consider updating the PR description (or dropping/combining a test) so the summary matches the changes.
|
Smoke test results for run 22931715733 —
Overall: PASS
|
|
PR titles (merged, latest):
|
Smoke Test Results
Overall: PASS
|
Chroot Version Comparison Results
Overall: ❌ FAILED — Python and Node.js versions differ between host and chroot environments.
|
This comment has been minimized.
This comment has been minimized.
Replace manual Date.now()-based temp dir construction with fs.mkdtempSync() to satisfy CodeQL insecure-temporary-file checks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Smoke test results for ✅ GitHub MCP — Last 2 merged PRs: Overall: PASS
|
Smoke Test Results
Overall: PASS
|
|
PR titles: test: add --allow-host-ports validation tests | test: add --proxy-logs-dir edge case coverage
|
Chroot Version Comparison Results
Overall: FAILED — Python and Node.js versions differ between host and chroot environments.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Summary
--proxy-logs-dirflag covering missing edge casesFixes #499
Test plan
npm run buildpassesnpm testpasses (845 tests)npm run lintpasses (0 errors)🤖 Generated with Claude Code