Add unit tests for NWWaitingHandler, closes #589

[natikgadzhi]

Motivation:

Closes #589. Since we already have a public initializer for NIOTSNetworkEvents.WaitingForConnectivity, we should add unit tests for the handler now that it's straightforward.

I'm here to learn — first patch in async-http-client.

Modifications:

The tests are in their own file Tests/NWWaitingHandlerTests.swift.

[natikgadzhi]

@dnadoba, hello there! I'm just learning, and that felt like a good issue to start with. How does that look?

[natikgadzhi]

@weissi, perhaps you would like to +1 a few unit tests? Am I doing this right?

[Lukasa]

This requires lifting the minimum version of swift-nio-transport-services to 1.13.0, can you please add that change?

[natikgadzhi]

@Lukasa, thanks for reviewing this, and for calling out the dependency mismatch.

swift build && swift test works locally now. scripts/soundness.sh has a formatting mismatch in a bunch of files with the default swiftformat from brew, but that's unrelated to this PR.

[dnadoba]

Looks really good! Two nits and this should be ready to be merged.
Sorry for the delayed review and thanks for tackling this!

[Lukasa]

@swift-server-bot test this please

[Lukasa]

A few formatting issues in the test:

diff --git a/Tests/AsyncHTTPClientTests/NWWaitingHandlerTests.swift b/Tests/AsyncHTTPClientTests/NWWaitingHandlerTests.swift
index ebbb851..9a1fa72 100644
--- a/Tests/AsyncHTTPClientTests/NWWaitingHandlerTests.swift
+++ b/Tests/AsyncHTTPClientTests/NWWaitingHandlerTests.swift
@@ -16,28 +16,24 @@
 @testable import AsyncHTTPClient
 import Network
 import NIOCore
-import NIOTransportServices
 import NIOEmbedded
 import NIOSSL
+import NIOTransportServices
 import XCTest
 
 @available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *)
 class NWWaitingHandlerTests: XCTestCase {
-
     class MockRequester: HTTPConnectionRequester {
         var waitingForConnectivityCalled = false
-        var connectionID: AsyncHTTPClient.HTTPConnectionPool.Connection.ID? = nil
-        var transientError: NWError? = nil
+        var connectionID: AsyncHTTPClient.HTTPConnectionPool.Connection.ID?
+        var transientError: NWError?
+
+        func http1ConnectionCreated(_: AsyncHTTPClient.HTTP1Connection) {}
+
+        func http2ConnectionCreated(_: AsyncHTTPClient.HTTP2Connection, maximumStreams: Int) {}
+
+        func failedToCreateHTTPConnection(_: AsyncHTTPClient.HTTPConnectionPool.Connection.ID, error: Error) {}
 
-        func http1ConnectionCreated(_: AsyncHTTPClient.HTTP1Connection) {
-        }
-        
-        func http2ConnectionCreated(_: AsyncHTTPClient.HTTP2Connection, maximumStreams: Int) {
-        }
-        
-        func failedToCreateHTTPConnection(_: AsyncHTTPClient.HTTPConnectionPool.Connection.ID, error: Error) {
-        }
-        
         func waitingForConnectivity(_ connectionID: AsyncHTTPClient.HTTPConnectionPool.Connection.ID, error: Error) {
             self.waitingForConnectivityCalled = true
             self.connectionID = connectionID
@@ -50,9 +46,9 @@ class NWWaitingHandlerTests: XCTestCase {
         let connectionID: AsyncHTTPClient.HTTPConnectionPool.Connection.ID = 1
         let waitingEventHandler = NWWaitingHandler(requester: requester, connectionID: connectionID)
         let embedded = EmbeddedChannel(handlers: [waitingEventHandler])
-        
+
         embedded.pipeline.fireUserInboundEventTriggered(NIOTSNetworkEvents.WaitingForConnectivity(transientError: .dns(1)))
-        
+
         XCTAssertTrue(requester.waitingForConnectivityCalled, "Expected the handler to invoke .waitingForConnectivity on the requester")
         XCTAssertEqual(requester.connectionID, connectionID, "Expected the handler to pass connectionID to requester")
         XCTAssertEqual(requester.transientError, NWError.dns(1))
@@ -81,4 +77,3 @@ class NWWaitingHandlerTests: XCTestCase {
 }
 
 #endif
-

[dnadoba]

FYI: we use SwiftFormat in version 0.48.8 which is defined here if you want to run it locally yourself to auto format your code.

[natikgadzhi]

@dnadoba, thank you for the tip on where to lookup the specific swiftFormat version! Soundness checks don't verify that it's installed (and don't fail if it's not), and it looks like the swiftformat version might not be the same across all SSWG repositories.

Is there a community best practice on how to manage multiple repos and versions of swiftformat? The brew formula does not work with versioned installs, but it's straightforward to checkout a git tag and rebuild — should I just do that for now?

[natikgadzhi]

• Merged main into this, so Transport version got booped to 1.19.
• Cleaned up formatting
• Committed the suggestions from @dnadoba (thank you!)

Local tests are happy.

[dnadoba]

Soundness checks don't verify that it's installed (and don't fail if it's not)

That is unfortunate. We have a set of nicer scripts in OpenAPI that catch this kind of issue though. We eventually want to have the same set of soundness checks across all of our repos.

Is there a community best practice on how to manage multiple repos and versions of swiftformat?

Not to my knowledge. I build them from source and luckily rarely need to switch versions for the projects I work on.

[dnadoba]

@swift-server-bot test this please

[natikgadzhi]

When I pushed up my first PR to swift-metrics, I noticed that, and did this: apple/swift-metrics#134 — but, it's not all that useful in a one-off pull request in one repository, like you mentioned, @dnadoba.

There's also this apple/swift-nio#2242 attempt to implement soundness check as a SwiftPM plugin, and I think I saw another take on that as well — and a discussion that the internal team have not yet aligned on what's the best spot to put the universal soundness checks in.

I'm pretty sure that once we align on how to do it, the rollout to all repositories will be easy, but if I can help out in any way — I'd be happy to. Seems like that might be tricky to do from outside of Apple, at least at the alignment stage. 👀