Skip to content

[Tests] Add some basic IPv6 tests #483

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

Merged
merged 2 commits into from
Nov 23, 2021
Merged

Conversation

karwa
Copy link
Contributor

@karwa karwa commented Nov 19, 2021

While working on the WebURL port, I noticed that there seemed to be no tests at all covering IPv6.

These are just 2 very simple ones, copies of testGetHttpsWithIP and testGetHTTPSWorksOnMTELGWithIP which use IPv6 rather than IPv4. It would be good to add more (in particular, I think it would be worth testing that the proxy's CONNECT request-target is properly formatted, but I'm not that familiar with the project so I'm unsure how to write proxy tests).

@swift-server-bot
Copy link

Can one of the admins verify this patch?

6 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

Copy link
Member

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Thanks @karwa! I've added two small code suggestions to the tests, that make the tests non throwing (which is how we prefer them).

@karwa
Copy link
Contributor Author

karwa commented Nov 20, 2021

@fabianfett These tests are just copy-pasted from the 2 tests immediately above them (testGetHttpsWithIP and testGetHTTPSWorksOnMTELGWithIP). Perhaps it's worth updating them all (and similar tests in that file) in a follow-up?

@fabianfett
Copy link
Member

@karwa Yes, we are aware that not all current tests are in the form we would like them to be in. For this reason, a follow up pr would be very welcome.

@fabianfett fabianfett added the semver/none No version bump required. label Nov 22, 2021
@karwa karwa force-pushed the ipv6-tests branch 2 times, most recently from 9c55cf9 to 1c05e2a Compare November 23, 2021 09:52
@karwa
Copy link
Contributor Author

karwa commented Nov 23, 2021

I reworked the tests not to throw as @fabianfett suggested, but the preflight check for IPv6 is best modelled in XCTest by XCSkip, which marks a test "skipped" instead of passed/failed. That feature works by throwing, which means the test once again needs to be throwing.

Otherwise I could make the tests silently pass if we can' bind the IPv6 loopback, but I figure showing skipped adds significant value, so it's worth just accepting the throws here.

@karwa karwa force-pushed the ipv6-tests branch 2 times, most recently from c36644d to d2b40d3 Compare November 23, 2021 10:27
@Lukasa
Copy link
Collaborator

Lukasa commented Nov 23, 2021

A few minor formatting issues to clean up and then this is good to go:

diff --git a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
index 0a61b10..b18c078 100644
--- a/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
+++ b/Tests/AsyncHTTPClientTests/HTTPClientTestUtils.swift
@@ -51,9 +51,9 @@ let canBindIPv6Loopback: Bool = {
     let elg = MultiThreadedEventLoopGroup(numberOfThreads: 1)
     defer { try! elg.syncShutdownGracefully() }
     let serverChannel = try? ServerBootstrap(group: elg)
-          .serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1)
-          .bind(host: "::1", port: 0)
-          .wait()
+        .serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1)
+        .bind(host: "::1", port: 0)
+        .wait()
     let didBind = (serverChannel != nil)
     try! serverChannel?.close().wait()
     return didBind

@karwa
Copy link
Contributor Author

karwa commented Nov 23, 2021

@Lukasa I've addressed the nit. For some reason, these are showing up as skipped in the bots - are they perhaps not configured with IPv6 support? The tests work (are not skipped) on my Mac.

@Lukasa
Copy link
Collaborator

Lukasa commented Nov 23, 2021

Yes, I suspect our CI is one of those environments where IPv6 doesn't work, which is probably what triggered that memory of this need.

@Lukasa Lukasa merged commit 6426c00 into swift-server:main Nov 23, 2021
@karwa karwa deleted the ipv6-tests branch November 23, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants