From 3d44efcffdac9aa327c747f96574db849037cc72 Mon Sep 17 00:00:00 2001 From: Stanislav Jakuschevskij Date: Tue, 22 Apr 2025 19:30:44 +0200 Subject: [PATCH] refactor: move Step onError and API version tests Now that `Step` implements the `Validatable` interface the tests for the `Step` validation are moved from `task_validation_test.go` to `container_validation_test.go`. `TestStepOnError` is moved. `TestIncompatibleAPIVersions` is broken down and the Step fields related tests i.e. "windows script support", "stdout stream support" and "stderr stream support" are moved to a new test `TestStepIncompatibleAPIVersions`. Issue #8700. Signed-off-by: Stanislav Jakuschevskij --- .../pipeline/v1/container_validation_test.go | 142 ++++++++++++++++++ pkg/apis/pipeline/v1/task_validation_test.go | 99 ------------ 2 files changed, 142 insertions(+), 99 deletions(-) diff --git a/pkg/apis/pipeline/v1/container_validation_test.go b/pkg/apis/pipeline/v1/container_validation_test.go index e14601bbe0e..4e97bfd1b7f 100644 --- a/pkg/apis/pipeline/v1/container_validation_test.go +++ b/pkg/apis/pipeline/v1/container_validation_test.go @@ -647,6 +647,148 @@ func TestStepValidateErrorWithStepActionRef(t *testing.T) { } } +func TestStepOnError(t *testing.T) { + tests := []struct { + name string + params []v1.ParamSpec + step v1.Step + expectedError *apis.FieldError + }{{ + name: "valid step - valid onError usage - set to continue", + step: v1.Step{ + OnError: v1.Continue, + Image: "image", + Args: []string{"arg"}, + }, + }, { + name: "valid step - valid onError usage - set to stopAndFail", + step: v1.Step{ + OnError: v1.StopAndFail, + Image: "image", + Args: []string{"arg"}, + }, + }, { + name: "valid step - valid onError usage - set to a task parameter", + params: []v1.ParamSpec{{ + Name: "CONTINUE", + Default: &v1.ParamValue{Type: v1.ParamTypeString, StringVal: string(v1.Continue)}, + }}, + step: v1.Step{ + OnError: "$(params.CONTINUE)", + Image: "image", + Args: []string{"arg"}, + }, + }, { + name: "invalid step - onError set to invalid value", + step: v1.Step{ + OnError: "onError", + Image: "image", + Args: []string{"arg"}, + }, + expectedError: &apis.FieldError{ + Message: `invalid value: "onError"`, + Paths: []string{"onError"}, + Details: `Task step onError must be either "continue" or "stopAndFail"`, + }, + }} + for _, st := range tests { + t.Run(st.name, func(t *testing.T) { + ctx := t.Context() + err := st.step.Validate(ctx) + if st.expectedError == nil && err != nil { + t.Errorf("No error expected from Step.Validate() but got = %v", err) + } else if st.expectedError != nil { + if err == nil { + t.Errorf("Expected error from Step.Validate() = %v, but got none", st.expectedError) + } else if d := cmp.Diff(st.expectedError.Error(), err.Error()); d != "" { + t.Errorf("returned error from Step.Validate() does not match with the expected error: %s", diff.PrintWantGot(d)) + } + } + }) + } +} + +// TestStepIncompatibleAPIVersions exercises validation of fields in a Step +// that require a specific feature gate version in order to work. +func TestStepIncompatibleAPIVersions(t *testing.T) { + versions := []string{"alpha", "beta", "stable"} + isStricterThen := func(first, second string) bool { + // assume values are in order alpha (less strict), beta, stable (strictest) + // return true if first is stricter then second + switch first { + case second, "alpha": + return false + case "stable": + return true + default: + // first is beta, true is second is alpha, false is second is stable + return second == "alpha" + } + } + + for _, st := range []struct { + name string + requiredVersion string + step v1.Step + }{ + { + name: "windows script support requires alpha", + requiredVersion: "alpha", + step: v1.Step{ + Image: "my-image", + Script: ` + #!win powershell -File + script-1`, + }, + }, { + name: "stdout stream support requires alpha", + requiredVersion: "alpha", + step: v1.Step{ + Image: "foo", + StdoutConfig: &v1.StepOutputConfig{ + Path: "/tmp/stdout.txt", + }, + }, + }, { + name: "stderr stream support requires alpha", + requiredVersion: "alpha", + step: v1.Step{ + Image: "foo", + StderrConfig: &v1.StepOutputConfig{ + Path: "/tmp/stderr.txt", + }, + }, + }, + } { + for _, version := range versions { + testName := fmt.Sprintf("(using %s) %s", version, st.name) + t.Run(testName, func(t *testing.T) { + ctx := t.Context() + if version == "alpha" { + ctx = cfgtesting.EnableAlphaAPIFields(ctx) + } + if version == "beta" { + ctx = cfgtesting.EnableBetaAPIFields(ctx) + } + if version == "stable" { + ctx = cfgtesting.EnableStableAPIFields(ctx) + } + err := st.step.Validate(ctx) + + // If the configured version is stricter than the required one, we expect an error + if isStricterThen(version, st.requiredVersion) && err == nil { + t.Fatalf("no error received even though version required is %q while feature gate is %q", st.requiredVersion, version) + } + + // If the configured version is more permissive than the required one, we expect no error + if isStricterThen(st.requiredVersion, version) && err != nil { + t.Fatalf("error received despite required version and feature gate matching %q: %v", version, err) + } + }) + } + } +} + func TestSidecarValidate(t *testing.T) { tests := []struct { name string diff --git a/pkg/apis/pipeline/v1/task_validation_test.go b/pkg/apis/pipeline/v1/task_validation_test.go index a6c68d199f4..012544f1e05 100644 --- a/pkg/apis/pipeline/v1/task_validation_test.go +++ b/pkg/apis/pipeline/v1/task_validation_test.go @@ -1670,72 +1670,6 @@ func TestStepAndSidecarWorkspacesErrors(t *testing.T) { } } -func TestStepOnError(t *testing.T) { - tests := []struct { - name string - params []v1.ParamSpec - steps []v1.Step - expectedError *apis.FieldError - }{{ - name: "valid step - valid onError usage - set to continue", - steps: []v1.Step{{ - OnError: v1.Continue, - Image: "image", - Args: []string{"arg"}, - }}, - }, { - name: "valid step - valid onError usage - set to stopAndFail", - steps: []v1.Step{{ - OnError: v1.StopAndFail, - Image: "image", - Args: []string{"arg"}, - }}, - }, { - name: "valid step - valid onError usage - set to a task parameter", - params: []v1.ParamSpec{{ - Name: "CONTINUE", - Default: &v1.ParamValue{Type: v1.ParamTypeString, StringVal: string(v1.Continue)}, - }}, - steps: []v1.Step{{ - OnError: "$(params.CONTINUE)", - Image: "image", - Args: []string{"arg"}, - }}, - }, { - name: "invalid step - onError set to invalid value", - steps: []v1.Step{{ - OnError: "onError", - Image: "image", - Args: []string{"arg"}, - }}, - expectedError: &apis.FieldError{ - Message: `invalid value: "onError"`, - Paths: []string{"steps[0].onError"}, - Details: `Task step onError must be either "continue" or "stopAndFail"`, - }, - }} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - ts := &v1.TaskSpec{ - Params: tt.params, - Steps: tt.steps, - } - ctx := t.Context() - ts.SetDefaults(ctx) - err := ts.Validate(ctx) - if tt.expectedError == nil && err != nil { - t.Errorf("No error expected from TaskSpec.Validate() but got = %v", err) - } else if tt.expectedError != nil { - if err == nil { - t.Errorf("Expected error from TaskSpec.Validate() = %v, but got none", tt.expectedError) - } else if d := cmp.Diff(tt.expectedError.Error(), err.Error()); d != "" { - t.Errorf("returned error from TaskSpec.Validate() does not match with the expected error: %s", diff.PrintWantGot(d)) - } - } - }) - } -} - // TestIncompatibleAPIVersions exercises validation of fields that // require a specific feature gate version in order to work. func TestIncompatibleAPIVersions(t *testing.T) { @@ -1790,39 +1724,6 @@ func TestIncompatibleAPIVersions(t *testing.T) { }}, }}, }, - }, { - name: "windows script support requires alpha", - requiredVersion: "alpha", - spec: v1.TaskSpec{ - Steps: []v1.Step{{ - Image: "my-image", - Script: ` - #!win powershell -File - script-1`, - }}, - }, - }, { - name: "stdout stream support requires alpha", - requiredVersion: "alpha", - spec: v1.TaskSpec{ - Steps: []v1.Step{{ - Image: "foo", - StdoutConfig: &v1.StepOutputConfig{ - Path: "/tmp/stdout.txt", - }, - }}, - }, - }, { - name: "stderr stream support requires alpha", - requiredVersion: "alpha", - spec: v1.TaskSpec{ - Steps: []v1.Step{{ - Image: "foo", - StderrConfig: &v1.StepOutputConfig{ - Path: "/tmp/stderr.txt", - }, - }}, - }, }, } { for _, version := range versions {