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

switch tests to use github.com/google/go-cmp package #1851

Closed
willnorris opened this issue Apr 8, 2021 · 4 comments · Fixed by #1875
Closed

switch tests to use github.com/google/go-cmp package #1851

willnorris opened this issue Apr 8, 2021 · 4 comments · Fixed by #1875
Assignees

Comments

@willnorris
Copy link
Collaborator

#1846 calls out not using reflect.DeepEqual for comparing error values, but in reality we shouldn't be using it all. https://pkg.go.dev/github.com/google/go-cmp/cmp is designed to be a much safer alternative, so we should use that for tests.

@willnorris
Copy link
Collaborator Author

and just to be clear, we should use go-cmp for all of our non-error values (of which there appear to be 600+ comparisons?!). Switching to go1.13's errors.Is is the right change for errors.

@photongupta
Copy link

@willnorris Can I start working on this?

@willnorris
Copy link
Collaborator Author

have at it! (With so many instances, you might want to do a global find and replace, or use gofmt -r)

@sagar23sj
Copy link
Contributor

sagar23sj commented Apr 22, 2021

Hi @photongupta. Are You Still working on this issue.
I would also like to contribute.

sagar23sj added a commit to sagar23sj/go-github that referenced this issue May 26, 2021
Replaced all reflect.DeepEqual comparisons in test files with cmp.Equal.

Fixes: google#1851
sagar23sj added a commit to sagar23sj/go-github that referenced this issue May 26, 2021
Replaced all reflect.DeepEqual comparisons in test files with cmp.Equal.

Fixes: google#1851
sagar23sj added a commit to sagar23sj/go-github that referenced this issue May 26, 2021
Replaced all reflect.DeepEqual comparisons in test files with cmp.Equal.

Fixes: google#1851
willnorris pushed a commit that referenced this issue May 26, 2021
Replaced all reflect.DeepEqual comparisons in test files with cmp.Equal.

Fixes: #1851
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@willnorris @sagar23sj @photongupta and others