Skip to content

refactor: split docker-manager tests into focused files#2496

Merged
lpcox merged 4 commits intomainfrom
refactor/split-docker-manager-tests
May 4, 2026
Merged

refactor: split docker-manager tests into focused files#2496
lpcox merged 4 commits intomainfrom
refactor/split-docker-manager-tests

Conversation

@lpcox
Copy link
Copy Markdown
Collaborator

@lpcox lpcox commented May 4, 2026

Summary

Split the 5,472-line src/docker-manager.test.ts into 4 focused test files for easier navigation and targeted test execution.

File Lines Content
docker-manager-utils.test.ts ~650 subnetsOverlap, validateIdNotInSystemRange, getSafeHostUid/Gid, getRealUserHome, stripScheme, readGitHubPathEntries, parseGitHubEnvFile
docker-manager-compose.test.ts ~3,525 generateDockerCompose tests
docker-manager-lifecycle.test.ts ~599 startContainers, stopContainers, fastKillAgentContainer, runAgentCommand
docker-manager-cleanup.test.ts ~752 writeConfigs, cleanup, collectDiagnosticLogs

Testing

  • All 413 tests accounted for (409 pass, 4 pre-existing failures unchanged)
  • Each file can be run independently: npx jest docker-manager-compose

Closes #2474

Split the 5472-line docker-manager.test.ts into focused test
files: utils, compose, lifecycle, and cleanup. Each file has
its own mock setup and can be run independently.

Closes #2474

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@lpcox lpcox requested a review from Mossaka as a code owner May 4, 2026 15:09
Copilot AI review requested due to automatic review settings May 4, 2026 15:09
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reorganizes docker-manager's monolithic test suite into smaller, purpose-focused files so contributors can navigate and run targeted subsets more easily. It keeps the existing docker-manager.ts behavior under test, but redistributes that coverage across utilities, compose generation, lifecycle, and cleanup concerns.

Changes:

  • Split general utility/helper tests into docker-manager-utils.test.ts.
  • Kept docker-manager-compose.test.ts focused on generateDockerCompose behavior.
  • Moved lifecycle and cleanup/diagnostic tests into dedicated files.
Show a summary per file
File Description
src/docker-manager-utils.test.ts New utility-focused tests for subnet helpers, UID/GID/home helpers, URL parsing, and GitHub env/path parsing.
src/docker-manager-lifecycle.test.ts New lifecycle-focused tests for container startup/shutdown, fast kill behavior, DOCKER_HOST handling, and agent command execution.
src/docker-manager-compose.test.ts Reduced the former monolithic file to generateDockerCompose-centered coverage plus related env/path integration cases.
src/docker-manager-cleanup.test.ts New tests for config writing, cleanup/preservation behavior, and diagnostic log collection.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (1)

src/docker-manager-compose.test.ts:1

  • This file still imports several helpers that are no longer referenced after the split (readGitHubPathEntries, mergeGitHubPathEntries, readGitHubEnvEntries, parseGitHubEnvFile, and readEnvFile). npm run lint runs eslint over src/**/*.ts with @typescript-eslint/no-unused-vars enabled, so this import list will now fail CI until the unused names are removed.
  • Files reviewed: 4/4 changed files
  • Comments generated: 2

Comment thread src/docker-manager-utils.test.ts Outdated
Comment thread src/docker-manager-lifecycle.test.ts Outdated
lpcox and others added 2 commits May 4, 2026 08:21
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@lpcox
Copy link
Copy Markdown
Collaborator Author

lpcox commented May 4, 2026

@copilot resolve the merge conflicts in this pull request

- Resolve rename conflict in docker-manager-compose.test.ts (keep split version)
- Remove unused imports from docker-manager-compose.test.ts
- Add parseDifcProxyHost tests to docker-manager-utils.test.ts (from #2471)
- Add preserveIptablesAudit tests to docker-manager-cleanup.test.ts (from #2471)
- Auto-merge log-streamer.ts refactor from #2495
Copy link
Copy Markdown
Contributor

Copilot AI commented May 4, 2026

@copilot resolve the merge conflicts in this pull request

Done. Merge conflicts resolved in commit 0a8d37d:

427 tests pass; the 2 parseDifcProxyHost failures (HTTPS default-port and IPv6 bracket notation) are pre-existing on main and unchanged here.

Copilot finished work on behalf of lpcox May 4, 2026 15:35
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

🔥 Smoke Test: Copilot BYOK (Offline) Mode

Test Result
GitHub MCP (list PRs)
GitHub.com HTTP connectivity ⚠️ pre-step data not expanded
File write/read ❌ file not found at path
BYOK inference (this response)

Running in BYOK offline mode (COPILOT_OFFLINE=true) via api-proxy → api.githubcopilot.com

PR author: @lpcox · Reviewer: @Mossaka

Overall: PARTIAL — BYOK inference and MCP working; pre-step outputs were not substituted (GitHub Actions template vars unexpanded), so HTTP and file tests could not be fully verified.

🔑 BYOK report filed by Smoke Copilot BYOK

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Smoke Test Results

✅ GitHub MCP: refactor: extract runWithSignalHandling helper, refactor: extract createBaseAdapterConfig helper
✅ Playwright: GitHub page title verified
✅ File Writing: smoke-test-claude-25328086122.txt created
✅ Bash Verification: File content confirmed

Overall: PASS

💥 [THE END] — Illustrated by Smoke Claude

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

🔥 Smoke Test Results

  • ✅ GitHub MCP: connected, fetched PR list
  • ✅ File write/read: smoke file confirmed on disk
  • ✅ GitHub.com connectivity: reachable

PR: "refactor: split docker-manager tests into focused files" by @lpcox

Overall: PASS

📰 BREAKING: Report filed by Smoke Copilot

@github-actions

This comment has been minimized.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Chroot Runtime Version Comparison

Runtime Host Version Chroot Version Match?
Python Python 3.12.13 Python 3.12.3
Node.js v24.14.1 v20.20.2
Go go1.22.12 go1.22.12

Result: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.

Tested by Smoke Chroot

@github-actions

This comment has been minimized.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

Smoke Test Results — Services Connectivity

Check Result
Redis PING ❌ timeout/no response
PostgreSQL pg_isready ❌ no response
PostgreSQL SELECT 1 ❌ skipped (pg_isready failed)

Overall: FAILhost.docker.internal is not reachable from this runner environment. Service containers appear unavailable.

🔌 Service connectivity validated by Smoke Services

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

refactor: extract runWithSignalHandling helper
refactor: extract createBaseAdapterConfig helper
refactor: split docker-manager.ts into focused modules
refactor: split cli.ts into focused modules

GitHub MCP ❌
safeinputs-gh ❌
Playwright ✅
Tavily ❌
File write ✅
Bash cat ✅
Discussion comment ✅
Build ✅
Overall: FAIL

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • registry.npmjs.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "registry.npmjs.org"

See Network Configuration for more information.

🔮 The oracle has spoken through Smoke Codex

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 4, 2026

🏗️ Build Test Suite Results

Ecosystem Project Build/Install Tests Status
Bun elysia 1/1 passed ✅ PASS
Bun hono 1/1 passed ✅ PASS
C++ fmt N/A ✅ PASS
C++ json N/A ✅ PASS
Deno oak N/A 1/1 passed ✅ PASS
Deno std N/A 1/1 passed ✅ PASS
.NET hello-world N/A ✅ PASS
.NET json-parse N/A ✅ PASS
Go color 1/1 passed ✅ PASS
Go env 1/1 passed ✅ PASS
Go uuid 1/1 passed ✅ PASS
Java gson 1/1 passed ✅ PASS
Java caffeine 1/1 passed ✅ PASS
Node.js clsx all passed ✅ PASS
Node.js execa all passed ✅ PASS
Node.js p-limit all passed ✅ PASS
Rust fd 1/1 passed ✅ PASS
Rust zoxide 1/1 passed ✅ PASS

Overall: 8/8 ecosystems passed — PASS

Generated by Build Test Suite for issue #2496 · ● 743.2K ·

@lpcox lpcox merged commit d3c6033 into main May 4, 2026
69 of 76 checks passed
@lpcox lpcox deleted the refactor/split-docker-manager-tests branch May 4, 2026 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Refactoring] Split src/docker-manager.test.ts into focused test files

3 participants