Add Unit Tests for TestMissingResultWhenStepErrorIsIgnored and update…

… e2e test: TestFailingStepOnContinue This commit adds test coverage for a pipelinerun that has 2 tasks where the first task produces 2 results that are consumed in the 2nd task. However, the first task contains a failing step so only 1 of the 2 results are produced which results in a failing pipeline run. Prior to this commit, this was an E2E integration test, but missing unit test coverage. This increases the test coverage and also modifies the e2e test to only test the behavior of the TaskRun, since the PipelineRun behavior is now being tested in unit tests to reduce redundancies, improve speed, and reduce flakes.
tektoncd · Jun 12, 2023 · c46b69f · c46b69f
1 parent 647a5ce
commit c46b69f
Show file tree

Hide file tree

Showing 2 changed files with 217 additions and 54 deletions.
diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go
@@ -1049,6 +1049,188 @@ spec:
 	}
 }
 
+func TestMissingResultWhenStepErrorIsIgnored(t *testing.T) {
+	prs := []*v1beta1.PipelineRun{parse.MustParseV1beta1PipelineRun(t, `
+metadata:
+  name: test-pipeline-missing-results
+  namespace: foo
+spec:
+  serviceAccountName: test-sa-0
+  pipelineSpec:
+    tasks:
+    - name: task1
+      taskSpec:
+        results:
+        - name: result1
+          type: string
+        - name: result2
+          type: string
+        steps:
+        - name: failing-step
+          onError: continue
+          image: busybox
+          script: 'echo -n 123 | tee $(results.result1.path); exit 1; echo -n 456 | tee $(results.result2.path)'
+    - name: task2
+      runAfter: [ task1 ]
+      params:
+      - name: param1
+        value: $(tasks.task1.results.result1)
+      - name: param2
+        value: $(tasks.task1.results.result2)
+      taskSpec:
+        params:
+        - name: param1
+          type: string
+        - name: param2
+          type: string
+        steps:
+        - name: foo
+          image: busybox
+          script: 'exit 0'
+  conditions:
+  - type: Succeeded
+    status: "False"
+    reason: "InvalidTaskResultReference"
+    message: "Could not find result with name result2 for task task1"
+  childReferences:
+  - apiVersion: tekton.dev/v1beta1
+    kind: TaskRun
+    name: pr-test-pipeline-missing-results-0
+    pipelineTaskName: test-pipeline-missing-results-task1`)}
+
+	trs := []*v1beta1.TaskRun{mustParseTaskRunWithObjectMeta(t,
+		taskRunObjectMeta("test-pipeline-missing-results-task1", "foo",
+			"test-pipeline-missing-results", "test-pipeline", "task1", true),
+		`
+spec:
+  serviceAccountName: test-sa
+  timeout: 1h0m0s
+status:
+  conditions:
+  - status: "True"
+    type: Succeeded
+  taskResults:
+  - name: result1
+    value: 123
+`)}
+
+	expectedPipelineRun :=
+		parse.MustParseV1beta1PipelineRun(t, `
+metadata:
+  name: test-pipeline-missing-results
+  namespace: foo
+  annotations: {}
+  labels:
+    tekton.dev/pipeline: test-pipeline-missing-results
+spec:
+  serviceAccountName: test-sa-0
+  pipelineSpec:
+    tasks:
+    - name: task1
+      taskSpec:
+        results:
+        - name: result1
+          type: string
+        - name: result2
+          type: string
+        steps:
+        - name: failing-step
+          onError: continue
+          image: busybox
+          script: 'echo -n 123 | tee $(results.result1.path); exit 1; echo -n 456 | tee $(results.result2.path)'
+    - name: task2
+      runAfter: [ task1 ]
+      params:
+      - name: param1
+        value: $(tasks.task1.results.result1)
+      - name: param2
+        value: $(tasks.task1.results.result2)
+      taskSpec:
+        params:
+        - name: param1
+          type: string
+        - name: param2
+          type: string
+        steps:
+        - name: foo
+          image: busybox
+          script: 'exit 0'
+status:
+  pipelineSpec:
+    tasks:
+    - name: task1
+      taskSpec:
+        results:
+        - name: result1
+          type: string
+        - name: result2
+          type: string
+        steps:
+        - name: failing-step
+          onError: continue
+          image: busybox
+          script: 'echo -n 123 | tee $(results.result1.path); exit 1; echo -n 456 | tee $(results.result2.path)'
+    - name: task2
+      runAfter: [ task1 ]
+      params:
+      - name: param1
+        value: $(tasks.task1.results.result1)
+      - name: param2
+        value: $(tasks.task1.results.result2)
+      taskSpec:
+        params:
+        - name: param1
+          type: string
+        - name: param2
+          type: string
+        steps:
+        - name: foo
+          image: busybox
+          script: 'exit 0'
+  conditions:
+  - message: "Could not find result with name result2 for task task1"
+    reason: InvalidTaskResultReference
+    status: "False"
+    type: Succeeded
+  childReferences:
+  - apiVersion: tekton.dev/v1beta1
+    kind: TaskRun
+    name: test-pipeline-missing-results-task1
+    pipelineTaskName: task1
+  provenance:
+    featureFlags:
+      RunningInEnvWithInjectedSidecars: true
+      EnableAPIFields: "beta"
+      AwaitSidecarReadiness: true
+      VerificationNoMatchPolicy: "ignore"
+      EnableProvenanceInStatus: true
+      ResultExtractionMethod: "termination-message"
+      MaxResultSize: 4096
+`)
+	d := test.Data{
+		PipelineRuns: prs,
+		TaskRuns:     trs,
+	}
+	prt := newPipelineRunTest(t, d)
+	defer prt.Cancel()
+
+	reconciledRun, clients := prt.reconcileRun("foo", "test-pipeline-missing-results", []string{}, true)
+	if reconciledRun.Status.CompletionTime == nil {
+		t.Errorf("Expected a CompletionTime on invalid PipelineRun but was nil")
+	}
+
+	// The PipelineRun should be marked as failed due to InvalidTaskResultReference.
+	if d := cmp.Diff(reconciledRun, expectedPipelineRun, ignoreResourceVersion, ignoreLastTransitionTime, ignoreTypeMeta, ignoreStartTime, ignoreCompletionTime); d != "" {
+		t.Errorf("Expected to see PipelineRun run marked as failed with the reason: InvalidTaskResultReference. Diff %s", diff.PrintWantGot(d))
+	}
+
+	// Check that the expected TaskRun was created
+	taskRuns := getTaskRunsForPipelineRun(prt.TestAssets.Ctx, t, clients, "foo", "test-pipeline-missing-results")
+
+	// We expect only 1 TaskRun to be created, since the PipelineRun should fail before creating the 2nd TaskRun due to the InvalidTaskResultReference
+	validateTaskRunsCount(t, taskRuns, 1)
+}
+
 func TestReconcile_InvalidPipelineRunNames(t *testing.T) {
 	// TestReconcile_InvalidPipelineRunNames runs "Reconcile" on several PipelineRuns that have invalid names.
 	// It verifies that reconcile fails, how it fails and which events are triggered.

diff --git a/test/ignore_step_error_test.go b/test/ignore_step_error_test.go
@@ -24,83 +24,64 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun"
+	"github.com/google/go-cmp/cmp"
+	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
+	"github.com/tektoncd/pipeline/test/diff"
 	"github.com/tektoncd/pipeline/test/parse"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"knative.dev/pkg/apis"
 	knativetest "knative.dev/pkg/test"
-	"knative.dev/pkg/test/helpers"
 )
 
-func TestMissingResultWhenStepErrorIsIgnored(t *testing.T) {
+func TestFailingStepOnContinue(t *testing.T) {
 	ctx := context.Background()
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
 	c, namespace := setup(ctx, t)
 	knativetest.CleanupOnInterrupt(func() { tearDown(ctx, t, c, namespace) }, t.Logf)
 	defer tearDown(ctx, t, c, namespace)
-
-	pipelineRun := parse.MustParseV1beta1PipelineRun(t, fmt.Sprintf(`
+	taskRunName := "mytaskrun"
+	tr := parse.MustParseV1beta1TaskRun(t, fmt.Sprintf(`
 metadata:
   name: %s
+  namespace: %s
 spec:
-  pipelineSpec:
-    tasks:
-    - name: task1
-      taskSpec:
-        results:
-        - name: result1
-        - name: result2
-        steps:
-        - name: failing-step
-          onError: continue
-          image: busybox
-          script: 'echo -n 123 | tee $(results.result1.path); exit 1; echo -n 456 | tee $(results.result2.path)'
-    - name: task2
-      runAfter: [ task1 ]
-      params:
-      - name: param1
-        value: $(tasks.task1.results.result1)
-      - name: param2
-        value: $(tasks.task1.results.result2)
-      taskSpec:
-        params:
-        - name: param1
-        - name: param2
-        steps:
-        - name: foo
-          image: busybox
-          script: 'exit 0'`, helpers.ObjectNameForTest(t)))
-
-	if _, err := c.V1beta1PipelineRunClient.Create(ctx, pipelineRun, metav1.CreateOptions{}); err != nil {
-		t.Fatalf("Failed to create PipelineRun `%s`: %s", pipelineRun.Name, err)
+  taskSpec:
+    results:
+      - name: result1
+      - name: result2
+    steps:
+      - name: failing-step
+        onError: continue
+        image: busybox
+        script: 'echo -n 123 | tee $(results.result1.path); exit 1; echo -n 456 | tee $(results.result2.path)'
+`, taskRunName, namespace))
+
+	if _, err := c.V1beta1TaskRunClient.Create(ctx, tr, metav1.CreateOptions{}); err != nil {
+		t.Fatalf("Failed to create TaskRun: %s", err)
 	}
 
-	t.Logf("Waiting for PipelineRun in namespace %s to fail", namespace)
-	if err := WaitForPipelineRunState(ctx, c, pipelineRun.Name, timeout, FailedWithReason(pipelinerun.ReasonInvalidTaskResultReference, pipelineRun.Name), "InvalidTaskResultReference", v1beta1Version); err != nil {
-		t.Errorf("Error waiting for PipelineRun to fail: %s", err)
+	t.Logf("Waiting for TaskRun in namespace %s to finish", namespace)
+	if err := WaitForTaskRunState(ctx, c, taskRunName, TaskRunSucceed(taskRunName), "TaskRunSucceeded", v1beta1Version); err != nil {
+		t.Errorf("Error waiting for TaskRun to finish: %s", err)
 	}
 
-	taskrunList, err := c.V1beta1TaskRunClient.List(ctx, metav1.ListOptions{LabelSelector: "tekton.dev/pipelineRun=" + pipelineRun.Name})
+	taskRun, err := c.V1beta1TaskRunClient.Get(ctx, taskRunName, metav1.GetOptions{})
 	if err != nil {
-		t.Fatalf("Error listing TaskRuns for PipelineRun %s: %s", pipelineRun.Name, err)
+		t.Fatalf("Couldn't get expected TaskRun %s: %s", taskRunName, err)
 	}
 
-	if len(taskrunList.Items) != 1 {
-		t.Fatalf("The pipelineRun should have exactly 1 taskRun for the first task \"task1\"")
+	if !taskRun.Status.GetCondition(apis.ConditionSucceeded).IsTrue() {
+		t.Fatalf("Expected TaskRun %s to have succeeded but Status is %v", taskRunName, taskRun.Status)
 	}
 
-	taskrunItem := taskrunList.Items[0]
-	if taskrunItem.Labels["tekton.dev/pipelineTask"] != "task1" {
-		t.Fatalf("TaskRun was not found for the task \"task1\"")
-	}
-
-	if len(taskrunItem.Status.TaskRunResults) != 1 {
-		t.Fatalf("task1 should have produced a result before failing the step")
-	}
+	expectedResults := []v1beta1.TaskRunResult{{
+		Name:  "result1",
+		Type:  "string",
+		Value: *v1beta1.NewArrayOrString("123"),
+	}}
 
-	for _, r := range taskrunItem.Status.TaskRunResults {
-		if r.Name == "result1" && r.Value.StringVal != "123" {
-			t.Fatalf("task1 should have initialized a result \"result1\" to \"123\"")
-		}
+	if d := cmp.Diff(expectedResults, taskRun.Status.TaskRunResults); d != "" {
+		t.Errorf("Got unexpected results %s", diff.PrintWantGot(d))
 	}
 }