From 72804c3d5e081d5b2c9c3a5218002afd7ecafade Mon Sep 17 00:00:00 2001 From: Yongxuan Zhang Date: Fri, 17 Mar 2023 19:26:22 +0000 Subject: [PATCH] divide ApplyTaskResultsToPipelineResults tests into success and error This commit divides ApplyTaskResultsToPipelineResults unit tests into success and error, two separate tests to improve the readibility and easier to find bugs. Before this commit, the unit test doesn't check the case when the return error is not nil but expect nil, and some errors are not checked. Signed-off-by: Yongxuan Zhang yongxuanzhang@google.com --- .../pipelinerun/resources/apply_test.go | 314 ++++++++++-------- 1 file changed, 170 insertions(+), 144 deletions(-) diff --git a/pkg/reconciler/pipelinerun/resources/apply_test.go b/pkg/reconciler/pipelinerun/resources/apply_test.go index 7f705d772bd..8aa34e2979f 100644 --- a/pkg/reconciler/pipelinerun/resources/apply_test.go +++ b/pkg/reconciler/pipelinerun/resources/apply_test.go @@ -3505,7 +3505,7 @@ func TestApplyFinallyResultsToPipelineResults(t *testing.T) { } } -func TestApplyTaskResultsToPipelineResults(t *testing.T) { +func TestApplyTaskResultsToPipelineResults_Success(t *testing.T) { for _, tc := range []struct { description string results []v1beta1.PipelineResult @@ -3513,7 +3513,6 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { runResults map[string][]v1beta1.CustomRunResult skippedTasks []v1beta1.SkippedTask expectedResults []v1beta1.PipelineRunResult - expectedError error }{{ description: "non-reference-results", results: []v1beta1.PipelineResult{{ @@ -3529,60 +3528,6 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { }, }, expectedResults: nil, - }, { - description: "array-index-out-of-bound", - results: []v1beta1.PipelineResult{{ - Name: "pipeline-result-1", - Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo[4])"), - }}, - taskResults: map[string][]v1beta1.TaskRunResult{ - "pt1": { - { - Name: "foo", - Value: *v1beta1.NewStructuredValues("do", "rae", "mi"), - }, - }, - }, - expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referred results don't exist"), - }, { - description: "object-reference-key-not-exist", - results: []v1beta1.PipelineResult{{ - Name: "pipeline-result-1", - Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo.key3)"), - }}, - taskResults: map[string][]v1beta1.TaskRunResult{ - "pt1": { - { - Name: "foo", - Value: *v1beta1.NewObject(map[string]string{ - "key1": "val1", - "key2": "val2", - }), - }, - }, - }, - expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referred results don't exist"), - }, { - description: "object-results-resultname-not-exist", - results: []v1beta1.PipelineResult{{ - Name: "pipeline-result-1", - Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.bar.key1)"), - }}, - taskResults: map[string][]v1beta1.TaskRunResult{ - "pt1": { - { - Name: "foo", - Value: *v1beta1.NewObject(map[string]string{ - "key1": "val1", - "key2": "val2", - }), - }, - }, - }, - expectedResults: nil, - expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referred results don't exist"), }, { description: "apply-array-results", results: []v1beta1.PipelineResult{{ @@ -3764,6 +3709,164 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { }}, }, expectedResults: nil, + }, { + description: "multiple-results-multiple-successful-tasks ", + results: []v1beta1.PipelineResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo)"), + }, { + Name: "pipeline-result-2", + Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo), $(tasks.pt2.results.baz), $(tasks.pt1.results.bar), $(tasks.pt2.results.baz), $(tasks.pt1.results.foo)"), + }}, + taskResults: map[string][]v1beta1.TaskRunResult{ + "pt1": { + { + Name: "foo", + Value: *v1beta1.NewStructuredValues("do"), + }, { + Name: "bar", + Value: *v1beta1.NewStructuredValues("mi"), + }, + }, + "pt2": {{ + Name: "baz", + Value: *v1beta1.NewStructuredValues("rae"), + }}, + }, + expectedResults: []v1beta1.PipelineRunResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewStructuredValues("do"), + }, { + Name: "pipeline-result-2", + Value: *v1beta1.NewStructuredValues("do, rae, mi, rae, do"), + }}, + }, { + description: "multiple-results-custom-and-normal-tasks", + results: []v1beta1.PipelineResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewStructuredValues("$(tasks.customtask.results.foo)"), + }, { + Name: "pipeline-result-2", + Value: *v1beta1.NewStructuredValues("$(tasks.customtask.results.foo), $(tasks.normaltask.results.baz), $(tasks.customtask.results.bar), $(tasks.normaltask.results.baz), $(tasks.customtask.results.foo)"), + }}, + runResults: map[string][]v1beta1.CustomRunResult{ + "customtask": { + { + Name: "foo", + Value: "do", + }, { + Name: "bar", + Value: "mi", + }, + }, + }, + taskResults: map[string][]v1beta1.TaskRunResult{ + "normaltask": {{ + Name: "baz", + Value: *v1beta1.NewStructuredValues("rae"), + }}, + }, + expectedResults: []v1beta1.PipelineRunResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewStructuredValues("do"), + }, { + Name: "pipeline-result-2", + Value: *v1beta1.NewStructuredValues("do, rae, mi, rae, do"), + }}, + }, { + description: "multiple-results-skipped-and-normal-tasks", + results: []v1beta1.PipelineResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo)"), + }, { + Name: "pipeline-result-2", + Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo), $(tasks.normaltask.results.baz)"), + }}, + taskResults: map[string][]v1beta1.TaskRunResult{ + "normaltask": {{ + Name: "baz", + Value: *v1beta1.NewStructuredValues("rae"), + }}, + }, + skippedTasks: []v1beta1.SkippedTask{{ + Name: "skippedTask", + }}, + expectedResults: nil, + }} { + t.Run(tc.description, func(t *testing.T) { + received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.skippedTasks) + if err != nil { + t.Errorf("Got unecpected error:%v", err) + } + if d := cmp.Diff(tc.expectedResults, received); d != "" { + t.Errorf(diff.PrintWantGot(d)) + } + }) + } +} + +func TestApplyTaskResultsToPipelineResults_Error(t *testing.T) { + for _, tc := range []struct { + description string + results []v1beta1.PipelineResult + taskResults map[string][]v1beta1.TaskRunResult + runResults map[string][]v1beta1.CustomRunResult + expectedResults []v1beta1.PipelineRunResult + expectedError error + }{{ + description: "array-index-out-of-bound", + results: []v1beta1.PipelineResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo[4])"), + }}, + taskResults: map[string][]v1beta1.TaskRunResult{ + "pt1": { + { + Name: "foo", + Value: *v1beta1.NewStructuredValues("do", "rae", "mi"), + }, + }, + }, + expectedResults: nil, + expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referred results don't exist"), + }, { + description: "object-reference-key-not-exist", + results: []v1beta1.PipelineResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo.key3)"), + }}, + taskResults: map[string][]v1beta1.TaskRunResult{ + "pt1": { + { + Name: "foo", + Value: *v1beta1.NewObject(map[string]string{ + "key1": "val1", + "key2": "val2", + }), + }, + }, + }, + expectedResults: nil, + expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referred results don't exist"), + }, { + description: "object-results-resultname-not-exist", + results: []v1beta1.PipelineResult{{ + Name: "pipeline-result-1", + Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.bar.key1)"), + }}, + taskResults: map[string][]v1beta1.TaskRunResult{ + "pt1": { + { + Name: "foo", + Value: *v1beta1.NewObject(map[string]string{ + "key1": "val1", + "key2": "val2", + }), + }, + }, + }, + expectedResults: nil, + expectedError: fmt.Errorf("invalid pipelineresults [pipeline-result-1], the referred results don't exist"), }, { description: "invalid-result-variable-no-returned-result", results: []v1beta1.PipelineResult{{ @@ -3802,6 +3905,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { }}, }, expectedResults: nil, + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), }, { description: "invalid-result-name-no-returned-result", results: []v1beta1.PipelineResult{{ @@ -3824,6 +3928,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { }}, taskResults: map[string][]v1beta1.TaskRunResult{}, expectedResults: nil, + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), }, { description: "mixed-success-tasks-some-returned-results", results: []v1beta1.PipelineResult{{ @@ -3844,37 +3949,6 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { Value: *v1beta1.NewStructuredValues("rae"), }}, expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), - }, { - description: "multiple-results-multiple-successful-tasks ", - results: []v1beta1.PipelineResult{{ - Name: "pipeline-result-1", - Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo)"), - }, { - Name: "pipeline-result-2", - Value: *v1beta1.NewStructuredValues("$(tasks.pt1.results.foo), $(tasks.pt2.results.baz), $(tasks.pt1.results.bar), $(tasks.pt2.results.baz), $(tasks.pt1.results.foo)"), - }}, - taskResults: map[string][]v1beta1.TaskRunResult{ - "pt1": { - { - Name: "foo", - Value: *v1beta1.NewStructuredValues("do"), - }, { - Name: "bar", - Value: *v1beta1.NewStructuredValues("mi"), - }, - }, - "pt2": {{ - Name: "baz", - Value: *v1beta1.NewStructuredValues("rae"), - }}, - }, - expectedResults: []v1beta1.PipelineRunResult{{ - Name: "pipeline-result-1", - Value: *v1beta1.NewStructuredValues("do"), - }, { - Name: "pipeline-result-2", - Value: *v1beta1.NewStructuredValues("do, rae, mi, rae, do"), - }}, }, { description: "no-run-results-no-returned-results", results: []v1beta1.PipelineResult{{ @@ -3883,6 +3957,7 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { }}, runResults: map[string][]v1beta1.CustomRunResult{}, expectedResults: nil, + expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), }, { description: "wrong-customtask-name-no-returned-result", results: []v1beta1.PipelineResult{{ @@ -3933,66 +4008,17 @@ func TestApplyTaskResultsToPipelineResults(t *testing.T) { }, expectedResults: nil, expectedError: fmt.Errorf("invalid pipelineresults [foo], the referred results don't exist"), - }, { - description: "multiple-results-custom-and-normal-tasks", - results: []v1beta1.PipelineResult{{ - Name: "pipeline-result-1", - Value: *v1beta1.NewStructuredValues("$(tasks.customtask.results.foo)"), - }, { - Name: "pipeline-result-2", - Value: *v1beta1.NewStructuredValues("$(tasks.customtask.results.foo), $(tasks.normaltask.results.baz), $(tasks.customtask.results.bar), $(tasks.normaltask.results.baz), $(tasks.customtask.results.foo)"), - }}, - runResults: map[string][]v1beta1.CustomRunResult{ - "customtask": { - { - Name: "foo", - Value: "do", - }, { - Name: "bar", - Value: "mi", - }, - }, - }, - taskResults: map[string][]v1beta1.TaskRunResult{ - "normaltask": {{ - Name: "baz", - Value: *v1beta1.NewStructuredValues("rae"), - }}, - }, - expectedResults: []v1beta1.PipelineRunResult{{ - Name: "pipeline-result-1", - Value: *v1beta1.NewStructuredValues("do"), - }, { - Name: "pipeline-result-2", - Value: *v1beta1.NewStructuredValues("do, rae, mi, rae, do"), - }}, - }, { - description: "multiple-results-skipped-and-normal-tasks", - results: []v1beta1.PipelineResult{{ - Name: "pipeline-result-1", - Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo)"), - }, { - Name: "pipeline-result-2", - Value: *v1beta1.NewStructuredValues("$(tasks.skippedTask.results.foo), $(tasks.normaltask.results.baz)"), - }}, - taskResults: map[string][]v1beta1.TaskRunResult{ - "normaltask": {{ - Name: "baz", - Value: *v1beta1.NewStructuredValues("rae"), - }}, - }, - skippedTasks: []v1beta1.SkippedTask{{ - Name: "skippedTask", - }}, - expectedResults: nil, }} { t.Run(tc.description, func(t *testing.T) { - received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, tc.skippedTasks) - if tc.expectedError != nil { - if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" { - t.Errorf("ApplyTaskResultsToPipelineResults() errors diff %s", diff.PrintWantGot(d)) - } + received, err := ApplyTaskResultsToPipelineResults(context.Background(), tc.results, tc.taskResults, tc.runResults, nil /*skipped tasks*/) + if err == nil { + t.Errorf("Expect error but got nil") + } + + if d := cmp.Diff(tc.expectedError.Error(), err.Error()); d != "" { + t.Errorf("ApplyTaskResultsToPipelineResults() errors diff %s", diff.PrintWantGot(d)) } + if d := cmp.Diff(tc.expectedResults, received); d != "" { t.Errorf(diff.PrintWantGot(d)) }