Replace all the various (-want, +got) styles with a single format #2580

ghost · 2020-05-07T19:59:00Z

Changes

Many, many of our tests use cmp.Diff to test whether two objects
match. Every time that func gets used the test-writer has to remember
to include "(-want, +got)" in the string they print. This is annoying
to remember the order and format and can be easily implemented in a
helper func instead.

The first commit in this PR introduces a helper func to print diff messages in a
consistent way. This means test writers dont have to manually type
this formatting out every single time. The func is called diff.PrintWantGot().

The second commit in this PR replaces every single place where cmp.Diff output
is printed with a call to diff.PrintWantGot() so that diff formatting becomes consistent
everywhere.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Commit messages follow commit message best practices

See the contribution guide for more details.

bobcatfish · 2020-05-07T20:00:12Z

AMAZING

bobcatfish · 2020-05-07T20:52:58Z

test/diff/print.go

+
+// Print takes a diff string generated by cmp.Diff and returns it
+// in a consistent format for reuse across all of our tests.
+func Print(diff string) string {


one detail that might be important to mention the docstring or maybe even in the function name is that this expects that cmp.Diff has been called with the arguments in a specific order (want vs. got)

another option is to wrap cmp.diff entirely and also display the error - if you use https://golang.org/pkg/testing/#B.Helper you'll get very similar functionality to what we have now, e.g. something in the test like

diff.FailIfWantNotGot(tc.expectedTaskSpec, *ts, "Didn't get expected TaskSpec modifications")

And that function would actually call Errorf

This is straying dangerously into non-go-like territory tho, your way is clearer! I think it's clarify vs ease

I did consider wrapping cmp.Diff and I do think that is a very good idea. Two considerations that lead me away from doing that here:

Some tests use t.Fatalf while others use t.Errorf. So I figured a diff wrapping func would probably need to account for that too.

With an 80 file diff I wanted to do the very minimum to make all the various call sites uniform and limit the amount of review required.

I figure we can write a wrapping func for cmp.Diff in a future PR and then progressively replace cmp.Diff usage, perhaps over the course of multiple PRs? Excellent point about B.Helper, I did not know about it!

I really like the idea of a func with a name that points out the ordering ala ...WantGot(want, got); great idea!

Created #2587 to follow up.

OK, I've updated the PR to use diff.PrintWantGot as the name and I've updated the docstring to make explicit the expectation of ordering.

bobcatfish · 2020-05-07T20:56:01Z

/approve

pritidesai · 2020-05-08T00:20:26Z

wow, this is a huge cleanup, its always confusing to remind myself which one means want + or - 🤔. This will help a lot, thanks @sbwsg 👍

Build is failing on imports order 😢 :

-	"github.com/tektoncd/pipeline/test/names"
 	"github.com/tektoncd/pipeline/test/diff"
+	"github.com/tektoncd/pipeline/test/names"

pritidesai · 2020-05-08T00:32:19Z

/lgtm

Many, many of our tests use cmp.Diff to test whether two objects match. Every time that func gets used the test-writer has to remember to include "(-want, +got)" in the string they print. This is annoying to remember the order and format and can be easily implemented in a helper func instead. This commit introduces a helper func to print diff messages in a consistent way so that test writers dont have to manually type this formatting out every single time.

tekton-robot · 2020-05-08T13:07:40Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	87.5%	88.6%	1.1

In the previous commit a new func for printing consistent diff messages was added as a test helper. This commit updates all locations which use cmp.Diff to also use diff.PrintWantGot when displaying the diff.

ghost · 2020-05-08T13:09:47Z

Thanks for reviewing @pritidesai !

Build is failing on imports order 😢

Thank goodness for the Makefile @vdemeester introduced, I ran make goimports and it fixed them all (I hope!)

tekton-robot · 2020-05-08T13:12:10Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/pipelinerun/resources/resultrefresolution.go	87.5%	88.6%	1.1

ghost · 2020-05-08T14:31:01Z

/test pull-tekton-pipeline-integration-tests

imjasonh · 2020-05-08T23:18:18Z

Can we have a presubmit check to ensure I don't add a new cmp.Diff call that bypasses the helper? Otherwise I'm going to make sure we're going to need one of these PRs every few quarters 😅.

vdemeester · 2020-05-11T12:36:01Z

Can we have a presubmit check to ensure I don't add a new cmp.Diff call that bypasses the helper? Otherwise I'm going to make sure we're going to need one of these PRs every few quarters sweat_smile.

Let's do that in a follow-up 😉 #2594

vdemeester

/lgtm

tekton-robot · 2020-05-11T12:38:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [bobcatfish,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ghost added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 7, 2020

tekton-robot requested review from afrittoli and bobcatfish May 7, 2020 19:59

tekton-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 7, 2020

bobcatfish reviewed May 7, 2020

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 7, 2020

tekton-robot assigned pritidesai May 8, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 8, 2020

ghost mentioned this pull request May 8, 2020

Wrap cmp.Diff to make clear the (want, got) ordering of arguments. #2587

Closed

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label May 8, 2020

Replace varying (-want, +got) test messages with diff.PrintWantGot()

46b0e4b

In the previous commit a new func for printing consistent diff messages was added as a test helper. This commit updates all locations which use cmp.Diff to also use diff.PrintWantGot when displaying the diff.

vdemeester mentioned this pull request May 11, 2020

Add a check to ensure diff.PrintWantGot is used #2594

Closed

vdemeester approved these changes May 11, 2020

View reviewed changes

tekton-robot assigned vdemeester May 11, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 11, 2020

tekton-robot merged commit 8bac7f3 into tektoncd:master May 11, 2020

bobcatfish mentioned this pull request May 21, 2020

adding finally type (without implementation) at the pipeline level #2650

Merged

3 tasks

Replace all the various (-want, +got) styles with a single format #2580

Replace all the various (-want, +got) styles with a single format #2580

Uh oh!

Conversation

ghost commented May 7, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Uh oh!

bobcatfish commented May 7, 2020

Uh oh!

bobcatfish May 7, 2020

Choose a reason for hiding this comment

Uh oh!

bobcatfish May 7, 2020

Choose a reason for hiding this comment

Uh oh!

ghost May 8, 2020

Choose a reason for hiding this comment

Uh oh!

ghost May 8, 2020

Choose a reason for hiding this comment

Uh oh!

ghost May 8, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost May 8, 2020

Choose a reason for hiding this comment

Uh oh!

bobcatfish commented May 7, 2020

Uh oh!

pritidesai commented May 8, 2020

Uh oh!

pritidesai commented May 8, 2020

Uh oh!

tekton-robot commented May 8, 2020

Uh oh!

ghost commented May 8, 2020

Uh oh!

tekton-robot commented May 8, 2020

Uh oh!

ghost commented May 8, 2020

Uh oh!

imjasonh commented May 8, 2020

Uh oh!

vdemeester commented May 11, 2020

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

tekton-robot commented May 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ghost commented May 7, 2020 •

edited by ghost

Loading

ghost May 8, 2020 •

edited by ghost

Loading