-
Notifications
You must be signed in to change notification settings - Fork 527
[purelock] Lock down extractExperimentVariantStubs, isValidToolName, validateObjectInput with pure-function test suites #52048
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.
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
Changes from all commits
475ce7c
472e5e3
6da4b0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| //go:build !integration | ||
|
|
||
| package cli | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/github/gh-aw/pkg/workflow" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestExtractExperimentVariantStubs_NoExperiments(t *testing.T) { | ||
| cfg := &workflow.FrontmatterConfig{} | ||
| stubs := extractExperimentVariantStubs(cfg) | ||
| assert.Empty(t, stubs) | ||
| } | ||
|
|
||
| func TestExtractExperimentVariantStubs_NilExperimentConfig(t *testing.T) { | ||
| cfg := &workflow.FrontmatterConfig{ | ||
| ExperimentConfigs: map[string]*workflow.ExperimentConfig{ | ||
| "exp1": nil, | ||
| }, | ||
| } | ||
| stubs := extractExperimentVariantStubs(cfg) | ||
| assert.Empty(t, stubs) | ||
| } | ||
|
|
||
| func TestExtractExperimentVariantStubs_SingleExperiment(t *testing.T) { | ||
| cfg := &workflow.FrontmatterConfig{ | ||
| ExperimentConfigs: map[string]*workflow.ExperimentConfig{ | ||
| "prompt_style": {Variants: []string{"concise", "verbose"}}, | ||
| }, | ||
| } | ||
| stubs := extractExperimentVariantStubs(cfg) | ||
| assert.Len(t, stubs, 2) | ||
| assert.Equal(t, "prompt_style", stubs[0].ExperimentName) | ||
| assert.Equal(t, "concise", stubs[0].Variant) | ||
| assert.Equal(t, "prompt_style", stubs[1].ExperimentName) | ||
| assert.Equal(t, "verbose", stubs[1].Variant) | ||
| } | ||
|
|
||
| func TestExtractExperimentVariantStubs_SortsByExperimentNameThenVariant(t *testing.T) { | ||
| cfg := &workflow.FrontmatterConfig{ | ||
| ExperimentConfigs: map[string]*workflow.ExperimentConfig{ | ||
| "zeta": {Variants: []string{"b", "a"}}, | ||
| "alpha": {Variants: []string{"y", "x"}}, | ||
| }, | ||
| } | ||
| stubs := extractExperimentVariantStubs(cfg) | ||
| assert.Len(t, stubs, 4) | ||
| // alpha comes before zeta | ||
| assert.Equal(t, "alpha", stubs[0].ExperimentName) | ||
| assert.Equal(t, "x", stubs[0].Variant) | ||
| assert.Equal(t, "alpha", stubs[1].ExperimentName) | ||
| assert.Equal(t, "y", stubs[1].Variant) | ||
| assert.Equal(t, "zeta", stubs[2].ExperimentName) | ||
| assert.Equal(t, "a", stubs[2].Variant) | ||
| assert.Equal(t, "zeta", stubs[3].ExperimentName) | ||
| assert.Equal(t, "b", stubs[3].Variant) | ||
| } | ||
|
|
||
| func TestExtractExperimentVariantStubs_MultipleExperimentsMixedNil(t *testing.T) { | ||
| cfg := &workflow.FrontmatterConfig{ | ||
| ExperimentConfigs: map[string]*workflow.ExperimentConfig{ | ||
| "exp1": {Variants: []string{"v1"}}, | ||
| "exp2": nil, | ||
| "exp3": {Variants: []string{}}, | ||
| }, | ||
| } | ||
| stubs := extractExperimentVariantStubs(cfg) | ||
| assert.Len(t, stubs, 1) | ||
| assert.Equal(t, "exp1", stubs[0].ExperimentName) | ||
| assert.Equal(t, "v1", stubs[0].Variant) | ||
| } | ||
|
|
||
| func TestExtractExperimentVariantStubs_EmptyVariants(t *testing.T) { | ||
| cfg := &workflow.FrontmatterConfig{ | ||
| ExperimentConfigs: map[string]*workflow.ExperimentConfig{ | ||
| "exp1": {Variants: []string{}}, | ||
| }, | ||
| } | ||
| assert.Empty(t, extractExperimentVariantStubs(cfg)) | ||
| } | ||
|
|
||
| func TestExtractExperimentVariantStubs_RunCountAndFractionDefaultZero(t *testing.T) { | ||
| cfg := &workflow.FrontmatterConfig{ | ||
| ExperimentConfigs: map[string]*workflow.ExperimentConfig{ | ||
| "exp1": {Variants: []string{"a"}}, | ||
| }, | ||
| } | ||
| stubs := extractExperimentVariantStubs(cfg) | ||
| assert.Len(t, stubs, 1) | ||
| assert.Equal(t, 0, stubs[0].RunCount) | ||
| assert.InDelta(t, 0.0, stubs[0].Fraction, 0) | ||
| } | ||
|
|
||
| func TestExtractExperimentVariantStubs_Pure(t *testing.T) { | ||
| cfg := &workflow.FrontmatterConfig{ | ||
| ExperimentConfigs: map[string]*workflow.ExperimentConfig{ | ||
| "exp1": {Variants: []string{"b", "a"}}, | ||
| }, | ||
| } | ||
| wantCfg := &workflow.FrontmatterConfig{ | ||
| ExperimentConfigs: map[string]*workflow.ExperimentConfig{ | ||
| "exp1": {Variants: []string{"b", "a"}}, | ||
| }, | ||
| } | ||
|
|
||
| first := extractExperimentVariantStubs(cfg) | ||
| second := extractExperimentVariantStubs(cfg) | ||
|
|
||
| assert.Equal(t, first, second) | ||
| assert.Equal(t, wantCfg, cfg) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| //go:build !integration | ||
|
|
||
| package cli | ||
|
|
||
| import ( | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestIsValidToolName(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| toolName string | ||
| want bool | ||
| }{ | ||
| {"empty string", "", false}, | ||
| {"whitespace only", " ", false}, | ||
| {"dash placeholder", "-", false}, | ||
| {"dash with spaces trimmed", " - ", false}, | ||
| {"single character", "a", false}, | ||
| {"single character uppercase", "X", false}, | ||
| {"stop word calls", "calls", false}, | ||
| {"stop word to", "to", false}, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The comment @copilot please address this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the mixed-case |
||
| {"stop word the", "the", false}, | ||
| {"stop word Testing (capitalized)", "Testing", false}, | ||
| {"stop word after trim", " calls ", false}, | ||
| {"short lowercase single word no separator", "abcdef", false}, | ||
| {"short lowercase single word exactly under length limit", "abcdefghi", false}, // len 9 < 10 | ||
| {"valid tool with underscore", "run_tests", true}, | ||
| {"valid tool with hyphen", "run-tests", true}, | ||
| {"valid camelCase tool", "runTests", true}, | ||
| {"valid mixed-case name at length limit", "abcdefghiJ", true}, | ||
| {"valid long all-lowercase single word", "abcdefghij", true}, // len 10, not < 10 | ||
| {"valid multi-word name", "a b", true}, | ||
| {"valid short name with underscore", "a_b", true}, | ||
| {"valid short name with hyphen", "a-b", true}, | ||
| {"github tool name", "github_search_code", true}, | ||
| {"bash tool name", "bash", false}, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got := isValidToolName(tt.toolName) | ||
| assert.Equal(t, tt.want, got, "isValidToolName(%q)", tt.toolName) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestIsValidToolName_AllStopWords(t *testing.T) { | ||
| for word := range toolNameStopWords { | ||
| t.Run("stopword_"+word, func(t *testing.T) { | ||
| assert.False(t, isValidToolName(word), "stop word %q should be invalid", word) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func FuzzIsValidToolName(f *testing.F) { | ||
| seeds := []string{"", "-", "a", "run_tests", "run-tests", "calls", "camelCase", " ", "abcdefghij"} | ||
| for _, s := range seeds { | ||
| f.Add(s) | ||
| } | ||
| f.Fuzz(func(t *testing.T, toolName string) { | ||
| // Must not panic, and must be deterministic (pure function). | ||
| got1 := isValidToolName(toolName) | ||
| got2 := isValidToolName(toolName) | ||
| if got1 != got2 { | ||
| t.Fatalf("isValidToolName(%q) not deterministic: %v != %v", toolName, got1, got2) | ||
| } | ||
| trimmed := strings.TrimSpace(toolName) | ||
| if trimmed == "" || trimmed == "-" { | ||
| if got1 { | ||
| t.Fatalf("expected false for empty/dash input %q", toolName) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| //go:build !integration | ||
|
|
||
| package parser | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestValidateObjectInput_NotAnObject(t *testing.T) { | ||
| err := validateObjectInput("config", "not-an-object", map[string]any{}, "org/repo/workflow.md") | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "must be an object") | ||
| assert.Contains(t, err.Error(), "config") | ||
| assert.Contains(t, err.Error(), "org/repo/workflow.md") | ||
| } | ||
|
|
||
| func TestValidateObjectInput_NoPropertiesDeclared_AcceptsAnyObject(t *testing.T) { | ||
| value := map[string]any{"anything": "goes", "num": 42} | ||
| err := validateObjectInput("config", value, map[string]any{}, "import") | ||
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| func TestValidateObjectInput_MalformedPropertiesField_FallsBackToPermissive(t *testing.T) { | ||
| paramDef := map[string]any{"properties": "not-a-map"} | ||
| value := map[string]any{"key": "value"} | ||
| err := validateObjectInput("config", value, paramDef, "import") | ||
| assert.NoError(t, err) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] @copilot please address this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed the case to |
||
| } | ||
|
|
||
| func TestValidateObjectInput_UnknownSubKey(t *testing.T) { | ||
| paramDef := map[string]any{ | ||
| "properties": map[string]any{ | ||
| "known": map[string]any{"type": "string"}, | ||
| }, | ||
| } | ||
| value := map[string]any{"unknown": "value"} | ||
| err := validateObjectInput("config", value, paramDef, "import") | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "unknown property") | ||
| assert.Contains(t, err.Error(), "unknown") | ||
| } | ||
|
|
||
| func TestValidateObjectInput_RequiredSubFieldMissing(t *testing.T) { | ||
| paramDef := map[string]any{ | ||
| "properties": map[string]any{ | ||
| "name": map[string]any{"required": true, "type": "string"}, | ||
| }, | ||
| } | ||
| value := map[string]any{} | ||
| err := validateObjectInput("config", value, paramDef, "import") | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "required property") | ||
| assert.Contains(t, err.Error(), "name") | ||
| } | ||
|
|
||
| func TestValidateObjectInput_RequiredSubFieldPresent(t *testing.T) { | ||
| paramDef := map[string]any{ | ||
| "properties": map[string]any{ | ||
| "name": map[string]any{"required": true, "type": "string"}, | ||
| }, | ||
| } | ||
| value := map[string]any{"name": "hello"} | ||
| err := validateObjectInput("config", value, paramDef, "import") | ||
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| func TestValidateObjectInput_OptionalSubFieldMissing_NoError(t *testing.T) { | ||
| paramDef := map[string]any{ | ||
| "properties": map[string]any{ | ||
| "optional": map[string]any{"type": "string"}, | ||
| }, | ||
| } | ||
| value := map[string]any{} | ||
| err := validateObjectInput("config", value, paramDef, "import") | ||
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| func TestValidateObjectInput_NoTypeDeclared_SkipsTypeValidation(t *testing.T) { | ||
| paramDef := map[string]any{ | ||
| "properties": map[string]any{ | ||
| "field": map[string]any{}, | ||
| }, | ||
| } | ||
| value := map[string]any{"field": 12345} // any type accepted since no "type" declared | ||
| err := validateObjectInput("config", value, paramDef, "import") | ||
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| func TestValidateObjectInput_TypeMismatch(t *testing.T) { | ||
| paramDef := map[string]any{ | ||
| "properties": map[string]any{ | ||
| "count": map[string]any{"type": "number"}, | ||
| }, | ||
| } | ||
| value := map[string]any{"count": "not-a-number"} | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] @copilot please address this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Converted the type-match checks to independently named table-driven subtests in 472e5e3. |
||
| err := validateObjectInput("config", value, paramDef, "import") | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "config.count") | ||
| } | ||
|
|
||
| func TestValidateObjectInput_TypeMatch(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| field string | ||
| typeName string | ||
| value any | ||
| }{ | ||
| {name: "number", field: "count", typeName: "number", value: 3}, | ||
| {name: "string", field: "label", typeName: "string", value: "hi"}, | ||
| {name: "boolean", field: "flag", typeName: "boolean", value: true}, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| paramDef := map[string]any{ | ||
| "properties": map[string]any{ | ||
| tt.field: map[string]any{"type": tt.typeName}, | ||
| }, | ||
| } | ||
| value := map[string]any{tt.field: tt.value} | ||
| assert.NoError(t, validateObjectInput("config", value, paramDef, "import")) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestValidateObjectInput_PropDefNotAMap_SkipsValidation(t *testing.T) { | ||
| paramDef := map[string]any{ | ||
| "properties": map[string]any{ | ||
| "weird": "not-a-map", | ||
| }, | ||
| } | ||
| value := map[string]any{"weird": "value"} | ||
| err := validateObjectInput("config", value, paramDef, "import") | ||
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| func TestValidateObjectInput_QualifiedNameInErrorMessage(t *testing.T) { | ||
| paramDef := map[string]any{ | ||
| "properties": map[string]any{ | ||
| "nested": map[string]any{"type": "choice", "options": []any{"a", "b"}}, | ||
| }, | ||
| } | ||
| value := map[string]any{"nested": "c"} | ||
| err := validateObjectInput("parentField", value, paramDef, "import/path") | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "parentField.nested") | ||
| } | ||
|
|
||
| func TestValidateObjectInput_Pure(t *testing.T) { | ||
| paramDef := map[string]any{ | ||
| "properties": map[string]any{ | ||
| "known": map[string]any{"type": "string"}, | ||
| }, | ||
| } | ||
| value := map[string]any{"unknown": "value"} | ||
| wantParamDef := map[string]any{ | ||
| "properties": map[string]any{ | ||
| "known": map[string]any{"type": "string"}, | ||
| }, | ||
| } | ||
| wantValue := map[string]any{"unknown": "value"} | ||
|
|
||
| first := validateObjectInput("config", value, paramDef, "import") | ||
| second := validateObjectInput("config", value, paramDef, "import") | ||
|
|
||
| require.Error(t, first) | ||
| require.EqualError(t, second, first.Error()) | ||
| assert.Equal(t, wantParamDef, paramDef) | ||
| assert.Equal(t, wantValue, value) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] No test for an experiment with an empty
Variantsslice —TestExtractExperimentVariantStubs_MultipleExperimentsMixedNilcovers a nil config butexp3uses{Variants: []string{}}. The test assertsLen(t, stubs, 1)which implicitly validates the empty-variants case, but there is no standalone test that names this contract. A dedicated_EmptyVariantscase makes the specification explicit and easier to maintain.@copilot please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a dedicated
TestExtractExperimentVariantStubs_EmptyVariantscase in 472e5e3.