test(sort): add more unit tests for pr sorting

This commit addresses the TODO in `pkg/sort/pipelinerun_test.go`
to add more test cases and improves the overall test coverage for
the sorting logic.

- Adds a new test function `TestPipelineRunSortByCompletionTime_missing`
  to cover more scenarios for sorting by completion time:
  - An empty list of PipelineRuns.
  - PipelineRuns with the same completion time.
  - A PipelineRun with a nil completion time.
- Fixes a failing test case in `TestPipelineRunSortByStartTime` by
  correctly setting up the test data for a PipelineRun that has not
  started.
- Removes the now-obsolete TODO comment.

Fixes #2196

Signed-off-by: Chmouel Boudjnah <[email protected]>

Author: chmouel

Makefile

@@ -74,7 +74,7 @@ test-e2e:  test-e2e-cleanup ## run e2e tests
 .PHONY: html-coverage
 html-coverage: ## generate html coverage
 	@mkdir -p tmp
-	@go test -coverprofile=tmp/c.out ./.../ && go tool cover -html=tmp/c.out
+	@go test -coverprofile=tmp/c.out ./pkg/... ./cmd/... && go tool cover -html=tmp/c.out
 
 ##@ Linting
 .PHONY: lint

pkg/sort/pipelinerun_test.go

@@ -9,33 +9,112 @@ import (
 	tektonv1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1"
 	"gotest.tools/v3/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"knative.dev/pkg/apis"
+	duckv1 "knative.dev/pkg/apis/duck/v1"
 )
 
 func TestPipelineRunSortByCompletionTime(t *testing.T) {
 	clock := clockwork.NewFakeClock()
 	ns := "namespace"
 	labels := map[string]string{}
 	success := tektonv1.PipelineRunReasonSuccessful.String()
+
+	pruns := []tektonv1.PipelineRun{
+		*(tektontest.MakePRCompletion(clock, "troisieme", ns, success, nil, labels, 30)),
+		*(tektontest.MakePRCompletion(clock, "premier", ns, success, nil, labels, 10)),
+		*(tektontest.MakePRCompletion(clock, "second", ns, success, nil, labels, 20)),
+	}
+
+	noCompletionTimePR := tektonv1.PipelineRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "no-completion-time",
+			Namespace: ns,
+			Labels:    labels,
+		},
+		Status: tektonv1.PipelineRunStatus{
+			Status: duckv1.Status{
+				Conditions: duckv1.Conditions{
+					{
+						Type:   apis.ConditionSucceeded,
+						Status: "True",
+						Reason: success,
+					},
+				},
+			},
+		},
+	}
+
+	prunsMissing := []tektonv1.PipelineRun{
+		*(tektontest.MakePRCompletion(clock, "troisieme", ns, success, nil, labels, 30)),
+		*(tektontest.MakePRCompletion(clock, "premier", ns, success, nil, labels, 10)),
+		noCompletionTimePR,
+	}
+
+	prunsWithOneMissing := []tektonv1.PipelineRun{
+		*(tektontest.MakePRCompletion(clock, "premier", ns, success, nil, labels, 10)),
+		noCompletionTimePR,
+	}
+
+	prunsWithUncompletedFirst := []tektonv1.PipelineRun{
+		noCompletionTimePR,
+		*(tektontest.MakePRCompletion(clock, "premier", ns, success, nil, labels, 10)),
+	}
+
 	tests := []struct {
 		name     string
 		pruns    []tektonv1.PipelineRun
 		wantName []string
 	}{
 		{
+			name:     "sort by completion time",
+			pruns:    pruns,
+			wantName: []string{"premier", "second", "troisieme"},
+		},
+		{
+			name:     "sort by completion time with missing",
+			pruns:    prunsMissing,
+			wantName: []string{"no-completion-time", "premier", "troisieme"},
+		},
+		{
+			name:     "sort by completion time with one missing",
+			pruns:    prunsWithOneMissing,
+			wantName: []string{"no-completion-time", "premier"},
+		},
+		{
+			name:     "sort with uncompleted item first",
+			pruns:    prunsWithUncompletedFirst,
+			wantName: []string{"no-completion-time", "premier"},
+		},
+		{
+			name:     "empty list",
+			pruns:    []tektonv1.PipelineRun{},
+			wantName: []string{},
+		},
+		{
+			name:     "single item",
+			pruns:    []tektonv1.PipelineRun{*(tektontest.MakePRCompletion(clock, "premier", ns, success, nil, labels, 10))},
+			wantName: []string{"premier"},
+		},
+		{
+			name: "PipelineRuns with same completion time",
 			pruns: []tektonv1.PipelineRun{
-				*(tektontest.MakePRCompletion(clock, "troisieme", ns, success, nil, labels, 30)),
-				*(tektontest.MakePRCompletion(clock, "premier", ns, success, nil, labels, 10)),
-				*(tektontest.MakePRCompletion(clock, "second", ns, success, nil, labels, 20)),
+				*(tektontest.MakePRCompletion(clock, "first-same-time", ns, success, nil, labels, 15)),
+				*(tektontest.MakePRCompletion(clock, "second-same-time", ns, success, nil, labels, 15)),
+				*(tektontest.MakePRCompletion(clock, "third-same-time", ns, success, nil, labels, 15)),
+				*(tektontest.MakePRCompletion(clock, "earlier", ns, success, nil, labels, 10)),
+				*(tektontest.MakePRCompletion(clock, "later", ns, success, nil, labels, 20)),
 			},
-			wantName: []string{"premier", "second", "troisieme"},
+			wantName: []string{"earlier", "first-same-time", "second-same-time", "third-same-time", "later"},
 		},
-		// TODO: Add test cases.
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			for key, value := range PipelineRunSortByCompletionTime(tt.pruns) {
-				assert.Equal(t, tt.wantName[key], value.GetName())
+			got := PipelineRunSortByCompletionTime(tt.pruns)
+			gotNames := make([]string, len(got))
+			for i, pr := range got {
+				gotNames[i] = pr.GetName()
 			}
+			assert.DeepEqual(t, tt.wantName, gotNames)
 		})
 	}
 }
@@ -53,8 +132,18 @@ func TestPipelineRunSortByStartTime(t *testing.T) {
 	noCompletionPR.Status.CompletionTime = nil
 
 	notStartedYet := tektontest.MakePRCompletion(clock, "notStarted", ns, success, nil, labels, 5)
-	noCompletionPR.Status.StartTime = nil
-	noCompletionPR.Status.CompletionTime = nil
+	notStartedYet.Status.StartTime = nil
+	notStartedYet.Status.CompletionTime = nil
+
+	prunsWithOneNotStarted := []tektonv1.PipelineRun{
+		*(tektontest.MakePRCompletion(clock, "otherFirst", ns, success, nil, labels, 30)),
+		*notStartedYet,
+	}
+
+	prunsWithNotStartedFirst := []tektonv1.PipelineRun{
+		*notStartedYet,
+		*(tektontest.MakePRCompletion(clock, "otherFirst", ns, success, nil, labels, 30)),
+	}
 
 	tests := []struct {
 		name     string
@@ -83,19 +172,41 @@ func TestPipelineRunSortByStartTime(t *testing.T) {
 		{
 			name: "not started yet",
 			pruns: []tektonv1.PipelineRun{
-				*notStartedYet,
 				*(tektontest.MakePRCompletion(clock, "otherFirst", ns, success, nil, labels, 30)),
 				*(tektontest.MakePRCompletion(clock, "otherSecond", ns, success, nil, labels, 10)),
+				*notStartedYet,
 			},
-			wantName: []string{"otherFirst", "otherSecond", "notStarted"},
+			wantName: []string{"notStarted", "otherFirst", "otherSecond"},
+		},
+		{
+			name:     "not started yet single",
+			pruns:    prunsWithOneNotStarted,
+			wantName: []string{"notStarted", "otherFirst"},
+		},
+		{
+			name:     "sort with not-started item first",
+			pruns:    prunsWithNotStartedFirst,
+			wantName: []string{"notStarted", "otherFirst"},
+		},
+		{
+			name:     "empty list",
+			pruns:    []tektonv1.PipelineRun{},
+			wantName: []string{},
+		},
+		{
+			name:     "single item",
+			pruns:    []tektonv1.PipelineRun{*startedEarlierPR},
+			wantName: []string{"earlier"},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			PipelineRunSortByStartTime(tt.pruns)
-			for key, value := range tt.pruns {
-				assert.Equal(t, tt.wantName[key], value.GetName())
+			gotNames := make([]string, len(tt.pruns))
+			for i, pr := range tt.pruns {
+				gotNames[i] = pr.GetName()
 			}
+			assert.DeepEqual(t, tt.wantName, gotNames)
 		})
 	}
 }