refactor: extract shared config-writer test harness to eliminate duplicate setup#5702
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the config-writer unit test suites by extracting duplicated Jest mock setup, temp-dir lifecycle handling, and minimal WrapperConfig construction into a shared test harness under src/test-helpers/, reducing repetition while preserving Jest’s per-file jest.mock() hoisting requirements.
Changes:
- Added
config-writer-test-harness.test-utils.tsto centralize mock factories, abuildWriteConfig(tempDir, overrides)factory, and temp directory setup/cleanup helpers. - Updated
config-writer.test.tsandconfig-writer-branches.test.tsto use the shared harness and collapse repeatedjest.mock()blocks into one-liners.
Show a summary per file
| File | Description |
|---|---|
| src/test-helpers/config-writer-test-harness.test-utils.ts | New shared harness providing mock factories and temp-dir/config factories for config-writer tests. |
| src/config-writer.test.ts | Replaces duplicated mocks and per-test setup/teardown with harness helpers; updates buildWriteConfig call sites. |
| src/config-writer-branches.test.ts | Same refactor as above for branch-focused tests; updates buildWriteConfig call sites. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
|
✅ Copilot review passed with no inline comments. @copilot Add the |
|
✅ Contribution Check completed successfully! Contribution check complete: PR #5702 follows the applicable CONTRIBUTING.md guidance. It is a test-only refactor with a clear description, appropriate test-helper placement under src/test-helpers, and no documentation or new-functionality tests required. |
|
✅ Build Test Suite completed successfully! |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
🚀 Security Guard has started processing this pull request |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
✅ Smoke Claude passed |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
|
🔌 Smoke Services — All services reachable! ✅ |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
Renamed the second '## Build-tools sysroot image' heading to '## Sysroot staging lifecycle' to resolve MD024/no-duplicate-heading. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.39% | 98.43% | 📈 +0.04% |
| Statements | 98.31% | 98.36% | 📈 +0.05% |
| Functions | 99.54% | 99.55% | ➡️ +0.01% |
| Branches | 94.44% | 94.41% | 📉 -0.03% |
📁 Per-file Coverage Changes (1 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/workdir-setup.ts |
92.7% → 94.5% (+1.82%) | 92.7% → 94.5% (+1.82%) |
✨ New Files (1 files)
src/test-helpers/config-writer-test-harness.test-utils.ts: 100.0% lines
Coverage comparison generated by scripts/ci/compare-coverage.ts
|
| Metric | Base | PR | Delta |
|---|---|---|---|
| Lines | 98.39% | 98.43% | 📈 +0.04% |
| Statements | 98.31% | 98.36% | 📈 +0.05% |
| Functions | 99.54% | 99.55% | ➡️ +0.01% |
| Branches | 94.44% | 94.41% | 📉 -0.03% |
📁 Per-file Coverage Changes (1 files)
| File | Lines (Before → After) | Statements (Before → After) |
|---|---|---|
src/workdir-setup.ts |
92.7% → 94.5% (+1.82%) | 92.7% → 94.5% (+1.82%) |
✨ New Files (1 files)
src/test-helpers/config-writer-test-harness.test-utils.ts: 100.0% lines
Coverage comparison generated by scripts/ci/compare-coverage.ts
Smoke Test: Claude Engine Validation
Overall result: PASS ✅
|
🔬 Smoke Test: Copilot PAT Auth — PR #5702
Overall: Auth mode: PAT (COPILOT_GITHUB_TOKEN) |
|
|
Smoke test summary
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "registry.npmjs.org"See Network Configuration for more information.
|
|
PR titles:
✅ MCP testing, GitHub.com, File I/O, BYOK inference
|
|
Smoke Test: Copilot BYOK (Direct) Mode ✅ GitHub MCP connectivity Status: PASS
|
|
Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) Overall: PASS
|
🔥 Smoke Test ResultsPR: refactor: extract shared config-writer test harness to eliminate duplicate setup
Overall: PASS ✅
|
Chroot Version Comparison Results
Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
📡 OTel Tracing Smoke Test Results
All scenarios pass or are expected-pending during development. ✅
|
Smoke Test Results
Overall status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Smoke Test: Services Connectivity — ❌ FAIL
|
config-writer.test.tsandconfig-writer-branches.test.tsduplicated ~53 lines of identical Jest mock setup, abuildWriteConfigfactory, and temp-dir lifecycle boilerplate verbatim.New:
src/test-helpers/config-writer-test-harness.test-utils.tsCentralises everything that was repeated:
sslBumpMockFactory,domainMatchersMockFactory,squidConfigMockFactory,composeGeneratorMockFactory— consumed viarequire()inside each file'sjest.mock()declarations (which must stay per-file due to Jest hoisting)buildWriteConfig(tempDir, overrides?)— minimal validWrapperConfigfactorysetupConfigWriterTempDir(prefix?)— creates temp dir, clears mocks, stubschownSyncandgetRealUserHome; returns the pathcleanupConfigWriterTempDir(tempDir)— removes temp dir and-chroot-homesiblingUpdated test files
The seven
jest.mock()blocks collapse from ~31 lines to 7 one-liners each:beforeEach/afterEachbodies and the localbuildWriteConfigdeclaration are replaced with imports from the harness. All existing call sites updated to the newbuildWriteConfig(tempDir, overrides)signature.