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,37 +9,151 @@ 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
 	}{
 		{
-			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)),
-			},
+			name:     "sort by completion time",
+			pruns:    pruns,
 			wantName: []string{"premier", "second", "troisieme"},
 		},
-		// TODO: Add test cases.
+		{
+			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"},
+		},
 	}
 	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)
 		})
 	}
 }
 
+func TestPipelineRunSortByCompletionTimeSameTime(t *testing.T) {
+	clock := clockwork.NewFakeClock()
+	ns := "namespace"
+	labels := map[string]string{}
+	success := tektonv1.PipelineRunReasonSuccessful.String()
+
+	pruns := []tektonv1.PipelineRun{
+		*(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)),
+	}
+
+	got := PipelineRunSortByCompletionTime(pruns)
+	gotNames := make([]string, len(got))
+	for i, pr := range got {
+		gotNames[i] = pr.GetName()
+	}
+
+	// Verify that "earlier" comes first
+	assert.Equal(t, "earlier", gotNames[0])
+	// Verify that "later" comes last
+	assert.Equal(t, "later", gotNames[len(gotNames)-1])
+	// Verify that the three items with same time are in positions 1-3 (any order)
+	sameTimeNames := gotNames[1:4]
+	expectedSameTime := map[string]bool{
+		"first-same-time":  false,
+		"second-same-time": false,
+		"third-same-time":  false,
+	}
+	for _, name := range sameTimeNames {
+		if _, exists := expectedSameTime[name]; exists {
+			expectedSameTime[name] = true
+		} else {
+			t.Errorf("Unexpected name %s in same-time group", name)
+		}
+	}
+	// Verify all expected names were found
+	for name, found := range expectedSameTime {
+		if !found {
+			t.Errorf("Expected name %s not found in same-time group", name)
+		}
+	}
+}
+
 func TestPipelineRunSortByStartTime(t *testing.T) {
 	clock := clockwork.NewFakeClock()
 	ns := "namespace"
@@ -53,8 +167,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 +207,101 @@ 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)
 		})
 	}
 }
+
+func TestPipelineRunSortByStartTimeSameTime(t *testing.T) {
+	clock := clockwork.NewFakeClock()
+	ns := "namespace"
+	labels := map[string]string{}
+	success := tektonv1.PipelineRunReasonSuccessful.String()
+
+	sameStartTime := clock.Now().Add(200 * time.Minute)
+
+	pr1 := tektontest.MakePRCompletion(clock, "first-same-start", ns, success, nil, labels, 5)
+	pr1.Status.StartTime = &metav1.Time{Time: sameStartTime}
+	pr1.Status.CompletionTime = &metav1.Time{Time: sameStartTime.Add(10 * time.Minute)}
+
+	pr2 := tektontest.MakePRCompletion(clock, "second-same-start", ns, success, nil, labels, 5)
+	pr2.Status.StartTime = &metav1.Time{Time: sameStartTime}
+	pr2.Status.CompletionTime = &metav1.Time{Time: sameStartTime.Add(15 * time.Minute)}
+
+	pr3 := tektontest.MakePRCompletion(clock, "third-same-start", ns, success, nil, labels, 5)
+	pr3.Status.StartTime = &metav1.Time{Time: sameStartTime}
+	pr3.Status.CompletionTime = &metav1.Time{Time: sameStartTime.Add(20 * time.Minute)}
+
+	earlierPR := tektontest.MakePRCompletion(clock, "started-earlier", ns, success, nil, labels, 5)
+	earlierPR.Status.StartTime = &metav1.Time{Time: sameStartTime.Add(-60 * time.Minute)}
+
+	laterPR := tektontest.MakePRCompletion(clock, "started-later", ns, success, nil, labels, 5)
+	laterPR.Status.StartTime = &metav1.Time{Time: sameStartTime.Add(60 * time.Minute)}
+
+	pruns := []tektonv1.PipelineRun{*pr1, *pr2, *pr3, *earlierPR, *laterPR}
+
+	PipelineRunSortByStartTime(pruns)
+	gotNames := make([]string, len(pruns))
+	for i, pr := range pruns {
+		gotNames[i] = pr.GetName()
+	}
+
+	// Verify that "started-later" comes first (since the sort puts later times first)
+	assert.Equal(t, "started-later", gotNames[0])
+	// Verify that "started-earlier" comes last
+	assert.Equal(t, "started-earlier", gotNames[len(gotNames)-1])
+	// Verify that the three items with same start time are in positions 1-3 (any order)
+	sameTimeNames := gotNames[1:4]
+	expectedSameTime := map[string]bool{
+		"first-same-start":  false,
+		"second-same-start": false,
+		"third-same-start":  false,
+	}
+	for _, name := range sameTimeNames {
+		if _, exists := expectedSameTime[name]; exists {
+			expectedSameTime[name] = true
+		} else {
+			t.Errorf("Unexpected name %s in same-start-time group", name)
+		}
+	}
+	// Verify all expected names were found
+	for name, found := range expectedSameTime {
+		if !found {
+			t.Errorf("Expected name %s not found in same-start-time group", name)
+		}
+	}
+}