Skip to content

Commit

Permalink
resolve test issues
Browse files Browse the repository at this point in the history
  • Loading branch information
JeromeJu committed Jan 31, 2023
1 parent 94d35c1 commit 5980bc4
Show file tree
Hide file tree
Showing 5 changed files with 125 additions and 58 deletions.
1 change: 1 addition & 0 deletions config/config-feature-flags.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,4 @@ data:
# If set to "none", then Tekton will not have non-falsifiable provenance.
# This is an experimental feature and thus should still be considered an alpha feature.
enforce-nonfalsifiablity: "none"
embedded-status: "minimal"
35 changes: 35 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -1052,6 +1052,41 @@ func propagateWorkspaces(rpt *resources.ResolvedPipelineTask) (*resources.Resolv
return rpt, nil
}

// Check with Andrew, if not with this it will fail the final test
// updateTaskRunsStatusDirectly is used with "full" or "both" set as the value for the "embedded-status" feature flag.
// When the "full" and "both" options are removed, updateTaskRunsStatusDirectly can be removed.
func (c *Reconciler) updateTaskRunsStatusDirectly(pr *v1beta1.PipelineRun) error {
for taskRunName := range pr.Status.TaskRuns {
prtrs := pr.Status.TaskRuns[taskRunName]
tr, err := c.taskRunLister.TaskRuns(pr.Namespace).Get(taskRunName)
if err != nil {
// If the TaskRun isn't found, it just means it won't be run
if !kerrors.IsNotFound(err) {
return fmt.Errorf("error retrieving TaskRun %s: %w", taskRunName, err)
}
} else {
prtrs.Status = &tr.Status
}
}
return nil
}

// updateRunsStatusDirectly is used with "full" or "both" set as the value for the "embedded-status" feature flag.
// When the "full" and "both" options are removed, updateRunsStatusDirectly can be removed.
func (c *Reconciler) updateRunsStatusDirectly(pr *v1beta1.PipelineRun) error {
for runName := range pr.Status.Runs {
prRunStatus := pr.Status.Runs[runName]
run, err := c.customRunLister.CustomRuns(pr.Namespace).Get(runName)
if err != nil {
if !kerrors.IsNotFound(err) {
return fmt.Errorf("error retrieving Run %s: %w", runName, err)
}
} else {
prRunStatus.Status = &run.Status
}
}
return nil
}
func getTaskrunWorkspaces(ctx context.Context, pr *v1beta1.PipelineRun, rpt *resources.ResolvedPipelineTask) ([]v1beta1.WorkspaceBinding, string, error) {
var workspaces []v1beta1.WorkspaceBinding
var pipelinePVCWorkspaceName string
Expand Down
113 changes: 82 additions & 31 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1458,9 +1458,10 @@ func TestReconcileOnCompletedPipelineRun(t *testing.T) {
// in the PipelineRun status, that the completion status is not altered, that not error is returned and
// a successful event is triggered
taskRunName := "test-pipeline-run-completed-hello-world"
prs := []*v1beta1.PipelineRun{parse.MustParseV1beta1PipelineRun(t, `
pipelineRunName := "test-pipeline-run-completed"
prs := []*v1beta1.PipelineRun{parse.MustParseV1beta1PipelineRun(t, fmt.Sprintf(`
metadata:
name: test-pipeline-run-completed
name: %s
namespace: foo
spec:
pipelineRef:
Expand All @@ -1473,17 +1474,21 @@ status:
reason: Succeeded
status: "True"
type: Succeeded
taskRuns:
test-pipeline-run-completed-hello-world:
childReferences:
- name: test-pipeline-run-completed-hello-world
pipelineTaskName: hello-world-1
`)}
`, pipelineRunName))}
ps := []*v1beta1.Pipeline{simpleHelloWorldPipeline}
ts := []*v1beta1.Task{simpleHelloWorldTask}
trs := []*v1beta1.TaskRun{createHelloWorldTaskRunWithStatus(t, taskRunName, "foo",
"test-pipeline-run-completed", "test-pipeline", "",
pipelineRunName, "test-pipeline", "",
apis.Condition{
Type: apis.ConditionSucceeded,
})}
expectedChildReferences := []v1beta1.ChildStatusReference{{
Name: taskRunName,
PipelineTaskName: "hello-world-1",
}}

d := test.Data{
PipelineRuns: prs,
Expand All @@ -1497,7 +1502,7 @@ status:
wantEvents := []string{
"Normal Succeeded All Tasks have completed executing",
}
reconciledRun, clients := prt.reconcileRun("foo", "test-pipeline-run-completed", wantEvents, false)
reconciledRun, clients := prt.reconcileRun("foo", pipelineRunName, wantEvents, false)

actions := clients.Pipeline.Actions()
if len(actions) < 2 {
Expand Down Expand Up @@ -1528,18 +1533,22 @@ status:
t.Errorf("Expected PipelineRun status to be complete, but was %v", reconciledRun.Status.GetCondition(apis.ConditionSucceeded))
}

expectedTaskRunsStatus := make(map[string]*v1beta1.PipelineRunTaskRunStatus)
expectedTaskRunsStatus[taskRunName] = &v1beta1.PipelineRunTaskRunStatus{
PipelineTaskName: "hello-world-1",
Status: &v1beta1.TaskRunStatus{
Status: duckv1.Status{
Conditions: []apis.Condition{{Type: apis.ConditionSucceeded}},
},
},
if d := cmp.Diff(reconciledRun.Status.ChildReferences, expectedChildReferences); d != "" {
t.Fatalf("Expected PipelineRun status to match ChildReference(s) status, but got a mismatch %s", diff.PrintWantGot(d))
}

if d := cmp.Diff(reconciledRun.Status.TaskRuns, expectedTaskRunsStatus); d != "" {
t.Fatalf("Expected PipelineRun status to match TaskRun(s) status, but got a mismatch %s", diff.PrintWantGot(d))
// Check the status of taskruns from ChildReferences to be Succeeded
trFromChildRef, err := clients.Pipeline.TektonV1beta1().TaskRuns("foo").Get(prt.TestAssets.Ctx, reconciledRun.Status.ChildReferences[0].Name, metav1.GetOptions{})
if err != nil {
t.Fatalf("Failure to get TaskRun %s from", err)
}

expectedTaskRunStatus := v1beta1.TaskRunStatus{Status: duckv1.Status{
Conditions: []apis.Condition{{Type: apis.ConditionSucceeded}},
}}

if d := cmp.Diff(trFromChildRef.Status, expectedTaskRunStatus); d != "" {
t.Fatalf("Expected TaskRun Status Conditions to match ChildReferences TaskRun Status Condition, but got a mismatch %s", diff.PrintWantGot(d))
}
}

Expand Down Expand Up @@ -4722,9 +4731,8 @@ status:
name: test-pipeline-run-finally-results-task-run-b
pipelineTaskName: b-task
`)
var expectedPr *v1beta1.PipelineRun

expectedPr = expectedPrMinimalStatus
expectedPr := expectedPrMinimalStatus

if d := cmp.Diff(expectedPr, reconciledRun, ignoreResourceVersion, ignoreLastTransitionTime, ignoreCompletionTime, ignoreStartTime, cmpopts.EquateEmpty()); d != "" {
t.Errorf("expected to see pipeline run results created. Diff %s", diff.PrintWantGot(d))
Expand Down Expand Up @@ -4874,9 +4882,8 @@ status:
name: test-pipeline-run-results-task-run-a
pipelineTaskName: a-task
`)
var expectedPr *v1beta1.PipelineRun

expectedPr = expectedPrMinimalStatus
expectedPr := expectedPrMinimalStatus

if d := cmp.Diff(expectedPr, reconciledRun, ignoreResourceVersion, ignoreLastTransitionTime, ignoreCompletionTime, ignoreStartTime, cmpopts.EquateEmpty()); d != "" {
t.Errorf("expected to see pipeline run results created. Diff %s", diff.PrintWantGot(d))
Expand Down Expand Up @@ -5457,7 +5464,7 @@ spec:
verifyCustomRunOrRunStatusesNames(t, pipeline.CustomRunControllerName, reconciledRun.Status, customRunName)
}

// TODO
// TODOTest
func TestReconcilePipeline_FinalTasks(t *testing.T) {
tests := []struct {
name string
Expand All @@ -5466,7 +5473,7 @@ func TestReconcilePipeline_FinalTasks(t *testing.T) {
ps []*v1beta1.Pipeline
ts []*v1beta1.Task
trs []*v1beta1.TaskRun
// TODO how should we update this
// TODOhow should we update this
expectedTaskRuns map[string]*v1beta1.PipelineRunTaskRunStatus
expectedChildReferences []v1beta1.ChildStatusReference
pipelineRunStatusUnknown bool
Expand Down Expand Up @@ -5747,6 +5754,12 @@ func TestReconcilePipeline_FinalTasks(t *testing.T) {
Name: "task-run-dag-task-2",
PipelineTaskName: "dag-task-2",
}},

expectedTaskRuns: map[string]*v1beta1.PipelineRunTaskRunStatus{
"task-run-dag-task-1": getTaskRunStatus("dag-task-1", corev1.ConditionFalse),
"task-run-dag-task-2": getTaskRunStatus("dag-task-2", corev1.ConditionUnknown),
},

pipelineRunStatusUnknown: true,
}, {
// pipeline run should not schedule final tasks until dag tasks are done i.e.
Expand Down Expand Up @@ -5809,6 +5822,10 @@ func TestReconcilePipeline_FinalTasks(t *testing.T) {
PipelineTaskName: "dag-task-1",
}},

expectedTaskRuns: map[string]*v1beta1.PipelineRunTaskRunStatus{
"task-run-dag-task-1": getTaskRunStatus("dag-task-1", corev1.ConditionUnknown),
},

pipelineRunStatusUnknown: true,
}}

Expand All @@ -5822,10 +5839,11 @@ func TestReconcilePipeline_FinalTasks(t *testing.T) {
Tasks: tt.ts,
TaskRuns: tt.trs,
}
namespace := "foo"
prt := newPipelineRunTest(t, d)
defer prt.Cancel()

reconciledRun, clients := prt.reconcileRun("foo", tt.pipelineRunName, []string{}, false)
reconciledRun, clients := prt.reconcileRun(namespace, tt.pipelineRunName, []string{}, false)

actions := clients.Pipeline.Actions()
if len(actions) < 2 {
Expand Down Expand Up @@ -5860,9 +5878,9 @@ func TestReconcilePipeline_FinalTasks(t *testing.T) {
t.Errorf("Expected PipelineRun status to be failed, but was %v for %s",
reconciledRun.Status.GetCondition(apis.ConditionSucceeded), tt.name)
}
// if d := cmp.Diff(reconciledRun.Status.TaskRuns, tt.expectedTaskRuns); d != "" {
// t.Fatalf("Expected PipelineRunTaskRun status to match TaskRun(s) status, but got a mismatch for %s: %s", tt.name, d)
// }
if d := cmp.Diff(reconciledRun.Status.TaskRuns, tt.expectedTaskRuns); d != "" {
t.Fatalf("Expected PipelineRunTaskRun status to match TaskRun(s) status, but got a mismatch for %s: %s", tt.name, d)
}
} else if tt.pipelineRunStatusUnknown {
// This PipelineRun should still be running and the status should reflect that
if !reconciledRun.Status.GetCondition(apis.ConditionSucceeded).IsUnknown() {
Expand All @@ -5872,15 +5890,48 @@ func TestReconcilePipeline_FinalTasks(t *testing.T) {
if d := cmp.Diff(reconciledRun.Status.ChildReferences, tt.expectedChildReferences); d != "" {
t.Fatalf("Expected PipelineRunTaskRun status to match TaskRun(s) status, but got a mismatch for %s: %s", tt.name, d)
}
compareTaskRunStatusFromChildRefs(prt.TestAssets.Ctx, t, namespace, clients, reconciledRun.Status.ChildReferences, tt.expectedTaskRuns)
}

// if d := cmp.Diff(reconciledRun.Status.TaskRuns, tt.expectedChildReferences); d != "" {
// t.Fatalf("Expected PipelineRunTaskRun status to match TaskRun(s) status, but got a mismatch for %s: %s", tt.name, d)
// }
})
}
}

// TODO rename this
// compareTaskRunStatusFromChildRefs checks the status of taskruns from ChildReferences to be expected
// This function is introduced during the trasition of using TaskRuns/Runs to using ChildReferences while
// removing the according to the deprecation of `PipelineRunStatus` in TEP0100.
// Question: make sure the assumptions hold that childRefs len shall be the same as expectedTRs
func compareTaskRunStatusFromChildRefs(ctx context.Context, t *testing.T, namespace string, clients test.Clients,
childRefs []v1beta1.ChildStatusReference, expectedTaskRuns map[string]*v1beta1.PipelineRunTaskRunStatus) {
t.Helper()
if len(expectedTaskRuns) == 0 {
return
}

for _, childRef := range childRefs {
if childRef.Kind != "TaskRun" {
continue
}
trName := childRef.Name

trFromChildRef, err := clients.Pipeline.TektonV1beta1().TaskRuns(namespace).Get(ctx, trName, metav1.GetOptions{})
if err != nil {
t.Fatalf("Failure to get TaskRun from ChildReference %s: %s", childRef.Name, err)
}

if expectedTaskRunStatus, ok := expectedTaskRuns[trName]; !ok {
t.Fatalf("Expected not to have TaskRun status from ChildReferences %s", trName)
} else if d := cmp.Diff(*expectedTaskRunStatus.Status, trFromChildRef.Status); d != "" {
t.Fatalf("Expected TaskRun Status Conditions to match ChildReferences TaskRun Status Condition, but got a mismatch %s", diff.PrintWantGot(d))
}
delete(expectedTaskRuns, trName)
}

if len(expectedTaskRuns) != 0 {
t.Fatalf("Expected ChildReferences to fully match TaskRun Status, but there are TaskRuns that are not %v", expectedTaskRuns)
}
}

func getPipelineRun(pr, p string, status corev1.ConditionStatus, reason string, m string, tr map[string]string) []*v1beta1.PipelineRun {
pRun := &v1beta1.PipelineRun{
ObjectMeta: baseObjectMeta(pr, "foo"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -927,7 +927,7 @@ pipelineTaskName: task-6

for _, tc := range tcs {

t.Run(fmt.Sprintf("%s", tc.prName), func(t *testing.T) {
t.Run(tc.prName, func(t *testing.T) {
ctx := context.Background()
cfg := config.NewStore(logtesting.TestLogger(t))

Expand Down
32 changes: 6 additions & 26 deletions test/conversion_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -656,30 +656,11 @@ status:
- name: "fetch-and-write-secure"
image: "ubuntu"
script: "echo hello"
taskRuns:
%s-fetch-secure-data:
childReferences:
- name: %s-fetch-secure-data
pipelineTaskName: fetch-secure-data
status:
conditions:
- reason: Succeeded
status: "True"
type: Succeeded
podName: %s-fetch-secure-data-pod
steps:
- container: step-fetch-and-write-secure
imageID: docker.io/library/ubuntu@sha256:4b1d0c4a2d2aaf63b37111f34eb9fa89fa1bf53dd6e4ca954d47caebca4005c2
name: fetch-and-write-secure
terminated:
containerID: containerd://07b57fc6fd515e6d1d0de27149c60be9149697207aedc130dd0d284fce6df3fd
exitCode: 0
finishedAt: "2022-12-07T15:56:32Z"
reason: Completed
startedAt: "2022-12-07T15:56:32Z"
taskSpec:
steps:
- name: "fetch-and-write-secure"
image: "ubuntu"
script: "echo hello"
apiVersion: tekton.dev/v1beta1
kind: TaskRun
`

v1PipelineRunYaml = `
Expand Down Expand Up @@ -755,9 +736,8 @@ status:
image: "ubuntu"
script: "echo hello"
childReferences:
- typeMeta:
- apiVersion: tekton.dev/v1beta1
kind: TaskRun
apiVersion: tekton.dev/v1beta1
name: %s-fetch-secure-data
pipelineTaskName: fetch-secure-data
`
Expand Down Expand Up @@ -986,7 +966,7 @@ func TestPipelineRunCRDConversion(t *testing.T) {

v1ToV1beta1PRName := helpers.ObjectNameForTest(t)
v1PipelineRun := parse.MustParseV1PipelineRun(t, fmt.Sprintf(v1PipelineRunYaml, v1ToV1beta1PRName, namespace))
v1beta1PipelineRunExpected := parse.MustParseV1beta1PipelineRun(t, fmt.Sprintf(v1beta1PipelineRunExpectedYaml, v1ToV1beta1PRName, namespace, v1ToV1beta1PRName, v1ToV1beta1PRName))
v1beta1PipelineRunExpected := parse.MustParseV1beta1PipelineRun(t, fmt.Sprintf(v1beta1PipelineRunExpectedYaml, v1ToV1beta1PRName, namespace, v1ToV1beta1PRName))

if _, err := c.V1PipelineRunClient.Create(ctx, v1PipelineRun, metav1.CreateOptions{}); err != nil {
t.Fatalf("Failed to create v1 PipelineRun: %s", err)
Expand Down

0 comments on commit 5980bc4

Please sign in to comment.