Skip to content

test: add unit tests for terminal ExecCommand detach behavior#181

Open
mason5052 wants to merge 2 commits intovxcontrol:masterfrom
mason5052:test/terminal-detached-context
Open

test: add unit tests for terminal ExecCommand detach behavior#181
mason5052 wants to merge 2 commits intovxcontrol:masterfrom
mason5052:test/terminal-detached-context

Conversation

@mason5052
Copy link
Contributor

Description of the Change

Problem

The terminal ExecCommand function (408 lines) had zero test coverage. The detached execution path -- recently fixed for context isolation (#176, #179) -- particularly needed validation.

Solution

Add mock infrastructure and 4 test cases for terminal command execution:

Mock infrastructure:

  • mockDockerClient: Implements full docker.DockerClient interface with configurable responses, timing delays, and error injection. Uses net.Pipe() for realistic streaming output simulation.
  • mockTermLogProvider: Implements TermLogProvider interface (no-op for tests).
  • Both include compile-time interface checks (var _ Interface = (*mock)(nil)).

Test cases:

  • TestExecCommandDetachReturnsQuickly: Verifies detach=true returns "Command started in background" within ~500ms even when the command takes 2s+
  • TestExecCommandDetachQuickCompletion: Verifies detach=true returns actual output when command finishes before the quick check timeout
  • TestExecCommandNonDetachWaitsForCompletion: Verifies detach=false blocks until command completes and returns full output
  • TestExecCommandContainerNotRunning: Verifies proper error handling when container is not running

The mock infrastructure is reusable for future terminal.go test expansion (ReadFile, WriteFile, etc.).

Related to #176
Adds test coverage for #179

Type of Change

  • Tests (adding or updating tests)

Areas Affected

  • Core Services (Backend API)

Testing and Verification

Test Configuration

  • PentAGI Version: master
  • Go Version: 1.24

Test Steps

  1. go test ./pkg/tools/ -run "TestExecCommand" -v -- all 4 tests pass
  2. go vet ./pkg/tools/ -- no warnings
  3. Pre-existing TestSploitusParseExploitsResponse/nginx failure is unrelated (Windows Defender file access issue on test data)

Security Considerations

No security impact. Test-only change. Mock infrastructure does not interact with real Docker.

Checklist

  • My code follows the project's coding standards
  • All new and existing tests pass
  • I have run go fmt and go vet
  • Security implications considered
  • Changes are backward compatible

Add test infrastructure (mock DockerClient + mock TermLogProvider) and
4 test cases covering terminal command execution paths, particularly
the detached execution behavior related to context isolation (vxcontrol#176).

TestExecCommandDetachReturnsQuickly:
- Verifies detach=true returns within ~500ms even when command is slow

TestExecCommandDetachQuickCompletion:
- Verifies detach=true returns actual output when command finishes fast

TestExecCommandNonDetachWaitsForCompletion:
- Verifies detach=false waits for and returns full command output

TestExecCommandContainerNotRunning:
- Verifies proper error when container is not running

Mock infrastructure implements the full docker.DockerClient interface
with configurable responses and timing delays, reusable for future
terminal.go test expansion.

Signed-off-by: mason5052 <ehehwnwjs5052@gmail.com>
Copilot AI review requested due to automatic review settings March 6, 2026 02:10
Copy link

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

Adds unit test coverage for the terminal.ExecCommand detach and non-detach execution paths, using lightweight Docker/terminal log mocks to simulate exec output and timing behavior.

Changes:

  • Introduces mockDockerClient implementing docker.DockerClient with configurable attach delays and streamed output via net.Pipe().
  • Adds mockTermLogProvider implementing TermLogProvider for no-op logging in tests.
  • Adds four ExecCommand unit tests covering detach quick-return, detach quick-completion, non-detach completion, and “container not running” error behavior.

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


assert.NoError(t, err)
assert.Contains(t, output, "Command started in background")
assert.Less(t, elapsed, 2*time.Second, "detach should return within ~500ms, not wait for command")
Copy link

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

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

TestExecCommandDetachReturnsQuickly claims detach should return within defaultQuickCheckTimeout (500ms), but the assertion only checks < 2s, which can let regressions through (e.g., returning after 1–1.5s would still pass). Consider asserting against defaultQuickCheckTimeout (plus a small scheduling margin) so the test actually enforces the intended contract.

Suggested change
assert.Less(t, elapsed, 2*time.Second, "detach should return within ~500ms, not wait for command")
assert.Less(t, elapsed, 750*time.Millisecond, "detach should return within defaultQuickCheckTimeout (~500ms) plus small margin, not wait for command")

Copilot uses AI. Check for mistakes.
Reduce the timing assertion margin for TestExecCommandDetachReturnsQuickly
from < 2s to < 1s. The quick check timeout is 500ms, so 1s provides
sufficient buffer while catching regressions that 2s would miss.

Signed-off-by: mason5052 <ehehwnwjs5052@gmail.com>
mason5052 added a commit to mason5052/pentagi that referenced this pull request Mar 6, 2026
Rename mockTermLogProvider to contextTestTermLogProvider in
terminal_context_test.go to prevent redeclaration error when
both PR vxcontrol#179 and PR vxcontrol#181 are merged into the same package.

Signed-off-by: mason5052 <ehehwnwjs5052@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants