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

Add NIOAsyncWriterSinkDelegate._didSuspend hook for testing #2597

Merged

Conversation

simonjbeaumont
Copy link
Contributor

@simonjbeaumont simonjbeaumont commented Nov 24, 2023

Motivation:

The tests for the NIOAsyncWriter currently rely on a number of Task.sleep calls to wait for tasks to be suspended or cancelled before performing some other operation. These have been shown to be flakey in CI. We could just increase all the sleeps, but instead we can make use of XCTestExpectation to wait for things to happen. The events that we are waiting on are these:

  1. Waiting within a task for the task to be cancelled before we yield.
  2. Waiting outside a task that is performing a yield for that task to be suspended.

This didn't require any additional changes to the writer for (1) and can just wait for an expectation inside the task that we fulfil after the cancelling the task from outside.

For (2) we need to have a hook that we can use when the task has performed the state transition and is parked that we can use to fulfil the expectation.

Modifications:

  • Add an internal _didSuspend optional closure to NIOAsyncWriterSinkDelegate.
  • Replace all calls to Task.sleep(nanoseconds:) in tests with await fulfillment(of:[timeout:]).

Result:

Tests should be more deterministic.

@simonjbeaumont simonjbeaumont force-pushed the sb/remove-sleeps-from-async-writer-tests branch 3 times, most recently from 0208d98 to cc38e09 Compare November 25, 2023 21:08
@simonjbeaumont simonjbeaumont changed the title RFC/DNM: Add NIOAsyncWriterSinkDelegate._didSuspend hook for testing Add NIOAsyncWriterSinkDelegate._didSuspend hook for testing Nov 25, 2023
@simonjbeaumont simonjbeaumont marked this pull request as ready for review November 25, 2023 22:17
@simonjbeaumont simonjbeaumont force-pushed the sb/remove-sleeps-from-async-writer-tests branch from cc38e09 to 31468a7 Compare November 27, 2023 09:37
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.

I hate it but it solves the flakiness, so let's do it! We should probably do the same for the NIO[Throwing]AsyncSequenceProducer tests. Do you mind doing the same there? Can even put it into this PR if you want!

@@ -13,7 +13,7 @@
//===----------------------------------------------------------------------===//

import DequeModule
import NIOCore
@testable import NIOCore
Copy link
Member

Choose a reason for hiding this comment

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

I prefer if we don't use @testable. It is a horrible compiler hack and I would love to slowly get away from it by either using SPI or the new package modifier. For now let's use @_spi(Testing) and we can migrate that in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, discussed this offline.

I think we agreed that what we really want here is package, but we cannot use that today. Making it SPI might have a performance impact when compiled without -enable-testability because we'd have to make some things public (SPI) that were internal, which could impact the optimisations possible.

I'm pretty flexible at this point. I have a lean toward @testable import because it leaves the release build unaffected, but if you'd like I can do the SPI dance before we merge.

@simonjbeaumont
Copy link
Contributor Author

I hate it but it solves the flakiness, so let's do it! We should probably do the same for the NIO[Throwing]AsyncSequenceProducer tests. Do you mind doing the same there? Can even put it into this PR if you want!

Added commit to give the NIOThrowingAsyncSequenceProducerTests the same treatment.

@FranzBusch FranzBusch enabled auto-merge (squash) November 30, 2023 10:37
@FranzBusch FranzBusch merged commit 4a42bc2 into apple:main Nov 30, 2023
6 of 8 checks passed
@glbrntt glbrntt added the semver/patch No public API change. label Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants