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.
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.