Skip to content

Commit

Permalink
Switch to cmpopts.SortSlices rather than explicitly sorting in test…
Browse files Browse the repository at this point in the history 
…s when possible

There are a couple cases where I left explicit sorts in place because the test wasn't using `cmp.Diff` in the first place, etc, but most are changed.

Signed-off-by: Andrew Bayer <[email protected]>
  • Loading branch information
@abayer @tekton-robot
abayer authored and tekton-robot committed Jun 22, 2022
1 parent 13a3c5d commit a84f97e
Show file tree
Hide file tree
Showing 7 changed files with 55 additions and 86 deletions.
11 changes: 5 additions & 6 deletions pkg/apis/pipeline/v1beta1/resultref_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ limitations under the License.
package v1beta1_test

import (
"sort"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/test/diff"
"k8s.io/apimachinery/pkg/selection"
Expand Down Expand Up @@ -277,16 +277,15 @@ func TestHasResultReference(t *testing.T) {
t.Fatalf("expected to find expressions but didn't find any")
}
got := v1beta1.NewResultRefs(expressions)
sort.Slice(got, func(i, j int) bool {
if got[i].PipelineTask > got[j].PipelineTask {
if d := cmp.Diff(tt.wantRef, got, cmpopts.SortSlices(func(i, j *v1beta1.ResultRef) bool {
if i.PipelineTask > j.PipelineTask {
return false
}
if got[i].Result > got[j].Result {
if i.Result > j.Result {
return false
}
return true
})
if d := cmp.Diff(tt.wantRef, got); d != "" {
})); d != "" {
t.Error(diff.PrintWantGot(d))
}
})
Expand Down
16 changes: 5 additions & 11 deletions pkg/pullrequest/disk_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,11 @@ import (
"net/url"
"os"
"path/filepath"
"reflect"
"sort"
"strconv"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/jenkins-x/go-scm/scm"
"github.com/tektoncd/pipeline/test/diff"
)
Expand Down Expand Up @@ -725,15 +724,10 @@ func TestLabelsFromDisk(t *testing.T) {
derefed = append(derefed, *l)
}

sort.Slice(derefed, func(i, j int) bool {
return derefed[i].Name < derefed[j].Name
})
sort.Slice(tt.want, func(i, j int) bool {
return tt.want[i].Name < tt.want[j].Name
})

if !reflect.DeepEqual(derefed, tt.want) {
t.Errorf("labelsFromDisk() = %v, want %v", derefed, tt.want)
if d := cmp.Diff(tt.want, derefed, cmpopts.SortSlices(func(i, j scm.Label) bool {
return i.Name < j.Name
})); d != "" {
t.Errorf("expected labelsFromDisk() to match %#v. Diff %s", derefed, diff.PrintWantGot(d))
}
})
}
Expand Down
10 changes: 7 additions & 3 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,7 @@ spec:
timeout: 1h0m0s
`)
// ignore IgnoreUnexported ignore both after and before steps fields
if d := cmp.Diff(expectedTaskRun, actual, ignoreTypeMeta, cmpopts.SortSlices(func(x, y v1beta1.TaskResourceBinding) bool { return x.Name < y.Name })); d != "" {
if d := cmp.Diff(expectedTaskRun, actual, ignoreTypeMeta, cmpopts.SortSlices(lessTaskResourceBindings)); d != "" {
t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRun, diff.PrintWantGot(d))
}
// test taskrun is able to recreate correct pipeline-pvc-name
Expand Down Expand Up @@ -6441,7 +6441,7 @@ spec:
timeout: 1h0m0s
`, ref))

if d := cmp.Diff(expectedTaskRun, actual, ignoreTypeMeta, cmpopts.SortSlices(func(x, y v1beta1.TaskResourceBinding) bool { return x.Name < y.Name })); d != "" {
if d := cmp.Diff(expectedTaskRun, actual, ignoreTypeMeta, cmpopts.SortSlices(lessTaskResourceBindings)); d != "" {
t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRun, diff.PrintWantGot(d))
}

Expand Down Expand Up @@ -6547,7 +6547,7 @@ spec:
timeout: 1h0m0s
`)

if d := cmp.Diff(expectedTaskRun, actual, ignoreTypeMeta, cmpopts.SortSlices(func(x, y v1beta1.TaskResourceBinding) bool { return x.Name < y.Name })); d != "" {
if d := cmp.Diff(expectedTaskRun, actual, ignoreTypeMeta, cmpopts.SortSlices(lessTaskResourceBindings)); d != "" {
t.Errorf("expected to see TaskRun %v created. Diff %s", expectedTaskRun, diff.PrintWantGot(d))
}

Expand Down Expand Up @@ -7749,3 +7749,7 @@ spec:
})
}
}

func lessTaskResourceBindings(i, j v1beta1.TaskResourceBinding) bool {
return i.Name < j.Name
}
27 changes: 7 additions & 20 deletions pkg/reconciler/pipelinerun/pipelinerun_updatestatus_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ import (
"context"
"fmt"
"regexp"
"sort"
"strings"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/tektoncd/pipeline/pkg/apis/config"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
Expand Down Expand Up @@ -788,15 +788,7 @@ pipelineTaskName: task
actualPrStatus.ChildReferences = fixedChildRefs
}

// Sort the ChildReferences to deal with annoying ordering issues.
sort.Slice(actualPrStatus.ChildReferences, func(i, j int) bool {
return actualPrStatus.ChildReferences[i].Name < actualPrStatus.ChildReferences[j].Name
})
sort.Slice(tc.expectedPrStatus.ChildReferences, func(i, j int) bool {
return tc.expectedPrStatus.ChildReferences[i].Name < tc.expectedPrStatus.ChildReferences[j].Name
})

if d := cmp.Diff(tc.expectedPrStatus, actualPrStatus); d != "" {
if d := cmp.Diff(tc.expectedPrStatus, actualPrStatus, cmpopts.SortSlices(lessChildReferences)); d != "" {
t.Errorf("expected the PipelineRun status to match %#v. Diff %s", tc.expectedPrStatus, diff.PrintWantGot(d))
}
})
Expand Down Expand Up @@ -950,14 +942,9 @@ metadata:
actualPrStatus.ChildReferences = fixedChildRefs
}

// Sort the ChildReferences to deal with annoying ordering issues.
sort.Slice(actualPrStatus.ChildReferences, func(i, j int) bool {
return actualPrStatus.ChildReferences[i].PipelineTaskName < actualPrStatus.ChildReferences[j].PipelineTaskName
})

expectedPRStatus := prStatusFromInputs(embeddedVal, prRunningStatus, tc.expectedStatusTRs, tc.expectedStatusRuns, tc.expectedStatusCRs)

if d := cmp.Diff(expectedPRStatus, actualPrStatus); d != "" {
if d := cmp.Diff(expectedPRStatus, actualPrStatus, cmpopts.SortSlices(lessChildReferences)); d != "" {
t.Errorf("expected the PipelineRun status to match %#v. Diff %s", expectedPRStatus, diff.PrintWantGot(d))
}
})
Expand Down Expand Up @@ -1197,10 +1184,6 @@ func prStatusFromInputs(embeddedStatus string, status duckv1beta1.Status, taskRu
}
if shouldHaveMinimalEmbeddedStatus(embeddedStatus) {
prs.ChildReferences = append(prs.ChildReferences, childRefs...)
// Sort the ChildReferences to deal with annoying ordering issues.
sort.Slice(prs.ChildReferences, func(i, j int) bool {
return prs.ChildReferences[i].PipelineTaskName < prs.ChildReferences[j].PipelineTaskName
})
}

return prs
Expand Down Expand Up @@ -1303,3 +1286,7 @@ func mustParseChildStatusReference(t *testing.T, yamlStr string) v1beta1.ChildSt
}
return output
}

func lessChildReferences(i, j v1beta1.ChildStatusReference) bool {
return i.Name < j.Name
}
18 changes: 8 additions & 10 deletions pkg/reconciler/pipelinerun/resources/input_output_steps_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ limitations under the License.
package resources_test

import (
"sort"
"testing"

"github.com/google/go-cmp/cmp"
Expand Down Expand Up @@ -124,8 +123,7 @@ func TestGetOutputSteps(t *testing.T) {
for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
postTasks := resources.GetOutputSteps(tc.outputs, tc.pipelineTaskName, pvcDir)
sort.SliceStable(postTasks, func(i, j int) bool { return postTasks[i].Name < postTasks[j].Name })
if d := cmp.Diff(postTasks, tc.expectedtaskOuputResources); d != "" {
if d := cmp.Diff(tc.expectedtaskOuputResources, postTasks, cmpopts.SortSlices(lessTaskResourceBindings)); d != "" {
t.Errorf("error comparing post steps %s", diff.PrintWantGot(d))
}
})
Expand Down Expand Up @@ -266,8 +264,7 @@ func TestGetInputSteps(t *testing.T) {
for _, tc := range tcs {
t.Run(tc.name, func(t *testing.T) {
taskInputResources := resources.GetInputSteps(tc.inputs, tc.pipelineTask.Resources.Inputs, pvcDir)
sort.SliceStable(taskInputResources, func(i, j int) bool { return taskInputResources[i].Name < taskInputResources[j].Name })
if d := cmp.Diff(tc.expectedtaskInputResources, taskInputResources); d != "" {
if d := cmp.Diff(tc.expectedtaskInputResources, taskInputResources, cmpopts.SortSlices(lessTaskResourceBindings)); d != "" {
t.Errorf("error comparing task resource inputs %s", diff.PrintWantGot(d))
}

Expand Down Expand Up @@ -346,13 +343,14 @@ func TestWrapSteps(t *testing.T) {
Paths: []string{"/pvc/test-task/test-output-2"},
}}

sort.SliceStable(expectedtaskInputResources, func(i, j int) bool { return expectedtaskInputResources[i].Name < expectedtaskInputResources[j].Name })
sort.SliceStable(expectedtaskOuputResources, func(i, j int) bool { return expectedtaskOuputResources[i].Name < expectedtaskOuputResources[j].Name })

if d := cmp.Diff(taskRunSpec.Resources.Inputs, expectedtaskInputResources, cmpopts.SortSlices(func(x, y v1beta1.TaskResourceBinding) bool { return x.Name < y.Name })); d != "" {
if d := cmp.Diff(taskRunSpec.Resources.Inputs, expectedtaskInputResources, cmpopts.SortSlices(lessTaskResourceBindings)); d != "" {
t.Errorf("error comparing input resources %s", diff.PrintWantGot(d))
}
if d := cmp.Diff(taskRunSpec.Resources.Outputs, expectedtaskOuputResources, cmpopts.SortSlices(func(x, y v1beta1.TaskResourceBinding) bool { return x.Name < y.Name })); d != "" {
if d := cmp.Diff(taskRunSpec.Resources.Outputs, expectedtaskOuputResources, cmpopts.SortSlices(lessTaskResourceBindings)); d != "" {
t.Errorf("error comparing output resources %s", diff.PrintWantGot(d))
}
}

func lessTaskResourceBindings(i, j v1beta1.TaskResourceBinding) bool {
return i.Name < j.Name
}
42 changes: 16 additions & 26 deletions pkg/reconciler/pipelinerun/resources/resultrefresolution_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@ import (
"strings"
"testing"

duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/test/diff"
Expand All @@ -17,6 +16,7 @@ import (
"k8s.io/apimachinery/pkg/selection"
"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1"
)

var (
Expand Down Expand Up @@ -614,22 +614,11 @@ func TestResolveResultRefs(t *testing.T) {
}} {
t.Run(tt.name, func(t *testing.T) {
got, pt, err := ResolveResultRefs(tt.pipelineRunState, tt.targets)
sort.SliceStable(got, func(i, j int) bool {
fromI := got[i].FromTaskRun
if fromI == "" {
fromI = got[i].FromRun
}
fromJ := got[j].FromTaskRun
if fromJ == "" {
fromJ = got[j].FromRun
}
return strings.Compare(fromI, fromJ) < 0
})
if (err != nil) != tt.wantErr {
t.Errorf("ResolveResultRefs() error = %v, wantErr %v", err, tt.wantErr)
return
}
if d := cmp.Diff(tt.want, got); d != "" {
if d := cmp.Diff(tt.want, got, cmpopts.SortSlices(lessResolvedResultRefs)); d != "" {
t.Fatalf("ResolveResultRef %s", diff.PrintWantGot(d))
}
if d := cmp.Diff(tt.wantPt, pt); d != "" {
Expand Down Expand Up @@ -709,22 +698,11 @@ func TestResolveResultRef(t *testing.T) {
}} {
t.Run(tt.name, func(t *testing.T) {
got, pt, err := ResolveResultRef(tt.pipelineRunState, tt.target)
sort.SliceStable(got, func(i, j int) bool {
fromI := got[i].FromTaskRun
if fromI == "" {
fromI = got[i].FromRun
}
fromJ := got[j].FromTaskRun
if fromJ == "" {
fromJ = got[j].FromRun
}
return strings.Compare(fromI, fromJ) < 0
})
if (err != nil) != tt.wantErr {
t.Errorf("ResolveResultRefs() error = %v, wantErr %v", err, tt.wantErr)
return
}
if d := cmp.Diff(tt.want, got); d != "" {
if d := cmp.Diff(tt.want, got, cmpopts.SortSlices(lessResolvedResultRefs)); d != "" {
t.Fatalf("ResolveResultRef %s", diff.PrintWantGot(d))
}
if d := cmp.Diff(tt.wantPt, pt); d != "" {
Expand All @@ -733,3 +711,15 @@ func TestResolveResultRef(t *testing.T) {
})
}
}

func lessResolvedResultRefs(i, j *ResolvedResultRef) bool {
fromI := i.FromTaskRun
if fromI == "" {
fromI = i.FromRun
}
fromJ := j.FromTaskRun
if fromJ == "" {
fromJ = j.FromRun
}
return strings.Compare(fromI, fromJ) < 0
}
17 changes: 7 additions & 10 deletions test/pipelinefinally_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,15 @@ import (
"context"
"encoding/json"
"fmt"
"sort"
"strings"
"testing"

"github.com/tektoncd/pipeline/test/parse"

"github.com/google/go-cmp/cmp"
"github.com/tektoncd/pipeline/test/diff"

"github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun/resources"

"github.com/google/go-cmp/cmp/cmpopts"
"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
"github.com/tektoncd/pipeline/pkg/reconciler/pipelinerun/resources"
"github.com/tektoncd/pipeline/test/diff"
"github.com/tektoncd/pipeline/test/parse"
jsonpatch "gomodules.xyz/jsonpatch/v2"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -391,9 +388,9 @@ spec:

actualSkippedTasks := pr.Status.SkippedTasks
// Sort tasks based on their names to get similar order as in expected list
sort.Slice(actualSkippedTasks, func(i int, j int) bool { return actualSkippedTasks[i].Name < actualSkippedTasks[j].Name })

if d := cmp.Diff(actualSkippedTasks, expectedSkippedTasks); d != "" {
if d := cmp.Diff(actualSkippedTasks, expectedSkippedTasks, cmpopts.SortSlices(func(i, j v1beta1.SkippedTask) bool {
return i.Name < j.Name
})); d != "" {
t.Fatalf("Expected four skipped tasks, dag task with condition failure dagtask3, dag task with when expression,"+
"two final tasks with missing result reference finaltaskconsumingdagtask1 and finaltaskconsumingdagtask4 in SkippedTasks."+
" Diff: %s", diff.PrintWantGot(d))
Expand Down

0 comments on commit a84f97e

Please sign in to comment.