/juju

Fix unittests for Go 1.8 #7217

Merged
merged 4 commits into from Apr 10, 2017

Conversation

Reviewers

@jameinel jameinel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@axw @jujubot @jameinel
@axw
Member

axw commented Apr 10, 2017

Description of change

Fix various unit tests for Go 1.8.

QA steps

Run the tests with go 1.8.

Documentation changes

None.

Bug reference

None.
network/hostport_test.go
}, {
input: "host:port",
err: `cannot parse "host:port" port: strconv.(ParseInt|Atoi): parsing "port": invalid syntax`,
}, {
input: "::1",
- err: `cannot parse "::1" as address:port: too many colons in address ::1`,
+ err: `cannot parse "::1" as address:port: .*too many colons in address.*`,
@jameinel

jameinel Apr 10, 2017

Owner

do we understand what impact this will have on users? It feels like we might be regressing the kind of errors we would be exposing users to.
This could certainly be fine, I'd just like to understand what the errors changed to.

@axw

axw Apr 10, 2017

Member

The error changed from:
cannot parse "::1" as address:port: too many colons in address ::1
to
cannot parse "::1" as address:port: address ::1: too many colons in address

utils/proxy/proxyconfig.go
@@ -163,6 +163,9 @@ var portMap = map[string]string{
func canonicalAddr(url *url.URL) string {
addr := url.Host
if !hasPort(addr) {
+ if strings.HasPrefix(addr, "[") && strings.HasSuffix(addr, "]") {
@jameinel

jameinel Apr 10, 2017

Owner

this feels really bad.
Why wouldn't JoinHostPort be aware of IPv6 addresses and do the right thing?

@axw

axw Apr 10, 2017

Member

The square brackets thing is for URLs. JoinHostPort does not deal with URLs, it deals with addresses.

The Go 1.8 code for proxies does the right thing and uses URL.Hostname, which strips off the brackets. When we move everything over to Go 1.8, we should look to drop this code. (I forget why we have it in the first place?)

utils/proxy/proxyconfig_test.go
@@ -47,7 +47,7 @@ var (
func (s *Suite) TestGetProxy(c *gc.C) {
checkProxy(c, normal, "https://perfect.crime", "https://https.proxy")
checkProxy(c, normal, "decemberists.com", "http://http.proxy")
- checkProxy(c, normal, "2001:db8:85a3::8a2e:370:7334", "http://http.proxy")
+ checkProxy(c, normal, "http://[2001:db8:85a3::8a2e:370:7334]", "http://http.proxy")
@jameinel

jameinel Apr 10, 2017

Owner

do you know what changed here? This feels like the sort of thing that might actually impact runtime if they changed when proxy values are returned.

@axw

axw Apr 10, 2017

Member

Go 1.8's url.Parse became more strict (see commit message). We were passing something that is not a URL into url.Parse, and expecting it to parse.

@jameinel

jameinel Apr 10, 2017

Owner

decemberists.com isn't a URL either, is it? (it has no scheme)

worker/introspection/socket_test.go
@@ -107,7 +107,7 @@ func (s *introspectionSuite) TestCmdLine(c *gc.C) {
}
func (s *introspectionSuite) TestGoroutineProfile(c *gc.C) {
- buf := s.call(c, "/debug/pprof/goroutine")
+ buf := s.call(c, "/debug/pprof/goroutine?debug=1")
@jameinel

jameinel Apr 10, 2017

Owner

was this for debugging purposes, what happens on a go 1.6 version if we pass these? Given that the URL is 'debug' it feels weird to have to specify debug=1 again.
And are old clients that are poking this URL (the juju shell scripts) going to be passing the right thing?

@axw

axw Apr 10, 2017

Member

was this for debugging purposes, what happens on a go 1.6 version if we pass these? Given that the URL is 'debug' it feels weird to have to specify debug=1 again.

There's two levels of debugging here :)
/debug/... is for debugging your application; ?debug is for debugging that output.

This is just to force the textual representation, for backwards compatibility. Go 1.6 does not care about the additional ?debug=1. When we raise the minimum version of Go for running the tests to Go 1.8, then we can check the new (binary) format, and drop this.

@jameinel

jameinel approved these changes Apr 10, 2017
View changes

we should audit our usage of url.Parse, but otherwise LGTM

utils/proxy/proxyconfig_test.go
@@ -47,7 +47,7 @@ var (
func (s *Suite) TestGetProxy(c *gc.C) {
checkProxy(c, normal, "https://perfect.crime", "https://https.proxy")
checkProxy(c, normal, "decemberists.com", "http://http.proxy")
- checkProxy(c, normal, "2001:db8:85a3::8a2e:370:7334", "http://http.proxy")
+ checkProxy(c, normal, "http://[2001:db8:85a3::8a2e:370:7334]", "http://http.proxy")
@jameinel

jameinel Apr 10, 2017

Owner

do you know what changed here? This feels like the sort of thing that might actually impact runtime if they changed when proxy values are returned.

@axw

axw Apr 10, 2017

Member

Go 1.8's url.Parse became more strict (see commit message). We were passing something that is not a URL into url.Parse, and expecting it to parse.

@jameinel

jameinel Apr 10, 2017

Owner

decemberists.com isn't a URL either, is it? (it has no scheme)

@axw
Member

axw commented Apr 10, 2017

we should audit our usage of url.Parse, but otherwise LGTM

The only other places where it would matter, we react to the error by interpreting the input as a host-optionally-with-port

axw added some commits Apr 10, 2017

@axw
environs/tools: fix archive tests for go 1.8 
The result of tar & gzip with Go 1.8 is slightly
different to Go 1.7. We don't need an exact
result, so change the test to test the output
functionally.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
ee7ba42
@axw
network: fix unit tests for go 1.8 
Go 1.8's net.SplitHostPort moves the
location of the address in the error
message. Change the assertion to be
less strict on the portion of the
error message that comes from the
standard library.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
472c0f2
@axw
utils/proxy: fix unit tests for go 1.8 
We were passing IPv6 addresses into the
url.Parse function, which is not valid.
We pass in a valid IPv6 URL, including
brackets to identify the address as
IPv6.

This identified an issue with the internal
"canonicalAddr" function, which was
tacking the port onto the end of the
address. That was making use of the Host
part of the URL, which in the case of
IPv6 includes the brackets. JoinHostPort
adds on brackets unconditionally if the
address contains colons; so the input must
not already have brackets. Strip them off
first.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
704d1d9
@axw
worker/introspection: fix unit tests for go 1.8 
Go 1.8 has changed the default format of pprof
profiles. Passing in a "?debug=1" parameter in
the HTTP request forces the old format.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
2f75cb2
@axw
Member

axw commented Apr 10, 2017

decemberists.com isn't a URL either, is it? (it has no scheme)

I've added the "http://" prefix here. I've also added a test that shows that we will happily parse an IPv6 address with or without [ ] or port, in http_proxy/https_proxy.
@axw
Member

axw commented Apr 10, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Apr 10, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit aacafa9 into juju:develop Apr 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment