keyring_test: support ipv6 - #188
Conversation
rogpeppe
left a comment
rogpeppe
left a comment
There was a problem hiding this comment.
Thanks very much for this change. LGTM modulo one suggestion.
| 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`) |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
Bumping the PR, as I encountered this issue again while updating the Debian packaging of this library. |
4 participants
the test wouldn't run on a machine with ipv6 interfaces