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

x/build/maintner: TestForeachRepo is failing on Go tip due to error change #30442

Closed
dmitshur opened this issue Feb 27, 2019 · 4 comments
Closed
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge
Milestone

Comments

@dmitshur
Copy link
Contributor

With Go 1.12:

$ go test -v -run=TestForeachRepo -count=1 golang.org/x/build/maintner
=== RUN   TestForeachRepo
--- PASS: TestForeachRepo (0.00s)
    github_test.go:751: Tested Reviews
PASS
ok  	golang.org/x/build/maintner	0.146s

With Go tip (at commit df557fe):

$ gotip test -v -run=TestForeachRepo -count=1 golang.org/x/build/maintner
=== RUN   TestForeachRepo
--- FAIL: TestForeachRepo (0.00s)
    github_test.go:743: Will Error: ForeachReview errs differ. got: Planned Failure, want: Planned Failure
    github_test.go:743: Will Error Late: ForeachReview errs differ. got: Planned Failure, want: Planned Failure
    github_test.go:751: Tested Reviews
FAIL
FAIL	golang.org/x/build/maintner	0.148s

I suspect it's because of 6be6f11. It changed the internal representation of the result of fmt.Errorf, making it so that !deep.ReflectEqual no longer compares favorably in github_test.go#L742-L744:

if !reflect.DeepEqual(tt.wantErr, err) {
	t.Errorf("%s: ForeachReview errs differ. got: %s, want: %s", tt.name, err, tt.wantErr)
}

This makes trybots in x/build repo fail, e.g., https://go-review.googlesource.com/c/build/+/163205/2#message-8c683121b60291567babee1aed8cd19a0f83bfc6.

/cc @bradfitz

@dmitshur dmitshur added the Builders x/build issues (builders, bots, dashboards) label Feb 27, 2019
@gopherbot gopherbot added this to the Unreleased milestone Feb 27, 2019
@bradfitz
Copy link
Contributor

We could switch to using https://github.com/google/go-cmp instead of reflect.

@dmitshur
Copy link
Contributor Author

That sounds like a good fix for the issue in x/build/maintner.

@dmitshur dmitshur self-assigned this Feb 27, 2019
@dmitshur
Copy link
Contributor Author

dmitshur commented Feb 27, 2019

Since it was just two error values being compared, and not a more deeply nested struct, I went with a simpler solution in CL 164297 for now.

@gopherbot
Copy link

Change https://golang.org/cl/164297 mentions this issue: maintner: compare errors via their public API

@golang golang locked and limited conversation to collaborators Feb 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Builders x/build issues (builders, bots, dashboards) FrozenDueToAge
Projects
None yet
Development

No branches or pull requests

3 participants