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.

Sign up for GitHub

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

keyring_test: support ipv6 #188

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from
Open

keyring_test: support ipv6 #188

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion httpbakery/keyring_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func (s *KeyringSuite) TestThirdPartyInfoForLocationWrongURL(c *gc.C) {
_, err := httpbakery.ThirdPartyInfoForLocation(testContext, client, "http://localhost:0")
c.Logf("%v", errgo.Details(err))
c.Assert(err, gc.ErrorMatches,
`(Get|GET) http://localhost:0/discharge/info: dial tcp 127.0.0.1:0: .*connection refused`)
`(Get|GET) http://localhost:0/discharge/info: dial tcp (127.0.0.1:0:|\[::1\]:0:) .*connection refused`)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that rather than fixing the message, I'd prefer to change the address, so that it's consistent (and also undialable). How about using 0.1.2.3 (and then changing the error message accordingly)?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend not comparing error messages at all. See also:
https://dave.cheney.net/2014/12/24/inspecting-errors
https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully

In fact, with Go 1.10, we observe other test failures in bakery.v2 because error messages have changed in subtle ways.

Switching away from verifying specific error messages would make our life much easier as a maintainer.

Thanks for considering!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that error messages changing is a pain. However, if you don't compare error messages at all, you end up with another problem, which is that the error might not be what you think it is. I've seen that multiple times - the test continues to pass, but actually the error is something completely different and actually the code is broken.

Also, users see error messages, and it's not uncommon for error messages to become substantially unreadable due to redundant and inappropriate information. Often the only place you see those error messages is in the tests - it's a good place to review them and check that they look OK.

I think the tradeoff is worth it, in general.

FWIW Dave Cheney's posts that you reference are about production code, not tests. He even says "this advice doesn’t apply to writing tests" (although he qualifies that a moment later).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the test continues to pass, but actually the error is something completely different and actually the code is broken.

I’d argue that your test is too far removed from the code in question in that case :)

Often the only place you see those error messages is in the tests - it's a good place to review them and check that they look OK.

Agreed, but t.Logf should be used for that, IMO :)

In the end it’s up to you, but I would at least suggest relaxing the regexen significantly.

}

func (s *KeyringSuite) TestThirdPartyInfoForLocationReturnsInvalidJSON(c *gc.C) {
Expand Down