Improve test coverage - #105
Conversation
| self.quicHandler.createOutboundConnection( | ||
| serverName: serverName, | ||
| remoteAddress: remoteAddress, | ||
| connectionInitializer: { [connectionInitializer] connectionChannel, streamCreator in |
There was a problem hiding this comment.
Nit: I don't think this capture is needed, it will be implicitly captured anyways
| connectionInitializer: { [connectionInitializer] connectionChannel, streamCreator in | |
| connectionInitializer: { connectionChannel, streamCreator in |
There was a problem hiding this comment.
Unfortunately we can't remove that because it would mean self (which is QUICConnectionCreator) would be captured inside the @Sendable closure. QUICConnectionCreator can't be made Sendable as it contains QUICHandler.
| case .http1(let asyncChannel): | ||
| do { | ||
| try await asyncChannel.channel.close() | ||
| } catch ChannelError.alreadyClosed { |
There was a problem hiding this comment.
Is it okay to swallow this error everywhere? why are we doing this?
There was a problem hiding this comment.
We need to swallow the alreadyClosed error, because in some tests, the connection may have already been closed by the server before we exit from the body closure. For example, in testServerCanContinueDespiteFailedConnection, the server closes the connection upon seeing the first request. Catching this error at the call site would complicate the tests in my opinion.
| #endif | ||
|
|
||
| default: | ||
| throw NIOHTTPServerConfigurationError.incompatibleTransportSecurity |
There was a problem hiding this comment.
I wonder if we should throw a test-specific error here instead of reusing this, since it could be mistaken from tests as an error coming from the actual business code.
| ) async throws -> NIOAsyncChannel<HTTPResponsePart, HTTPRequestPart> { | ||
| switch self.connectionProtocol { | ||
| case .http1(let http1Channel): | ||
| #expect( |
There was a problem hiding this comment.
These expects in this func should probably be requires instead, as anything that we assert after this will fail too anyways.
|
|
||
| for expectedHeadPart in expectedHead { | ||
| let headResponsePart = try await responseIterator.next() | ||
| #expect(headResponsePart == .head(expectedHeadPart), sourceLocation: sourceLocation) |
There was a problem hiding this comment.
Similar to my other comment elsewhere, I think these should be requires instead of expects
2 participants
Motivation:
All tests currently only test the plaintext or secure upgrade transport. It would be useful to also include the HTTP/3 transport in these tests. Due to the differences in setting up a secure upgrade client compared to an HTTP/3 client, this requires some refactoring to the test utilities.
Modifications:
NIOHTTPServerTeststo a newTestHelperstype to make accessing the helpers more convenient.NegotiatedClientConnectiontoTestClientConnectionand extended it to also wrap HTTP/3 connections.HTTPVersionand removed some tests that became redundant as a result.Result:
Better test coverage.