Make cmp.Diff want vs got usage clear 🚰

While working on tektoncd#1417 (which we decided not to go ahead with) I made a
_lot_ of unit tests fail. B/c I was changing how steps were added (for
the volume resource) these failures were often in the middle of big
diffs. It turns out we were inconsistent about how we used cmp.Diff:
anything that is in the first argument but not in the second gets a `-`
before it, anything that is in the second argument but not in the first
gets a `+`, which is why the example at
https://godoc.org/github.com/google/go-cmp/cmp#Diff passes these
arguments as `cmp.Diff(want, got)`. If we reverse them, which many of
our tests did, the results are counter intuitive: the "extra" stuff gets
a `-` and the missing stuff gets a `+`. It seems like a small thing but
it's really hard to wrap your head around! So this commit takes a bunch
of these tests (not all of them, just the ones I was touchin in tektoncd#1417)
and:
- Makes them consistently pass `(want, got)`
- Annotates the failure message with a legend for `-` and `+`
- Adds more details to the messages when applicable so you know what the
  diff is that you're looking at

pkg/apis/pipeline/v1alpha1/pipelineresource_validation_test.go

@@ -119,8 +119,8 @@ func TestResourceValidation_Invalid(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			err := tt.res.Validate(context.Background())
-			if d := cmp.Diff(err.Error(), tt.want.Error()); d != "" {
-				t.Errorf("PipelineResource.Validate/%s (-want, +got) = %v", tt.name, d)
+			if d := cmp.Diff(tt.want.Error(), err.Error()); d != "" {
+				t.Errorf("Didn't get expected error for %s (-want, +got) = %v", tt.name, d)
 			}
 		})
 	}

pkg/reconciler/taskrun/resources/input_resource_test.go

@@ -968,8 +968,8 @@ func TestStorageInputResource(t *testing.T) {
 			if (err != nil) != c.wantErr {
 				t.Errorf("Test: %q; AddInputResource() error = %v, WantErr %v", c.desc, err, c.wantErr)
 			}
-			if d := cmp.Diff(got, c.want); d != "" {
-				t.Errorf("Diff:\n%s", d)
+			if d := cmp.Diff(c.want, got); d != "" {
+				t.Errorf("Didn't get expected Task spec (-want, +got): %s", d)
 			}
 		})
 	}
@@ -1209,8 +1209,8 @@ func TestAddStepsToTaskWithBucketFromConfigMap(t *testing.T) {
 			if err != nil {
 				t.Errorf("Test: %q; AddInputResource() error = %v", c.desc, err)
 			}
-			if d := cmp.Diff(got, c.want); d != "" {
-				t.Errorf("Diff:\n%s", d)
+			if d := cmp.Diff(c.want, got); d != "" {
+				t.Errorf("Didn't get expected TaskSpec (-want, +got): %s", d)
 			}
 		})
 	}

pkg/reconciler/taskrun/resources/output_resource_test.go

@@ -795,8 +795,8 @@ func TestValidOutputResources(t *testing.T) {
 			}
 
 			if got != nil {
-				if d := cmp.Diff(got.Steps, c.wantSteps); d != "" {
-					t.Fatalf("post build steps mismatch: %s", d)
+				if d := cmp.Diff(c.wantSteps, got.Steps); d != "" {
+					t.Fatalf("post build steps mismatch (-want, +got): %s", d)
 				}
 
 				if c.taskRun.GetPipelineRunPVCName() != "" {
@@ -812,8 +812,8 @@ func TestValidOutputResources(t *testing.T) {
 						},
 					)
 				}
-				if d := cmp.Diff(got.Volumes, c.wantVolumes); d != "" {
-					t.Fatalf("post build steps volumes mismatch: %s", d)
+				if d := cmp.Diff(c.wantVolumes, got.Volumes); d != "" {
+					t.Fatalf("post build steps volumes mismatch (-want, +got): %s", d)
 				}
 			}
 		})
@@ -1010,8 +1010,8 @@ func TestValidOutputResourcesWithBucketStorage(t *testing.T) {
 				t.Fatalf("Failed to declare output resources for test name %q ; test description %q: error %v", c.name, c.desc, err)
 			}
 			if got != nil {
-				if d := cmp.Diff(got.Steps, c.wantSteps); d != "" {
-					t.Fatalf("post build steps mismatch: %s", d)
+				if d := cmp.Diff(c.wantSteps, got.Steps); d != "" {
+					t.Fatalf("post build steps mismatch (-want, got): %s", d)
 				}
 			}
 		})

pkg/reconciler/taskrun/taskrun_test.go

@@ -426,12 +426,12 @@ func TestReconcile_ExplicitDefaultSA(t *testing.T) {
 				t.Fatalf("Failed to fetch build pod: %v", err)
 			}
 
-			if d := cmp.Diff(pod.ObjectMeta, tc.wantPod.ObjectMeta, ignoreRandomPodNameSuffix); d != "" {
-				t.Errorf("Pod metadata doesn't match, diff: %s", d)
+			if d := cmp.Diff(tc.wantPod.ObjectMeta, pod.ObjectMeta, ignoreRandomPodNameSuffix); d != "" {
+				t.Errorf("Pod metadata doesn't match (-want, +got): %s", d)
 			}
 
-			if d := cmp.Diff(pod.Spec, tc.wantPod.Spec, resourceQuantityCmp); d != "" {
-				t.Errorf("Pod spec doesn't match, diff: %s", d)
+			if d := cmp.Diff(tc.wantPod.Spec, pod.Spec, resourceQuantityCmp); d != "" {
+				t.Errorf("Pod spec doesn't match, (-want, +got): %s", d)
 			}
 			if len(clients.Kube.Actions()) == 0 {
 				t.Fatalf("Expected actions to be logged in the kubeclient, got none")
@@ -1302,12 +1302,12 @@ func TestReconcile(t *testing.T) {
 				t.Fatalf("Failed to fetch build pod: %v", err)
 			}
 
-			if d := cmp.Diff(pod.ObjectMeta, tc.wantPod.ObjectMeta, ignoreRandomPodNameSuffix); d != "" {
-				t.Errorf("Pod metadata doesn't match, diff: %s", d)
+			if d := cmp.Diff(tc.wantPod.ObjectMeta, pod.ObjectMeta, ignoreRandomPodNameSuffix); d != "" {
+				t.Errorf("Pod metadata doesn't match (-want, +got): %s", d)
 			}
 
-			if d := cmp.Diff(pod.Spec, tc.wantPod.Spec, resourceQuantityCmp); d != "" {
-				t.Errorf("Pod spec doesn't match, diff: %s", d)
+			if d := cmp.Diff(tc.wantPod.Spec, pod.Spec, resourceQuantityCmp); d != "" {
+				t.Errorf("Pod spec doesn't match (-want, +got): %s", d)
 			}
 			if len(clients.Kube.Actions()) == 0 {
 				t.Fatalf("Expected actions to be logged in the kubeclient, got none")
@@ -1394,10 +1394,10 @@ func TestReconcile_SortTaskRunStatusSteps(t *testing.T) {
 	if err := testAssets.Controller.Reconciler.Reconcile(context.Background(), getRunName(taskRun)); err != nil {
 		t.Errorf("expected no error reconciling valid TaskRun but got %v", err)
 	}
-	verify_TaskRunStatusStep(t, taskRun)
+	verifyTaskRunStatusStep(t, taskRun)
 }
 
-func verify_TaskRunStatusStep(t *testing.T, taskRun *v1alpha1.TaskRun) {
+func verifyTaskRunStatusStep(t *testing.T, taskRun *v1alpha1.TaskRun) {
 	actualStepOrder := []string{}
 	for _, state := range taskRun.Status.Steps {
 		actualStepOrder = append(actualStepOrder, state.Name)
@@ -1408,8 +1408,8 @@ func verify_TaskRunStatusStep(t *testing.T, taskRun *v1alpha1.TaskRun) {
 	}
 	// Add a nop in the end. This may be removed in future.
 	expectedStepOrder = append(expectedStepOrder, "nop")
-	if d := cmp.Diff(actualStepOrder, expectedStepOrder); d != "" {
-		t.Errorf("The status steps in TaksRun doesn't match the spec steps in Task, diff: %s", d)
+	if d := cmp.Diff(expectedStepOrder, actualStepOrder); d != "" {
+		t.Errorf("The status steps in TaksRun doesn't match the spec steps in Task (-want, +got): %s", d)
 	}
 }
 
@@ -1573,13 +1573,13 @@ func TestReconcilePodUpdateStatus(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Expected TaskRun %s to exist but instead got error when getting it: %v", taskRun.Name, err)
 	}
-	if d := cmp.Diff(newTr.Status.GetCondition(apis.ConditionSucceeded), &apis.Condition{
+	if d := cmp.Diff(&apis.Condition{
 		Type:    apis.ConditionSucceeded,
 		Status:  corev1.ConditionUnknown,
 		Reason:  "Running",
 		Message: "Not all Steps in the Task have finished executing",
-	}, ignoreLastTransitionTime); d != "" {
-		t.Fatalf("-got, +want: %v", d)
+	}, newTr.Status.GetCondition(apis.ConditionSucceeded), ignoreLastTransitionTime); d != "" {
+		t.Fatalf("Did not get expected condition (-want, +got): %v", d)
 	}
 
 	// update pod status and trigger reconcile : build is completed
@@ -1597,13 +1597,13 @@ func TestReconcilePodUpdateStatus(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Unexpected error fetching taskrun: %v", err)
 	}
-	if d := cmp.Diff(newTr.Status.GetCondition(apis.ConditionSucceeded), &apis.Condition{
+	if d := cmp.Diff(&apis.Condition{
 		Type:    apis.ConditionSucceeded,
 		Status:  corev1.ConditionTrue,
 		Reason:  status.ReasonSucceeded,
 		Message: "All Steps have completed executing",
-	}, ignoreLastTransitionTime); d != "" {
-		t.Errorf("Taskrun Status diff -got, +want: %v", d)
+	},newTr.Status.GetCondition(apis.ConditionSucceeded), ignoreLastTransitionTime); d != "" {
+		t.Errorf("Did not get expected condition (-want, +got): %v", d)
 	}
 }
 
@@ -1665,7 +1665,7 @@ func TestReconcileOnCompletedTaskRun(t *testing.T) {
 		t.Fatalf("Expected completed TaskRun %s to exist but instead got error when getting it: %v", taskRun.Name, err)
 	}
 	if d := cmp.Diff(taskSt, newTr.Status.GetCondition(apis.ConditionSucceeded), ignoreLastTransitionTime); d != "" {
-		t.Fatalf("-want, +got: %v", d)
+		t.Fatalf("Did not get expected conditon (-want, +got): %v", d)
 	}
 }
 
@@ -1702,7 +1702,7 @@ func TestReconcileOnCancelledTaskRun(t *testing.T) {
 		Message: `TaskRun "test-taskrun-run-cancelled" was cancelled`,
 	}
 	if d := cmp.Diff(expectedStatus, newTr.Status.GetCondition(apis.ConditionSucceeded), ignoreLastTransitionTime); d != "" {
-		t.Fatalf("-want, +got: %v", d)
+		t.Fatalf("Did not get expected condition (-want, +got): %v", d)
 	}
 }
 
@@ -1784,7 +1784,7 @@ func TestReconcileTimeouts(t *testing.T) {
 		}
 		condition := newTr.Status.GetCondition(apis.ConditionSucceeded)
 		if d := cmp.Diff(tc.expectedStatus, condition, ignoreLastTransitionTime); d != "" {
-			t.Fatalf("-want, +got: %v", d)
+			t.Fatalf("Did not get expected condition (-want, +got): %v", d)
 		}
 	}
 }
@@ -2080,8 +2080,8 @@ func TestUpdateTaskRunStatus_withValidJson(t *testing.T) {
 			if err := updateTaskRunStatusWithResourceResult(c.taskRun, c.podLog); err != nil {
 				t.Errorf("UpdateTaskRunStatusWithResourceResult failed with error: %s", err)
 			}
-			if d := cmp.Diff(c.taskRun.Status.ResourcesResult, c.want); d != "" {
-				t.Errorf("post build steps mismatch: %s", d)
+			if d := cmp.Diff(c.want, c.taskRun.Status.ResourcesResult); d != "" {
+				t.Errorf("post build steps mismatch (-want, +got): %s", d)
 			}
 		})
 	}
@@ -2145,8 +2145,8 @@ func TestUpdateTaskRunStatus_withInvalidJson(t *testing.T) {
 			if err := updateTaskRunStatusWithResourceResult(c.taskRun, c.podLog); err == nil {
 				t.Error("UpdateTaskRunStatusWithResourceResult expected to fail with error")
 			}
-			if d := cmp.Diff(c.taskRun.Status.ResourcesResult, c.want); d != "" {
-				t.Errorf("post build steps mismatch: %s", d)
+			if d := cmp.Diff(c.want, c.taskRun.Status.ResourcesResult); d != "" {
+				t.Errorf("post build steps mismatch (-want, +got): %s", d)
 			}
 		})
 	}