Skip to content

[purelock] Lock down firstForbiddenCharInParamValue, validateGCPWIFEngineAuth, hasPullRequestTargetTrigger with pure-function test suites #52890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 2 commits into from
Aug 15, 2026
Merged

[purelock] Lock down firstForbiddenCharInParamValue, validateGCPWIFEngineAuth, hasPullRequestTargetTrigger with pure-function test suites #52890

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60ea272
test: add pure-function test suites for 3 low-coverage functions
github-actions[bot] Aug 15, 2026
a23c80a
test: remove redundant loop variable copies
Copilot Aug 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 112 additions & 0 deletions pkg/cli/codemod_pull_request_target_trigger_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
package cli

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestHasPullRequestTargetTrigger(t *testing.T) {
t.Parallel()

tests := []struct {
name string
frontmatter map[string]any
want bool
}{
{
name: "no on key",
frontmatter: map[string]any{},
want: false,
},
{
name: "on key nil map",
frontmatter: map[string]any{"on": nil},
want: false,
},
{
name: "on map with pull_request_target",
frontmatter: map[string]any{"on": map[string]any{"pull_request_target": nil}},
want: true,
},
{
name: "on map without pull_request_target",
frontmatter: map[string]any{"on": map[string]any{"push": nil}},
want: false,
},
{
name: "on map empty",
frontmatter: map[string]any{"on": map[string]any{}},
want: false,
},
{
name: "on []any with pull_request_target string",
frontmatter: map[string]any{"on": []any{"push", "pull_request_target"}},
want: true,
},
{
name: "on []any with pull_request_target and whitespace",
frontmatter: map[string]any{"on": []any{" pull_request_target "}},
want: true,
},
{
name: "on []any without pull_request_target",
frontmatter: map[string]any{"on": []any{"push", "issues"}},
want: false,
},
{
name: "on []any with non-string entries",
frontmatter: map[string]any{"on": []any{123, true, nil}},
want: false,
},
{
name: "on []any empty",
frontmatter: map[string]any{"on": []any{}},
want: false,
},
{
name: "on []string with pull_request_target",
frontmatter: map[string]any{"on": []string{"push", "pull_request_target"}},
want: true,
},
{
name: "on []string without pull_request_target",
frontmatter: map[string]any{"on": []string{"push"}},
want: false,
},
{
name: "on []string with whitespace around match",
frontmatter: map[string]any{"on": []string{" pull_request_target "}},
want: true,
},
{
name: "on string exact match",
frontmatter: map[string]any{"on": "pull_request_target"},
want: true,
},
{
name: "on string with surrounding whitespace",
frontmatter: map[string]any{"on": " pull_request_target "},
want: true,
},
{
name: "on string non-matching",
frontmatter: map[string]any{"on": "push"},
want: false,
},
{
name: "on unsupported type (int)",
frontmatter: map[string]any{"on": 42},
want: false,
},
}

for _, tt := range tests {
tt := tt

Check failure on line 105 in pkg/cli/codemod_pull_request_target_trigger_test.go

View workflow job for this annotation

GitHub Actions / lint-go 

The copy of the 'for' variable "tt" can be deleted (Go 1.22+) (copyloopvar)

Check failure on line 105 in pkg/cli/codemod_pull_request_target_trigger_test.go

View workflow job for this annotation

GitHub Actions / lint-go 

The copy of the 'for' variable "tt" can be deleted (Go 1.22+) (copyloopvar)
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
got := hasPullRequestTargetTrigger(tt.frontmatter)
assert.Equal(t, tt.want, got, "hasPullRequestTargetTrigger(%#v)", tt.frontmatter)
})
}
}
121 changes: 121 additions & 0 deletions pkg/workflow/compiler_validators_gcp_wif_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
package workflow

import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestValidateGCPWIFEngineAuth(t *testing.T) {
t.Parallel()

completeGCPAuth := &EngineAuthConfig{
Type: "github-oidc",
Provider: "gcp",
GoogleWorkloadIdentityProvider: "projects/123/locations/global/workloadIdentityPools/pool/providers/provider",
GoogleServiceAccount: "sa@project.iam.gserviceaccount.com",
GoogleProject: "my-project",
}

tests := []struct {
name string
workflow *WorkflowData
wantErr bool
errContains string
}{
{
name: "nil workflow data",
workflow: nil,
wantErr: false,
},
{
name: "nil engine config",
workflow: &WorkflowData{EngineConfig: nil},
wantErr: false,
},
{
name: "nil auth config",
workflow: &WorkflowData{EngineConfig: &EngineConfig{Auth: nil}},
wantErr: false,
},
{
name: "auth type not github-oidc",
workflow: &WorkflowData{EngineConfig: &EngineConfig{Auth: &EngineAuthConfig{
Type: "other",
Provider: "gcp",
}}},
wantErr: false,
},
{
name: "auth provider not gcp",
workflow: &WorkflowData{EngineConfig: &EngineConfig{Auth: &EngineAuthConfig{
Type: "github-oidc",
Provider: "azure",
}}},
wantErr: false,
},
{
name: "complete gcp auth",
workflow: &WorkflowData{EngineConfig: &EngineConfig{Auth: completeGCPAuth}},
wantErr: false,
},
{
name: "missing workload-identity-provider only",
workflow: &WorkflowData{EngineConfig: &EngineConfig{Auth: &EngineAuthConfig{
Type: "github-oidc",
Provider: "gcp",
GoogleServiceAccount: "sa@project.iam.gserviceaccount.com",
GoogleProject: "my-project",
}}},
wantErr: true,
errContains: "workload-identity-provider",
},
{
name: "missing service-account only",
workflow: &WorkflowData{EngineConfig: &EngineConfig{Auth: &EngineAuthConfig{
Type: "github-oidc",
Provider: "gcp",
GoogleWorkloadIdentityProvider: "provider",
GoogleProject: "my-project",
}}},
wantErr: true,
errContains: "service-account",
},
{
name: "missing project only",
workflow: &WorkflowData{EngineConfig: &EngineConfig{Auth: &EngineAuthConfig{
Type: "github-oidc",
Provider: "gcp",
GoogleWorkloadIdentityProvider: "provider",
GoogleServiceAccount: "sa@project.iam.gserviceaccount.com",
}}},
wantErr: true,
errContains: "project",
},
{
name: "missing all three fields",
workflow: &WorkflowData{EngineConfig: &EngineConfig{Auth: &EngineAuthConfig{
Type: "github-oidc",
Provider: "gcp",
}}},
wantErr: true,
errContains: "workload-identity-provider, service-account, project",
},
}

for _, tt := range tests {
tt := tt

Check failure on line 108 in pkg/workflow/compiler_validators_gcp_wif_test.go

View workflow job for this annotation

GitHub Actions / lint-go 

The copy of the 'for' variable "tt" can be deleted (Go 1.22+) (copyloopvar)

Check failure on line 108 in pkg/workflow/compiler_validators_gcp_wif_test.go

View workflow job for this annotation

GitHub Actions / lint-go 

The copy of the 'for' variable "tt" can be deleted (Go 1.22+) (copyloopvar)
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
err := validateGCPWIFEngineAuth(tt.workflow)
if tt.wantErr {
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 {

@github-actions github-actions Bot Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/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.

require.NoError(t, err)
}
})
}
}
42 changes: 42 additions & 0 deletions pkg/workflow/model_identifier_param_value_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
package workflow

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestFirstForbiddenCharInParamValue(t *testing.T) {
t.Parallel()

tests := []struct {
name string
in string
want rune
}{
{name: "empty string", in: "", want: 0},
{name: "all lowercase letters", in: "abcxyz", want: 0},
{name: "all uppercase letters", in: "ABCXYZ", want: 0},
{name: "all digits", in: "0123456789", want: 0},
{name: "underscore allowed", in: "a_b", want: 0},
{name: "dot allowed", in: "a.b", want: 0},
{name: "hyphen allowed", in: "a-b", want: 0},
{name: "mixed allowed chars", in: "gpt-4o_v1.2", want: 0},
{name: "space forbidden", in: "a b", want: ' '},
{name: "slash forbidden as first char", in: "/abc", want: '/'},
{name: "slash forbidden mid string", in: "abc/def", want: '/'},
{name: "question mark forbidden", in: "abc?def", want: '?'},
{name: "unicode letter forbidden", in: "abcé", want: 'é'},
{name: "forbidden char not at start returns first occurrence", in: "ab!cd!ef", want: '!'},
{name: "only forbidden char", in: "!", want: '!'},
}

for _, tt := range tests {
tt := tt

Check failure on line 35 in pkg/workflow/model_identifier_param_value_test.go

View workflow job for this annotation

GitHub Actions / lint-go 

The copy of the 'for' variable "tt" can be deleted (Go 1.22+) (copyloopvar)

Check failure on line 35 in pkg/workflow/model_identifier_param_value_test.go

View workflow job for this annotation

GitHub Actions / lint-go 

The copy of the 'for' variable "tt" can be deleted (Go 1.22+) (copyloopvar)

@github-actions github-actions Bot Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/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.

t.Run(tt.name, func(t *testing.T) {
t.Parallel()
got := firstForbiddenCharInParamValue(tt.in)
assert.Equal(t, tt.want, got, "firstForbiddenCharInParamValue(%q)", tt.in)
})
}
}
Loading