Skip to content
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

NIOHTTP1TestServer #1152

Merged
merged 6 commits into from Oct 2, 2019

Conversation

@mariosangiorgio
Copy link
Contributor

commented Oct 1, 2019

Added NIOHTTP1TestServer, which serializes requests making it easier to write unit tests where the clients connect to a "real" server.

Motivation:

I am working on a client where a logical request consists of multiple HTTP requests, some of which might happen in parallel. The client uses AsyncHTTPClient, which doesn't expose the NIO Channel it uses. This makes it hard to write unit tests as I normally would.

Modifications:

I added NIOHTTP1TestServer and some tests that cover its behaviour. The test server exposes the same API we would use while interacting with a Channel, making the same testing approach viable also for more complex clients.

Result:

After my change developers writing client based on complex HTTP1 requests will have the facility they need to write unit tests following the same patters they would use writing unit tests for a NIO Channel.

@swift-nio-bot

This comment has been minimized.

Copy link

commented Oct 1, 2019

Can one of the admins verify this patch?

8 similar comments
@swift-nio-bot

This comment has been minimized.

Copy link

commented Oct 1, 2019

Can one of the admins verify this patch?

@swift-nio-bot

This comment has been minimized.

Copy link

commented Oct 1, 2019

Can one of the admins verify this patch?

@swift-nio-bot

This comment has been minimized.

Copy link

commented Oct 1, 2019

Can one of the admins verify this patch?

@swift-nio-bot

This comment has been minimized.

Copy link

commented Oct 1, 2019

Can one of the admins verify this patch?

@swift-nio-bot

This comment has been minimized.

Copy link

commented Oct 1, 2019

Can one of the admins verify this patch?

@swift-nio-bot

This comment has been minimized.

Copy link

commented Oct 1, 2019

Can one of the admins verify this patch?

@swift-nio-bot

This comment has been minimized.

Copy link

commented Oct 1, 2019

Can one of the admins verify this patch?

@swift-nio-bot

This comment has been minimized.

Copy link

commented Oct 1, 2019

Can one of the admins verify this patch?

@Lukasa

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2019

@swift-nio-bot add to whitelist

Copy link
Contributor

left a comment

Generally looks really good! Just a few small notes.

func handleChannels() {
self.eventLoop.assertInEventLoop()

let channel: Channel!

This comment has been minimized.

Copy link
@Lukasa

Lukasa Oct 1, 2019

Contributor

This doesn’t need to be an IUO, there must be a Channel if we get past this section.


assert(self.currentClientChannel == nil)
self.currentClientChannel = channel
channel.closeFuture.whenSuccess {

This comment has been minimized.

Copy link
@Lukasa

Lukasa Oct 1, 2019

Contributor

Do we need to consider what happens here if the connection is closed by the remote peer while we’re waiting to handle a previous connection? In this case I think we’d just lose the connection as this promise will fire immediately.

struct NonEmptyInboundBufferOnStop: Error {}

public func stop() throws {
assert(!self.eventLoop.inEventLoop)

This comment has been minimized.

Copy link
@Lukasa

Lukasa Oct 1, 2019

Contributor

Why does this have to be called off the loop?

case .idle:
self.state = .stopped
case .stopped:
preconditionFailure("double stopped NIOHTTP1TestServer")

This comment has been minimized.

Copy link
@Lukasa

Lukasa Oct 1, 2019

Contributor

Should this crash? It seems unnecessarily forceful.

XCTAssertNoThrow(try group.syncShutdownGracefully())
}

// -- SNIP --

This comment has been minimized.

Copy link
@Lukasa

Lukasa Oct 1, 2019

Contributor

What is this comment for?

This comment has been minimized.

Copy link
@weissi

weissi Oct 1, 2019

Member

@Lukasa everything below the line serves as the example that's copied into the docs :). Just to have it exeutable.

This comment has been minimized.

Copy link
@mariosangiorgio

mariosangiorgio Oct 1, 2019

Author Contributor

That was the intent, but on a second thought if we move sendRequestTo definition outside this function we won't need to snip anything.

@Lukasa Lukasa added this to the 2.9.0 milestone Oct 2, 2019
@Lukasa
Lukasa approved these changes Oct 2, 2019
Copy link
Contributor

left a comment

LGTM, I'm happy!

@Lukasa Lukasa merged commit 23303e7 into apple:master Oct 2, 2019
4 checks passed
4 checks passed
pull request validation (5.0) Build finished.
Details
pull request validation (5.1) Build finished.
Details
pull request validation (api breakage) Build finished.
Details
pull request validation (sanity) Build finished.
Details
@mariosangiorgio mariosangiorgio deleted the mariosangiorgio:nio-http1-test-server branch Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.