From 475ce7c7c5221a17bcdc84912e4711e10592e5b5 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 11 Aug 2026 12:55:09 +0000 Subject: [PATCH 1/2] test: add pure-function test suites for extractExperimentVariantStubs, isValidToolName, validateObjectInput Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...a_extract_experiment_variant_stubs_test.go | 84 +++++++++++ .../logs_report_tools_isvalidtoolname_test.go | 82 +++++++++++ ...ort_schema_validation_object_input_test.go | 135 ++++++++++++++++++ 3 files changed, 301 insertions(+) create mode 100644 pkg/cli/forecast_metadata_extract_experiment_variant_stubs_test.go create mode 100644 pkg/cli/logs_report_tools_isvalidtoolname_test.go create mode 100644 pkg/parser/import_schema_validation_object_input_test.go diff --git a/pkg/cli/forecast_metadata_extract_experiment_variant_stubs_test.go b/pkg/cli/forecast_metadata_extract_experiment_variant_stubs_test.go new file mode 100644 index 00000000000..6a113b0ddfd --- /dev/null +++ b/pkg/cli/forecast_metadata_extract_experiment_variant_stubs_test.go @@ -0,0 +1,84 @@ +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.Nil(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_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.Equal(t, 0.0, stubs[0].Fraction) +} diff --git a/pkg/cli/logs_report_tools_isvalidtoolname_test.go b/pkg/cli/logs_report_tools_isvalidtoolname_test.go new file mode 100644 index 00000000000..3bfa24cbb7c --- /dev/null +++ b/pkg/cli/logs_report_tools_isvalidtoolname_test.go @@ -0,0 +1,82 @@ +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}, + {"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 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 TestIsValidToolName_Idempotent(t *testing.T) { + // Pure function property: calling twice with the same input yields the same result. + inputs := []string{"", "-", "a", "run_tests", "calls", " spaced ", "camelCase"} + for _, in := range inputs { + assert.Equal(t, isValidToolName(in), isValidToolName(in)) + } +} + +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) + } + } + }) +} diff --git a/pkg/parser/import_schema_validation_object_input_test.go b/pkg/parser/import_schema_validation_object_input_test.go new file mode 100644 index 00000000000..8fddb0f68a3 --- /dev/null +++ b/pkg/parser/import_schema_validation_object_input_test.go @@ -0,0 +1,135 @@ +package parser + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestValidateObjectInput_NotAnObject(t *testing.T) { + err := validateObjectInput("config", "not-an-object", map[string]any{}, "org/repo/workflow.md") + assert.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_PropertiesNotAMap_AcceptsAnyObject(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) +} + +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") + assert.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") + assert.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"} + err := validateObjectInput("config", value, paramDef, "import") + assert.Error(t, err) + assert.Contains(t, err.Error(), "config.count") +} + +func TestValidateObjectInput_TypeMatch(t *testing.T) { + paramDef := map[string]any{ + "properties": map[string]any{ + "count": map[string]any{"type": "number"}, + "label": map[string]any{"type": "string"}, + "flag": map[string]any{"type": "boolean"}, + }, + } + value := map[string]any{"count": 3, "label": "hi", "flag": true} + err := validateObjectInput("config", value, paramDef, "import") + assert.NoError(t, err) +} + +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") + assert.Error(t, err) + assert.Contains(t, err.Error(), "parentField.nested") +} From 472e5e35252d1ced9d3fc3e698c45c6da46f743d Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 11 Aug 2026 16:10:36 +0000 Subject: [PATCH 2/2] test: address pure-function suite review feedback Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- ...a_extract_experiment_variant_stubs_test.go | 34 +++++++++- .../logs_report_tools_isvalidtoolname_test.go | 11 +-- ...ort_schema_validation_object_input_test.go | 67 ++++++++++++++----- 3 files changed, 87 insertions(+), 25 deletions(-) diff --git a/pkg/cli/forecast_metadata_extract_experiment_variant_stubs_test.go b/pkg/cli/forecast_metadata_extract_experiment_variant_stubs_test.go index 6a113b0ddfd..d0f940bc984 100644 --- a/pkg/cli/forecast_metadata_extract_experiment_variant_stubs_test.go +++ b/pkg/cli/forecast_metadata_extract_experiment_variant_stubs_test.go @@ -1,3 +1,5 @@ +//go:build !integration + package cli import ( @@ -10,7 +12,7 @@ import ( func TestExtractExperimentVariantStubs_NoExperiments(t *testing.T) { cfg := &workflow.FrontmatterConfig{} stubs := extractExperimentVariantStubs(cfg) - assert.Nil(t, stubs) + assert.Empty(t, stubs) } func TestExtractExperimentVariantStubs_NilExperimentConfig(t *testing.T) { @@ -71,6 +73,15 @@ func TestExtractExperimentVariantStubs_MultipleExperimentsMixedNil(t *testing.T) 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{ @@ -80,5 +91,24 @@ func TestExtractExperimentVariantStubs_RunCountAndFractionDefaultZero(t *testing stubs := extractExperimentVariantStubs(cfg) assert.Len(t, stubs, 1) assert.Equal(t, 0, stubs[0].RunCount) - assert.Equal(t, 0.0, stubs[0].Fraction) + 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) } diff --git a/pkg/cli/logs_report_tools_isvalidtoolname_test.go b/pkg/cli/logs_report_tools_isvalidtoolname_test.go index 3bfa24cbb7c..d6d7ab1e9d9 100644 --- a/pkg/cli/logs_report_tools_isvalidtoolname_test.go +++ b/pkg/cli/logs_report_tools_isvalidtoolname_test.go @@ -1,3 +1,5 @@ +//go:build !integration + package cli import ( @@ -29,6 +31,7 @@ func TestIsValidToolName(t *testing.T) { {"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}, @@ -52,14 +55,6 @@ func TestIsValidToolName_AllStopWords(t *testing.T) { } } -func TestIsValidToolName_Idempotent(t *testing.T) { - // Pure function property: calling twice with the same input yields the same result. - inputs := []string{"", "-", "a", "run_tests", "calls", " spaced ", "camelCase"} - for _, in := range inputs { - assert.Equal(t, isValidToolName(in), isValidToolName(in)) - } -} - func FuzzIsValidToolName(f *testing.F) { seeds := []string{"", "-", "a", "run_tests", "run-tests", "calls", "camelCase", " ", "abcdefghij"} for _, s := range seeds { diff --git a/pkg/parser/import_schema_validation_object_input_test.go b/pkg/parser/import_schema_validation_object_input_test.go index 8fddb0f68a3..316dfdc9c35 100644 --- a/pkg/parser/import_schema_validation_object_input_test.go +++ b/pkg/parser/import_schema_validation_object_input_test.go @@ -1,14 +1,17 @@ +//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") - assert.Error(t, err) + 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") @@ -20,7 +23,7 @@ func TestValidateObjectInput_NoPropertiesDeclared_AcceptsAnyObject(t *testing.T) assert.NoError(t, err) } -func TestValidateObjectInput_PropertiesNotAMap_AcceptsAnyObject(t *testing.T) { +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") @@ -35,7 +38,7 @@ func TestValidateObjectInput_UnknownSubKey(t *testing.T) { } value := map[string]any{"unknown": "value"} err := validateObjectInput("config", value, paramDef, "import") - assert.Error(t, err) + require.Error(t, err) assert.Contains(t, err.Error(), "unknown property") assert.Contains(t, err.Error(), "unknown") } @@ -48,7 +51,7 @@ func TestValidateObjectInput_RequiredSubFieldMissing(t *testing.T) { } value := map[string]any{} err := validateObjectInput("config", value, paramDef, "import") - assert.Error(t, err) + require.Error(t, err) assert.Contains(t, err.Error(), "required property") assert.Contains(t, err.Error(), "name") } @@ -94,21 +97,32 @@ func TestValidateObjectInput_TypeMismatch(t *testing.T) { } value := map[string]any{"count": "not-a-number"} err := validateObjectInput("config", value, paramDef, "import") - assert.Error(t, err) + require.Error(t, err) assert.Contains(t, err.Error(), "config.count") } func TestValidateObjectInput_TypeMatch(t *testing.T) { - paramDef := map[string]any{ - "properties": map[string]any{ - "count": map[string]any{"type": "number"}, - "label": map[string]any{"type": "string"}, - "flag": map[string]any{"type": "boolean"}, - }, + 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")) + }) } - value := map[string]any{"count": 3, "label": "hi", "flag": true} - err := validateObjectInput("config", value, paramDef, "import") - assert.NoError(t, err) } func TestValidateObjectInput_PropDefNotAMap_SkipsValidation(t *testing.T) { @@ -130,6 +144,29 @@ func TestValidateObjectInput_QualifiedNameInErrorMessage(t *testing.T) { } value := map[string]any{"nested": "c"} err := validateObjectInput("parentField", value, paramDef, "import/path") - assert.Error(t, err) + 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) +}