Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review usage of cmp.Diff to follow the (want, got) pattern #7027

Closed
vdemeester opened this issue Aug 7, 2023 · 4 comments · Fixed by #7209
Closed

Review usage of cmp.Diff to follow the (want, got) pattern #7027

vdemeester opened this issue Aug 7, 2023 · 4 comments · Fixed by #7209
Assignees
Labels
area/testing Issues or PRs related to testing good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. Hacktoberfest Categorizes issue as one for Hacktoberfest 2021 help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@vdemeester
Copy link
Member

vdemeester commented Aug 7, 2023

We heavily use cmp.Diff in our tests. Those should follow the (want, got) pattern (or (expected, actual)) when used.
However, if we go through the tests, sometimes it is used in the opposite way (aka (got, want)).

This can become a problem when reviewing the failures of tests as, we are assuming the correct usage when printing out the error (diff.PrintWantGot(d)). This means, in some tests, the error is inverted, it displays "got" as wanted, and "wanted" as got… Which tends to be relatively painful when figuring things out.

// PrintWantGot takes a diff string generated by cmp.Diff and returns it
// in a consistent format for reuse across all of our tests. This
// func assumes that the order of arguments passed to cmp.Diff was
// (want, got) or, in other words, the expectedResult then the actualResult.

We would need to go from the former to the latter.

        // Invalid usage
	if d := cmp.Diff(updatedTR.Status.TaskSpec, expectedStatusSpec); d != "" {
		t.Errorf("expected Status.TaskSpec to match, but differed: %s", diff.PrintWantGot(d))
	}
        // Valid usage
	if d := cmp.Diff(expectedStatusSpec, updatedTR.Status.TaskSpec); d != "" {
		t.Errorf("expected Status.TaskSpec to match, but differed: %s", diff.PrintWantGot(d))
	}

/cc @tektoncd/core-collaborators @tektoncd/core-maintainers

@vdemeester vdemeester added help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. area/testing Issues or PRs related to testing kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 7, 2023
@xiangpingjiang xiangpingjiang removed their assignment Sep 13, 2023
@T3-git
Copy link

T3-git commented Oct 2, 2023

/assign T3-git

@afrittoli afrittoli added the Hacktoberfest Categorizes issue as one for Hacktoberfest 2021 label Oct 4, 2023
@minhoryang
Copy link
Contributor

Is there any progress on this?

@T3-git
Copy link

T3-git commented Oct 15, 2023

@minhoryang, I just returned from my vacation and was about to address the issue/task. It seems you have handled this so let me know how to procced.

@T3-git T3-git removed their assignment Oct 15, 2023
JeromeJu pushed a commit to JeromeJu/pipeline that referenced this issue Oct 17, 2023
@minhoryang
Copy link
Contributor

/assign @minhoryang

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Issues or PRs related to testing good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. Hacktoberfest Categorizes issue as one for Hacktoberfest 2021 help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants