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

Provide NIOAsyncTestingChannel #2238

Merged
merged 14 commits into from
Aug 10, 2022

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Aug 8, 2022

Motivation

Testing versions of NIO code that involve interfacing with Swift
Concurrency is currently a difficult business. In particular,
EmbeddedChannel is not available in Swift concurrency, making it
difficult to write tests where you fully control the I/O.

To that end, we should provide a variation of EmbeddedChannel that makes
testing these things possible.

Modifications

Provide an implementation of NIOAsyncTestingChannel.

Results

Users can write tests confidently with async/await.

@Lukasa Lukasa added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Aug 8, 2022
@Lukasa
Copy link
Contributor Author

Lukasa commented Aug 8, 2022

cc @0xTim, this should help with the WebSocket concerns you had.

Motivation

Testing versions of NIO code that involve interfacing with Swift
Concurrency is currently a difficult business. In particular,
EmbeddedChannel is not available in Swift concurrency, making it
difficult to write tests where you fully control the I/O.

To that end, we should provide a variation of EmbeddedChannel that makes
testing these things possible.

Modifications

Provide an implementation of NIOAsyncTestingChannel.

Results

Users can write tests confidently with async/await.
@Lukasa Lukasa force-pushed the cb-time-for-async-embedded-channel branch from f0a457d to fb84a97 Compare August 8, 2022 13:21
@0xTim
Copy link
Contributor

0xTim commented Aug 8, 2022

Great once this is merged I'll try and get my Websocket PR working

Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Looks good modulo a couple of small things.

}
}

/// `WrongTypeError` is throws if you use `readInbound` or `readOutbound` and request a certain type but the first
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// `WrongTypeError` is throws if you use `readInbound` or `readOutbound` and request a certain type but the first
/// `WrongTypeError` is thrown if you use `readInbound` or `readOutbound` and request a certain type but the first


/// Returns the `NIOAsyncTestingEventLoop` that this `NIOAsyncTestingChannel` uses. This will return the same instance as
/// `NIOAsyncTestingChannel.eventLoop` but as the concrete `NIOAsyncTestingEventLoop` rather than as `EventLoop` existential.
public let embeddedEventLoop: NIOAsyncTestingEventLoop
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be testingEventLoop?

case clean

/// The `NIOAsyncTestingChannel` has inbound, outbound, or pending outbound data left on `finish`.
case leftOvers(inbound: CircularBuffer<NIOAny>, outbound: CircularBuffer<NIOAny>, pendingOutbound: [NIOAny])
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to continue using CircularBuffer in new public API or should we start switching to Deque where possible?

(I know these come from the channel core and we probably don't want to incur the additional allocation cost from copying them over but I'm still interested in the answer to the general case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that's a good question. In this instance we should leave it due to the existing use in the embedded channel, but I think that we should be using Deque in new API if we don't have a good reason not to.

Sources/NIOEmbedded/AsyncTestingChannel.swift Show resolved Hide resolved
@Lukasa
Copy link
Contributor Author

Lukasa commented Aug 9, 2022

Hmm, we've regressed allocations in EmbeddedChannel due to the extra atomics. Let me see if I can tweak the numbers a bit.

Copy link
Member

@FranzBusch FranzBusch left a comment

Choose a reason for hiding this comment

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

Great stuff! I think this will make async testing way better. Just left a couple of smaller comments inline.

Sources/NIOEmbedded/AsyncTestingChannel.swift Show resolved Hide resolved
import NIOConcurrencyHelpers
import NIOCore

/// `NIOAsyncTestingChannel` is a `Channel` implementation that does no
Copy link
Member

Choose a reason for hiding this comment

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

Should we use DocC syntax in all the comments here, i.e. double backticks `` and reference all the types fully?

/// are no elements in the outbound buffer, `nil` will be returned.
///
/// Data hits the `NIOAsyncTestingChannel`'s outbound buffer when data was written using `write`, then `flush`ed, and
/// then travelled the `ChannelPipeline` all the way too the front. For data to hit the outbound buffer, the very
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// then travelled the `ChannelPipeline` all the way too the front. For data to hit the outbound buffer, the very
/// then travelled the `ChannelPipeline` all the way to the front. For data to hit the outbound buffer, the very

Sources/NIOEmbedded/AsyncTestingChannel.swift Show resolved Hide resolved

/// This method will throw the error that is stored in the `NIOAsyncTestingChannel` if any.
///
/// The `NIOAsyncTestingChannel` will store an error if some error travels the `ChannelPipeline` all the way past its end.
Copy link
Member

Choose a reason for hiding this comment

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

Can we specify what end of the pipeline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Errors can only go to one end of the pipeline.

///
/// - parameters:
/// - loop: The `NIOAsyncTestingEventLoop` to use.
public init(loop: NIOAsyncTestingEventLoop = NIOAsyncTestingEventLoop()) {
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Could we move all the inits above the methods?

@Lukasa Lukasa merged commit f5448fb into apple:main Aug 10, 2022
@Lukasa Lukasa deleted the cb-time-for-async-embedded-channel branch August 10, 2022 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants