Test: remove part of the hardcoded network ports #206
Conversation
| httpServer := httptest.NewServer(mux) | ||
|
|
||
| // Make sure domain is used instead of IP address. | ||
| conf.URL = strings.Replace(httpServer.URL, "127.0.0.1", "localhost", 1) |
There was a problem hiding this comment.
Why is this necessary? If it is because IPv6 might be used, it would be helpful to add that to the comment above.
There was a problem hiding this comment.
Hmm, I didn't think of IPv6. The httptest library gives us an IP address instead of a domain. This causes the requestor name test to fail, because this one relies on the domain being localhost. I'm not sure whether we can rewrite that test in such a way that it expects an IP address. If that's not possible, we can use url.Parse to actually replace the host part with localhost. Then it should work in all cases.
There was a problem hiding this comment.
Switching the domain in the test-requestors scheme to an IP address does not seem to be a very smart solution, because then we hardcode the use of IPv4 even more. I'd say let's do it this way then:
conf.URL = fmt.Sprintf("http://localhost:%d", portFromTestServer(httpServer))
With portFromTestServer being:
func portFromTestServer(s *httptest.Server) int {
return s.Listener.Addr().(*net.TCPAddr).Port
}
This function can also be used in the function findFreePort to prevent code duplication. The findFreePort function is a bit of a hack for the RequestorServer now. Ideally, that function shouldn't be needed.
| c := redisRequestorConfigDecorator(mr, cert, "", RequestorServerAuthConfiguration)() | ||
| c.Configuration.URL = fmt.Sprintf("http://localhost:%d/irma", port) | ||
| // Make sure URL of load balancer is being used in QRs. | ||
| c.URL = requestorServerURL |
There was a problem hiding this comment.
This changes the test behaviour itself, instead of just some ports. Strange that it wasn't already like this.
There was a problem hiding this comment.
True, it was a mistake in the test I think. We overlooked that one. Luckily, it works like a charm, so it only was a testing mistake and not a functional mistake.
The test sometimes fail because one of the hardcoded ports is not available. This PR removes most of these hardcoded ports.
The ones I did not remove where reliant on hardcoded URLs in the test configuration:
I did also not remove the global variables for the URL of the requestor server, because the
optionReuseServerof thedoSessionhelper relies on a globally specified URL. I managed to make the URL being generated dynamically, but an improvement would still be to refactordoSessionsuch that also the hardcodedrequestorServerPortandrequestorServerURLcan be removed.