From 99ba54ba8dab650d8fd7f0a8bd8adfeb76b2ff77 Mon Sep 17 00:00:00 2001 From: Eli Zucker Date: Fri, 12 Jul 2019 15:04:16 -0400 Subject: [PATCH 1/2] Actually call PipelineSpec.Validate(). Call PipelineSpec.Validate() within Pipeline.Validate(), and add tests for Pipeline.Validate(). Previously, none of the PipelineSpec validation logic was actually being called, but the unit tests passed because they manually called PipelineSpec.Validate(). --- .../pipeline/v1alpha1/pipeline_validation.go | 2 +- .../v1alpha1/pipeline_validation_test.go | 67 +++++++++++++++++++ 2 files changed, 68 insertions(+), 1 deletion(-) diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go index ce6f6171d8b..2f9ad98b821 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_validation.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation.go @@ -33,7 +33,7 @@ func (p *Pipeline) Validate(ctx context.Context) *apis.FieldError { if err := validateObjectMetadata(p.GetObjectMeta()); err != nil { return err.ViaField("metadata") } - return nil + return p.Spec.Validate(ctx) } func validateDeclaredResources(ps *PipelineSpec) error { diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go index 962d0127930..ea3328ee722 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go @@ -24,6 +24,73 @@ import ( tb "github.com/tektoncd/pipeline/test/builder" ) +func TestPipeline_Validate_Error(t *testing.T) { + tests := []struct { + name string + p *v1alpha1.Pipeline + }{ + { + name: "period in name", + p: tb.Pipeline("pipe.line", "namespace", tb.PipelineSpec( + tb.PipelineTask("foo", "foo-task"), + )), + }, + { + name: "pipeline name too long", + p: tb.Pipeline("asdf123456789012345678901234567890123456789012345678901234567890", "namespace", tb.PipelineSpec( + tb.PipelineTask("foo", "foo-task"), + )), + }, + { + name: "pipeline spec invalid (duplicate tasks)", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineTask("foo", "foo-task"), + tb.PipelineTask("foo", "foo-task"), + )), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := tt.p.Validate(context.Background()); err == nil { + t.Error("Pipeline.Validate() did not return error, wanted error") + } + }) + } +} + +func TestPipeline_Validate_Valid(t *testing.T) { + tests := []struct { + name string + p *v1alpha1.Pipeline + }{ + { + name: "valid metadata", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineTask("foo", "foo-task"), + )), + }, + { + name: "valid resource declarations and usage", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), + tb.PipelineDeclaredResource("wonderful-resource", v1alpha1.PipelineResourceTypeImage), + tb.PipelineTask("bar", "bar-task", + tb.PipelineTaskInputResource("some-workspace", "great-resource"), + tb.PipelineTaskOutputResource("some-image", "wonderful-resource")), + tb.PipelineTask("foo", "foo-task", + tb.PipelineTaskInputResource("wow-image", "wonderful-resource", tb.From("bar"))), + )), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := tt.p.Spec.Validate(context.Background()); err != nil { + t.Errorf("Pipeline.Validate() returned error: %v", err) + } + }) + } +} + func TestPipelineSpec_Validate_Error(t *testing.T) { tests := []struct { name string From cc7d8aa41e5e18cad77ada3f0cd1cef441c2d235 Mon Sep 17 00:00:00 2001 From: Eli Zucker Date: Fri, 12 Jul 2019 16:21:39 -0400 Subject: [PATCH 2/2] Combine pipeline verification success/error tests. Refactor tests so that success/failure testing is done within the same function (specified with a bool). Also formatted brackets differently following feeback from @imjasonh. --- .../v1alpha1/pipeline_validation_test.go | 390 +++++++++--------- 1 file changed, 185 insertions(+), 205 deletions(-) diff --git a/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go b/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go index ea3328ee722..012c1c0c8b8 100644 --- a/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go +++ b/pkg/apis/pipeline/v1alpha1/pipeline_validation_test.go @@ -24,227 +24,207 @@ import ( tb "github.com/tektoncd/pipeline/test/builder" ) -func TestPipeline_Validate_Error(t *testing.T) { +func TestPipeline_Validate(t *testing.T) { tests := []struct { - name string - p *v1alpha1.Pipeline - }{ - { - name: "period in name", - p: tb.Pipeline("pipe.line", "namespace", tb.PipelineSpec( - tb.PipelineTask("foo", "foo-task"), - )), - }, - { - name: "pipeline name too long", - p: tb.Pipeline("asdf123456789012345678901234567890123456789012345678901234567890", "namespace", tb.PipelineSpec( - tb.PipelineTask("foo", "foo-task"), - )), - }, - { - name: "pipeline spec invalid (duplicate tasks)", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineTask("foo", "foo-task"), - tb.PipelineTask("foo", "foo-task"), - )), - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if err := tt.p.Validate(context.Background()); err == nil { - t.Error("Pipeline.Validate() did not return error, wanted error") - } - }) - } -} - -func TestPipeline_Validate_Valid(t *testing.T) { - tests := []struct { - name string - p *v1alpha1.Pipeline - }{ - { - name: "valid metadata", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineTask("foo", "foo-task"), - )), - }, - { - name: "valid resource declarations and usage", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), - tb.PipelineDeclaredResource("wonderful-resource", v1alpha1.PipelineResourceTypeImage), - tb.PipelineTask("bar", "bar-task", - tb.PipelineTaskInputResource("some-workspace", "great-resource"), - tb.PipelineTaskOutputResource("some-image", "wonderful-resource")), - tb.PipelineTask("foo", "foo-task", - tb.PipelineTaskInputResource("wow-image", "wonderful-resource", tb.From("bar"))), - )), - }, - } + name string + p *v1alpha1.Pipeline + failureExpected bool + }{{ + name: "valid metadata", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineTask("foo", "foo-task"), + )), + failureExpected: false, + }, { + name: "valid resource declarations and usage", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), + tb.PipelineDeclaredResource("wonderful-resource", v1alpha1.PipelineResourceTypeImage), + tb.PipelineTask("bar", "bar-task", + tb.PipelineTaskInputResource("some-workspace", "great-resource"), + tb.PipelineTaskOutputResource("some-image", "wonderful-resource")), + tb.PipelineTask("foo", "foo-task", + tb.PipelineTaskInputResource("wow-image", "wonderful-resource", tb.From("bar"))), + )), + failureExpected: false, + }, { + name: "period in name", + p: tb.Pipeline("pipe.line", "namespace", tb.PipelineSpec( + tb.PipelineTask("foo", "foo-task"), + )), + failureExpected: true, + }, { + name: "pipeline name too long", + p: tb.Pipeline("asdf123456789012345678901234567890123456789012345678901234567890", "namespace", tb.PipelineSpec( + tb.PipelineTask("foo", "foo-task"), + )), + failureExpected: true, + }, { + name: "pipeline spec invalid (duplicate tasks)", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineTask("foo", "foo-task"), + tb.PipelineTask("foo", "foo-task"), + )), + failureExpected: true, + }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := tt.p.Spec.Validate(context.Background()); err != nil { + err := tt.p.Validate(context.Background()) + if (!tt.failureExpected) && (err != nil) { t.Errorf("Pipeline.Validate() returned error: %v", err) } - }) - } -} -func TestPipelineSpec_Validate_Error(t *testing.T) { - tests := []struct { - name string - p *v1alpha1.Pipeline - }{ - { - name: "duplicate tasks", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineTask("foo", "foo-task"), - tb.PipelineTask("foo", "foo-task"), - )), - }, - { - name: "from is on first task", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), - tb.PipelineTask("foo", "foo-task", - tb.PipelineTaskInputResource("the-resource", "great-resource", tb.From("bar"))), - )), - }, - { - name: "from task doesnt exist", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), - tb.PipelineTask("baz", "baz-task"), - tb.PipelineTask("foo", "foo-task", - tb.PipelineTaskInputResource("the-resource", "great-resource", tb.From("bar"))), - )), - }, - { - name: "unused resources declared", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), - tb.PipelineDeclaredResource("extra-resource", v1alpha1.PipelineResourceTypeImage), - tb.PipelineTask("foo", "foo-task", - tb.PipelineTaskInputResource("the-resource", "great-resource")), - )), - }, - { - name: "output resources missing from declaration", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), - tb.PipelineTask("foo", "foo-task", - tb.PipelineTaskInputResource("the-resource", "great-resource"), - tb.PipelineTaskOutputResource("the-magic-resource", "missing-resource")), - )), - }, - { - name: "input resources missing from declaration", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), - tb.PipelineTask("foo", "foo-task", - tb.PipelineTaskInputResource("the-resource", "missing-resource"), - tb.PipelineTaskOutputResource("the-magic-resource", "great-resource")), - )), - }, - { - name: "from resource isn't output by task", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), - tb.PipelineDeclaredResource("wonderful-resource", v1alpha1.PipelineResourceTypeImage), - tb.PipelineTask("bar", "bar-task", - tb.PipelineTaskInputResource("some-workspace", "great-resource")), - tb.PipelineTask("foo", "foo-task", - tb.PipelineTaskInputResource("wow-image", "wonderful-resource", tb.From("bar"))), - )), - }, - { - name: "not defined parameter variable", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineTask("foo", "foo-task", - tb.PipelineTaskParam("a-param", "${params.does-not-exist}")))), - }, - { - name: "not defined parameter variable with defined", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineParam("foo", tb.PipelineParamDefault("something")), - tb.PipelineTask("foo", "foo-task", - tb.PipelineTaskParam("a-param", "${params.foo} and ${params.does-not-exist}")))), - }, - { - name: "invalid dependency graph between the tasks", - p: tb.Pipeline("foo", "namespace", tb.PipelineSpec( - tb.PipelineTask("foo", "foo", tb.RunAfter("bar")), - tb.PipelineTask("bar", "bar", tb.RunAfter("foo")), - )), - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if err := tt.p.Spec.Validate(context.Background()); err == nil { - t.Error("PipelineSpec.Validate() did not return error, wanted error") + if tt.failureExpected && (err == nil) { + t.Error("Pipeline.Validate() did not return error, wanted error") } }) } } -func TestPipelineSpec_Validate_Valid(t *testing.T) { +func TestPipelineSpec_Validate(t *testing.T) { tests := []struct { - name string - p *v1alpha1.Pipeline - }{ - { - name: "no duplicate tasks", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineTask("foo", "foo-task"), - tb.PipelineTask("bar", "bar-task"), - )), - }, - { - // Adding this case because `task.Resources` is a pointer, explicitly making sure this is handled - name: "task without resources", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineDeclaredResource("wonderful-resource", v1alpha1.PipelineResourceTypeImage), - tb.PipelineTask("bar", "bar-task"), - tb.PipelineTask("foo", "foo-task", - tb.PipelineTaskInputResource("wow-image", "wonderful-resource")), - )), - }, - { - name: "valid resource declarations and usage", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), - tb.PipelineDeclaredResource("wonderful-resource", v1alpha1.PipelineResourceTypeImage), - tb.PipelineTask("bar", "bar-task", - tb.PipelineTaskInputResource("some-workspace", "great-resource"), - tb.PipelineTaskOutputResource("some-image", "wonderful-resource")), - tb.PipelineTask("foo", "foo-task", - tb.PipelineTaskInputResource("wow-image", "wonderful-resource", tb.From("bar"))), - )), - }, - { - name: "valid parameter variables", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineParam("baz"), - tb.PipelineParam("foo-is-baz"), - tb.PipelineTask("bar", "bar-task", - tb.PipelineTaskParam("a-param", "${baz} and ${foo-is-baz}")), - )), - }, - { - name: "pipeline parameter nested in task parameter", - p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( - tb.PipelineParam("baz"), - tb.PipelineTask("bar", "bar-task", - tb.PipelineTaskParam("a-param", "${input.workspace.${baz}}")), - )), - }, - } + name string + p *v1alpha1.Pipeline + failureExpected bool + }{{ + name: "no duplicate tasks", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineTask("foo", "foo-task"), + tb.PipelineTask("bar", "bar-task"), + )), + failureExpected: false, + }, { + // Adding this case because `task.Resources` is a pointer, explicitly making sure this is handled + name: "task without resources", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("wonderful-resource", v1alpha1.PipelineResourceTypeImage), + tb.PipelineTask("bar", "bar-task"), + tb.PipelineTask("foo", "foo-task", + tb.PipelineTaskInputResource("wow-image", "wonderful-resource")), + )), + failureExpected: false, + }, { + name: "valid resource declarations and usage", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), + tb.PipelineDeclaredResource("wonderful-resource", v1alpha1.PipelineResourceTypeImage), + tb.PipelineTask("bar", "bar-task", + tb.PipelineTaskInputResource("some-workspace", "great-resource"), + tb.PipelineTaskOutputResource("some-image", "wonderful-resource")), + tb.PipelineTask("foo", "foo-task", + tb.PipelineTaskInputResource("wow-image", "wonderful-resource", tb.From("bar"))), + )), + failureExpected: false, + }, { + name: "valid parameter variables", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineParam("baz"), + tb.PipelineParam("foo-is-baz"), + tb.PipelineTask("bar", "bar-task", + tb.PipelineTaskParam("a-param", "${baz} and ${foo-is-baz}")), + )), + failureExpected: false, + }, { + name: "pipeline parameter nested in task parameter", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineParam("baz"), + tb.PipelineTask("bar", "bar-task", + tb.PipelineTaskParam("a-param", "${input.workspace.${baz}}")), + )), + failureExpected: false, + }, { + name: "duplicate tasks", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineTask("foo", "foo-task"), + tb.PipelineTask("foo", "foo-task"), + )), + failureExpected: true, + }, { + name: "from is on first task", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), + tb.PipelineTask("foo", "foo-task", + tb.PipelineTaskInputResource("the-resource", "great-resource", tb.From("bar"))), + )), + failureExpected: true, + }, { + name: "from task doesnt exist", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), + tb.PipelineTask("baz", "baz-task"), + tb.PipelineTask("foo", "foo-task", + tb.PipelineTaskInputResource("the-resource", "great-resource", tb.From("bar"))), + )), + failureExpected: true, + }, { + name: "unused resources declared", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), + tb.PipelineDeclaredResource("extra-resource", v1alpha1.PipelineResourceTypeImage), + tb.PipelineTask("foo", "foo-task", + tb.PipelineTaskInputResource("the-resource", "great-resource")), + )), + failureExpected: true, + }, { + name: "output resources missing from declaration", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), + tb.PipelineTask("foo", "foo-task", + tb.PipelineTaskInputResource("the-resource", "great-resource"), + tb.PipelineTaskOutputResource("the-magic-resource", "missing-resource")), + )), + failureExpected: true, + }, { + name: "input resources missing from declaration", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), + tb.PipelineTask("foo", "foo-task", + tb.PipelineTaskInputResource("the-resource", "missing-resource"), + tb.PipelineTaskOutputResource("the-magic-resource", "great-resource")), + )), + failureExpected: true, + }, { + name: "from resource isn't output by task", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineDeclaredResource("great-resource", v1alpha1.PipelineResourceTypeGit), + tb.PipelineDeclaredResource("wonderful-resource", v1alpha1.PipelineResourceTypeImage), + tb.PipelineTask("bar", "bar-task", + tb.PipelineTaskInputResource("some-workspace", "great-resource")), + tb.PipelineTask("foo", "foo-task", + tb.PipelineTaskInputResource("wow-image", "wonderful-resource", tb.From("bar"))), + )), + failureExpected: true, + }, { + name: "not defined parameter variable", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineTask("foo", "foo-task", + tb.PipelineTaskParam("a-param", "${params.does-not-exist}")))), + failureExpected: true, + }, { + name: "not defined parameter variable with defined", + p: tb.Pipeline("pipeline", "namespace", tb.PipelineSpec( + tb.PipelineParam("foo", tb.PipelineParamDefault("something")), + tb.PipelineTask("foo", "foo-task", + tb.PipelineTaskParam("a-param", "${params.foo} and ${params.does-not-exist}")))), + failureExpected: true, + }, { + name: "invalid dependency graph between the tasks", + p: tb.Pipeline("foo", "namespace", tb.PipelineSpec( + tb.PipelineTask("foo", "foo", tb.RunAfter("bar")), + tb.PipelineTask("bar", "bar", tb.RunAfter("foo")), + )), + failureExpected: true, + }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if err := tt.p.Spec.Validate(context.Background()); err != nil { + err := tt.p.Spec.Validate(context.Background()) + if (!tt.failureExpected) && (err != nil) { t.Errorf("PipelineSpec.Validate() returned error: %v", err) } + + if tt.failureExpected && (err == nil) { + t.Error("PipelineSpec.Validate() did not return error, wanted error") + } }) } }