[purelock] Lock down validateCopilotSetupStepsRunsOn, extractGitHubToolsets, buildConnectionString with pure-function test suite [Content truncated due to length] - #54004
Conversation
- validateCopilotSetupStepsRunsOn (pkg/cli/copilot_setup.go): 57.9% -> 100% - extractGitHubToolsets (pkg/cli/codemod_dependabot_permissions.go): 62.5% -> 100% - buildConnectionString (pkg/cli/mcp_inspect_mcp.go): 0% -> 100% Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Great work on the test coverage expansion! 🎉 This PR adds comprehensive test suites for three pure Go functions, achieving 100% function coverage for each. Why this is excellent:
This PR is aligned with the project's contribution guidelines and ready for review!
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel analysis complete: PR #54004 is already merged. Analysis shows excellent test quality: 80/100 score with 36 table-driven subtests covering 3 pure functions (100% coverage achieved). 0% implementation-test ratio, no violations. All assertions descriptive, no mocking violations, no build-tag issues. Edge-case coverage comprehensive (type variants, empty/whitespace, container priority).
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
No actionable issues found in the changed lines.
Review notes
This PR only adds focused table-driven tests for existing pure helper functions, and the added cases cover the important accepted/rejected input shapes without changing production behavior. I did not find a correctness, security, performance, or maintainability issue in the added test code that would justify blocking merge.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 3.42 AIC · ⌖ 7.93 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Pure test additions only — three new table-driven test files covering validateCopilotSetupStepsRunsOn, extractGitHubToolsets, and buildConnectionString. Branch coverage is thorough (whitespace trimming, type coercion, empty/nil edge cases). No production code changed; no blocking issues found. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.2 AIC · ⌖ 11.6 AIC · ⊞ 5.8K
There was a problem hiding this comment.
Pull request overview
Adds comprehensive branch-coverage tests for three pure pkg/cli helper functions.
Changes:
- Tests Copilot setup runner validation edge cases.
- Tests GitHub toolset extraction and normalization.
- Tests MCP connection-string formatting.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/copilot_setup_runs_on_test.go |
Covers supported and invalid runs-on forms. |
pkg/cli/codemod_dependabot_permissions_extract_test.go |
Covers toolset extraction input variants. |
pkg/cli/mcp_inspect_mcp_conn_test.go |
Covers connection formatting branches. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — solid test-suite additions, with a few minor specification gaps worth tightening. Overall COMMENT (no blocking issues).
📋 Key Themes & Highlights
Key Themes
- nil vs empty-slice contract (
codemod_dependabot_permissions_extract_test.go):reflect.DeepEqualmakes the nil/non-nil distinction load-bearing. If the function implementation ever returns[]string{}instead ofnil, tests fail despite correct behaviour. The nil-return contract should be explicit or the assertion relaxed. - Silent-skip semantics (
[]anywith mixed types): the test verifies that non-string items are skipped but doesn't verify the all-non-string edge case separately, leaving an ambiguity about thewantOKvalue. - Fall-through branch coverage (
copilot_setup_runs_on_test.go): the test name documents agroup → labelsfall-through, but a sibling case forempty group + missing labelsis absent.
Positive Highlights
- ✅ Thorough table-driven structure with clear Arrange/Assert separation in all three files
- ✅
buildConnectionStringgoing from 0% to 100% coverage is particularly valuable - ✅ Whitespace and empty-value edge cases are well-exercised across all three suites
- ✅
!integrationbuild tag applied consistently, keeping the unit tests fast - ✅
go test -racepassing is a strong signal on a pure-function suite
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 41.8 AIC · ⌖ 11.6 AIC · ⊞ 7.9K
Comment /matt to run again
| "tools": map[string]any{ | ||
| "github": map[string]any{ | ||
| "toolsets": " , , ", | ||
| }, |
There was a problem hiding this comment.
[/tdd] reflect.DeepEqual distinguishes nil from []string{}, so if the implementation ever returns an empty non-nil slice for the empty-input case, this assertion will fail despite correct behaviour. The nil/non-nil contract should be explicit.
💡 Suggestion
Either document that the function must return nil (not []string{}) on failure, or use a length-based assertion that doesn't conflate nil and empty:
if len(got) != len(tt.wantToolsets) {
t.Fatalf("toolsets length = %d, want %d; got %#v", len(got), len(tt.wantToolsets), got)
}
for i := range tt.wantToolsets {
if got[i] != tt.wantToolsets[i] {
t.Fatalf("toolsets[%d] = %q, want %q", i, got[i], tt.wantToolsets[i])
}
}Use reflect.DeepEqual only when the nil/non-nil distinction is a documented, intentional part of the contract.
@copilot please address this.
| "toolsets": []any{"repos", " issues ", "", 42, nil}, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
[/tdd] The []any{"repos", " issues ", "", 42, nil} test case verifies that non-string items (42, nil) are silently skipped — but there's no assertion that the function returns wantOK: true even though some items were ignored. This may mask a silent data-loss bug.
💡 Suggestion
Consider adding a dedicated subtest that expects wantOK: false (or an error return) when all items in the []any slice are non-strings — so the test suite distinguishes "partially non-string = partial success" from "all non-string = failure". This keeps the spec unambiguous and prevents the implementation from silently returning success with fewer toolsets than the caller expects.
@copilot please address this.
| }, | ||
| { | ||
| name: "map with labels as non-empty slice", | ||
| job: map[string]any{"runs-on": map[string]any{"labels": []any{"linux", "x64"}}}, |
There was a problem hiding this comment.
[/tdd] The "map with empty group falls through to labels string" test name implies the implementation has a fall-through from group to labels — but the test itself only covers labels: "linux" (non-empty string). The name is a specification claim about branching logic, but it's not verified by a companion test that also has an empty group AND empty labels.
💡 Suggestion
Add a sibling case to make the fall-through rule explicit:
{
name: "map with empty group and no labels key",
job: map[string]any{"runs-on": map[string]any{"group": ""}},
wantErr: true,
},This specifies the complete fall-through contract (empty group + missing labels = error) and prevents a future refactor from silently breaking the branching order.
@copilot please address this.
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The "empty type returns empty string" case for buildConnectionString documents that an empty Type field returns "". This is functionally testable but the expected behaviour may be surprising — callers can't distinguish "unknown type" (which returns the type string) from "empty type" (which returns ""). A brief comment or test-name clarification would help future readers understand whether this is intentional.
💡 Suggestion
Rename the test case to make the spec explicit:
{
name: "empty type returns empty string (default branch)",
...
},Or add a brief inline comment in the test explaining the intended contract. This helps differentiate it from the "unrecognized type returns type itself" case which could be confused with the empty case.
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
Summary
Adds pure-function unit test suites for three previously under-tested functions in
pkg/cli:extractGitHubToolsets,validateCopilotSetupStepsRunsOn, andbuildConnectionString. No production code is modified; the change is purely additive test coverage.Change Classification
Key Changes
extractGitHubToolsets, covering missing/malformedtools/github/toolsetskeys and all supported toolsets value types ([]string,[]any, comma-separatedstring) with trimming/filtering behavior.validateCopilotSetupStepsRunsOn, covering string/slice/mapruns-onvariants, group/labels handling, and empty/whitespace validation.buildConnectionString, covering stdio (with and without container), command/args, http, and unrecognized server types.Impact Assessment
pkg/cli.Commits