[test] Add tests for cmd.newCompletionCmd#4082
Conversation
Comprehensive test coverage for the newCompletionCmd function in internal/cmd/completion.go, which previously had zero test coverage. Tests added: - TestNewCompletionCmd_CommandStructure: verifies Use, Short, Long, DisableFlagsInUseLine, and ValidArgs metadata - TestNewCompletionCmd_PersistentPreRunE: verifies the override always returns nil, bypassing the root's config-validation preRun - TestNewCompletionCmd_ArgsValidation: table-driven test covering all four valid shells plus invalid cases (empty, unknown, too many args) - TestNewCompletionCmd_BashOutput: verifies bash completion generates non-empty output with expected shell tokens - TestNewCompletionCmd_ZshOutput: verifies zsh completion generates non-empty output - TestNewCompletionCmd_FishOutput: verifies fish completion generates non-empty output - TestNewCompletionCmd_PowerShellOutput: verifies powershell completion generates non-empty output - TestNewCompletionCmd_DefaultCaseFallback: exercises the defensive default branch directly via RunE to achieve 100% branch coverage - TestNewCompletionCmd_AllShellsProduceDifferentOutput: ensures each shell generates a distinct completion script - TestNewCompletionCmd_OverridesParentPersistentPreRunE: regression guard confirming completions work without a config file Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds unit tests for the internal/cmd.newCompletionCmd Cobra subcommand to raise coverage from 0% to near-complete, including validation behavior and shell-specific completion generation.
Changes:
- Introduces
internal/cmd/completion_test.gocovering command metadata, args validation, andPersistentPreRunEoverride behavior. - Executes
completionfor bash/zsh/fish/powershell and asserts non-empty output. - Directly exercises the defensive
defaultbranch inRunEfor coverage.
Show a summary per file
| File | Description |
|---|---|
| internal/cmd/completion_test.go | Adds comprehensive tests for the completion command, including output generation and pre-run override regression coverage. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 1
| func captureStdoutDuring(t *testing.T, fn func()) string { | ||
| t.Helper() | ||
| r, w, err := os.Pipe() | ||
| require.NoError(t, err) | ||
|
|
||
| orig := os.Stdout | ||
| os.Stdout = w | ||
| // Safety net: restore if the function panics before we can restore manually. | ||
| t.Cleanup(func() { | ||
| if os.Stdout != orig { | ||
| os.Stdout = orig | ||
| } | ||
| }) | ||
|
|
||
| fn() | ||
|
|
||
| w.Close() | ||
| os.Stdout = orig // restore immediately so repeated calls in the same test work | ||
|
|
||
| var buf bytes.Buffer | ||
| _, err = io.Copy(&buf, r) | ||
| r.Close() | ||
| require.NoError(t, err) | ||
|
|
||
| return buf.String() | ||
| } |
There was a problem hiding this comment.
captureStdoutDuring writes to an os.Pipe() and only starts reading from the pipe after fn() returns. If the completion generator writes more than the pipe buffer (common risk for shell completion scripts as the CLI grows), the write can block and the test can deadlock/hang. Consider draining r concurrently (start a goroutine to io.Copy into a buffer before calling fn()), and use defer to close w/restore os.Stdout so resources are cleaned up even if fn() panics.
|
@copilot address the review feedback #4082 (review) |
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/0247f7a2-b758-496f-b765-effc1dd5b27f Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/0247f7a2-b758-496f-b765-effc1dd5b27f Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Test Coverage Improvement:
newCompletionCmdFunction Analyzed
internal/cmdnewCompletionCmdinternal/cmd/completion.goWhy This Function?
newCompletionCmdhad zero test coverage despite being a user-facing CLI command. It contains:defaultdefensive error path that cobra'sOnlyValidArgsnormally prevents from being reachedPersistentPreRunEoverride that intentionally skips the root command's config-file validation — a critical correctness property with no regression testPrevious agent runs (tracked in cache memory) had improved coverage for
cmd.detectGuardWasm,oidc.extractJWTExpiry, andconfig.AllowOnlyPolicy, leaving this function as the next high-priority target.Tests Added
Use,Short,Long,DisableFlagsInUseLine, andValidArgsmetadatanil(no args or with args)RunEdirectly with unknown shell to exercise the defensivedefaultcasePersistentPreRunEwould fail (no config file)Coverage Report
Test Infrastructure
Tests use an isolated root command (
newTestRootWithCompletion) to avoid triggering the realrootCmd's config-validationPersistentPreRunE. Output is captured viacaptureStdoutDuring, which usesos.Pipe()with immediateos.Stdoutrestoration (plus at.Cleanupsafety net) — safe for sequential calls within a single test.Generated by Test Coverage Improver
Next candidates:
internal/server/response_writer.go,validateCustomServerConfig,resolveGuardPolicyOverrideWarning
The following domains were blocked by the firewall during workflow execution:
proxy.golang.orgreleaseassets.githubusercontent.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.