-
Notifications
You must be signed in to change notification settings - Fork 49
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
Test: remove part of the hardcoded network ports #206
base: master
Are you sure you want to change the base?
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -240,14 +240,16 @@ func TestRedisRedundancy(t *testing.T) { | |
mr, cert := startRedis(t, true) | ||
defer mr.Close() | ||
|
||
ports := []int{48690, 48691, 48692} | ||
ports := make([]int, 3) | ||
servers := make([]*requestorserver.Server, len(ports)) | ||
|
||
for i, port := range ports { | ||
for i := range ports { | ||
port := findFreePort() | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
c.Port = port | ||
rs := StartRequestorServer(t, c) | ||
ports[i] = port | ||
servers[i] = rs | ||
} | ||
lb := startLoadBalancer(t, ports) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 useurl.Parse
to actually replace the host part withlocalhost
. Then it should work in all cases.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:With portFromTestServer being:
This function can also be used in the function
findFreePort
to prevent code duplication. ThefindFreePort
function is a bit of a hack for theRequestorServer
now. Ideally, that function shouldn't be needed.