[test-improver] Improve tests for sys/Docker - #10419
Conversation
Add TestCheckContainerRuntimeAccessible_DockerBranch which exercises the code path in CheckContainerRuntimeAccessible where the command basename is "docker", causing it to delegate to CheckDockerAccessible instead of running the command directly. Before: CheckContainerRuntimeAccessible = 88.9% coverage After: CheckContainerRuntimeAccessible = 100.0% coverage Package total: 99.1% → 100.0% Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds coverage for Docker-specific runtime accessibility checks.
Changes:
- Tests
dockerand absolute Docker command paths. - Identified that assertions do not uniquely verify delegation.
Show a summary per file
| File | Description |
|---|---|
internal/sys/docker_test.go |
Adds Docker-branch coverage tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| func TestCheckContainerRuntimeAccessible_DockerBranch(t *testing.T) { | ||
| // Force a nonexistent Docker socket so CheckDockerAccessible returns false. | ||
| t.Setenv("DOCKER_HOST", "unix:///nonexistent/docker.sock") | ||
|
|
||
| // "docker" basename triggers the CheckDockerAccessible path. | ||
| assert.False(t, CheckContainerRuntimeAccessible("docker"), | ||
| "docker branch should return false when Docker socket is inaccessible") | ||
|
|
||
| // An absolute path with docker basename also triggers the docker branch. | ||
| assert.False(t, CheckContainerRuntimeAccessible("/usr/local/bin/docker"), | ||
| "absolute docker path should also delegate to CheckDockerAccessible") | ||
| } |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot address review feedback |
Addressed in 978a7fc. The Docker-branch test now adds a temporary executable named |
🔒 mcpg Read-Only Stress — gVisorSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: PASS Full ReportPart B gateway error detail: All 7 write tool calls returned Part D/E note: References: §30602265401
|
🔒 mcpg Read-Only Stress — default AWFSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: PASS Notes:
References: §30602265399
|
🔒 mcpg Read-Only Stress — docker-sbxSurface coverage: MCP tool calls + proxied CLI (REST) + GraphQL mutations
Overall: PASS
|
Summary
Improves test coverage for
internal/sys/docker_test.goby adding a missing test case for thedockerbranch ofCheckContainerRuntimeAccessible.File Analyzed
internal/sys/docker_test.go— tests for thesyspackage (container detection and Docker inspection utilities)Improvements Made
Added
TestCheckContainerRuntimeAccessible_DockerBranch: The existingTestCheckContainerRuntimeAccessibletest only covered:"true")/path/that/does/not/exist/podman)It did not exercise the branch inside
CheckContainerRuntimeAccessiblewherefilepath.Base(command) == "docker"causes it to delegate toCheckDockerAccessible()rather than running the command directly.The new test:
DOCKER_HOSTto a nonexistent socket path to get a deterministicfalseresult fromCheckDockerAccessible()CheckContainerRuntimeAccessible("docker")to exercise the basename check"/usr/local/bin/docker"absolute-path form, confirming basename extraction works correctlyCoverage
CheckContainerRuntimeAccessibleinternal/sys)Test Output