Repository navigation
[purelock] Lock down firstForbiddenCharInParamValue, validateGCPWIFEngineAuth, hasPullRequestTargetTrigger with pure-function test suites - #52890
Conversation
Locks down firstForbiddenCharInParamValue, validateGCPWIFEngineAuth, and hasPullRequestTargetTrigger with table-driven testify suites. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Great work on the PureLock test suite additions! This PR adds comprehensive test coverage for three pure functions with detailed specifications for each:
The changes are focused (test files only), well-documented, and pass validation ( This PR is ready for review and appears aligned with the project's testing and quality standards.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail review: PR only adds table-driven testify test files with no abstractions, dependencies, or dead code to cut. Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Reviewed PR #52890 locally; no actionable review comments because the changes only add focused table-driven tests for existing pure functions and I found no correctness, maintainability, or coverage regressions in the added test code.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check.
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pure-function test suites look correct and well-structured. Good coverage of nil/edge cases, whitespace handling, and partial-field combinations. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 14 AIC · ⌖ 9.83 AIC · ⊞ 5.6K
ADR Gate: Verified ✓Status: Implementation aligns with ADR-52295. Evidence of alignment
Minor scope note: ADR-52295's Decision section references No action required from the author. This PR may merge when other checks pass.
|
There was a problem hiding this comment.
Pull request overview
Adds focused table-driven tests that lock down three pure helper and validation functions.
Changes:
- Tests allowed and forbidden model-parameter characters.
- Tests GCP WIF authentication validation branches and errors.
- Tests supported
pull_request_targetfrontmatter representations.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/model_identifier_param_value_test.go |
Covers parameter-value character validation. |
pkg/workflow/compiler_validators_gcp_wif_test.go |
Covers GCP WIF authentication validation. |
pkg/cli/codemod_pull_request_target_trigger_test.go |
Covers trigger detection across frontmatter shapes. |
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 — changes are test-only additions. Overall solid; two minor observations flagged as inline comments.
📋 Key Themes & Highlights
Positive Highlights
- ✅ Comprehensive table-driven tests with parallel subtests on all three functions
- ✅ Clear Arrange/Act/Assert structure; test names read as specifications
- ✅ Good boundary and nil-path coverage (nil workflow, nil engine config, nil auth)
- ✅ Whitespace-trimming and non-string slice-entry cases for
hasPullRequestTargetTrigger - ✅ Each missing-field combination covered for
validateGCPWIFEngineAuth
Minor Issues (inline)
tt := ttin Go 1.26.5 — dead code; loop-variable capture has been automatic since Go 1.22. Appears in all three test files.- Hard-coded error message prefix in
compiler_validators_gcp_wif_test.go— ties tests to phrasing rather than behaviour.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 27.2 AIC · ⌖ 8.03 AIC · ⊞ 7.7K
Comment /matt to run again
| } | ||
|
|
||
| for _, tt := range tests { | ||
| tt := tt |
There was a problem hiding this comment.
[/tdd] tt := tt is unnecessary in Go 1.22+ (this module uses Go 1.26.5) — loop-variable capture is automatic.
Removing it keeps the test idiomatic and avoids confusion for future contributors.
💡 Suggested change
Delete the tt := tt line in all three test files. The parallel subtests will still work correctly.
@copilot please address this.
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), tt.errContains) | ||
| assert.Contains(t, err.Error(), "engine.auth with provider=gcp requires the following fields") | ||
| } else { |
There was a problem hiding this comment.
[/tdd] The errContains assertion hard-codes the exact error message format ("engine.auth with provider=gcp requires the following fields"). If the production error message is ever reworded, this test silently regresses.
Consider extracting the expected prefix to a package-level constant or using a sentinel error type so tests remain coupled to behaviour, not phrasing.
💡 Suggested approach
If validateGCPWIFEngineAuth returns a typed error, assert errors.As rather than Contains. Alternatively, define:
const gcpWIFErrPrefix = "engine.auth with provider=gcp requires the following fields"and reference it in both production code and tests.
@copilot please address this.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in |
|
🎉 This pull request is included in a new release. Release: |
Summary
This PR adds three new pure-function unit test files that exercise
hasPullRequestTargetTrigger(pkg/cli),validateGCPWIFEngineAuth(pkg/workflow), andfirstForbiddenCharInParamValue(pkg/workflow). Each suite uses table-driven tests witht.Parallel()to cover edge cases (nil inputs, empty collections, type variants, whitespace handling, and forbidden-character detection) that were previously under-tested. A follow-up commit removes redundant loop variable copies introduced during the initial test authoring.Change Classification
Key Changes
pkg/cli/codemod_pull_request_target_trigger_test.gohasPullRequestTargetTrigger, covering nil/emptyonkey,map[string]any,[]any,[]string, and plain string forms with whitespace and type-mismatch casespkg/workflow/compiler_validators_gcp_wif_test.govalidateGCPWIFEngineAuth, covering nil workflow/engine/auth config, non-GCP/non-oidc auth, complete config, and each combination of missing required GCP WIF fieldspkg/workflow/model_identifier_param_value_test.gofirstForbiddenCharInParamValue, covering allowed characters (letters, digits,_,.,-), forbidden characters (space,/,?, unicode), and first-occurrence behaviorImpact Assessment
Commits