add a timeout to confirmation expectation

[mredig]

Motivation

Here's a situation where running tests with an asynchronous expectation causes confirmation to conclude that the expectation never ran, but prematurely.

	@Test func cancelViaAbandon() async throws {

		try await confirmation { terminatedExpectation in
			_ = Task {
				let input = [0, 1, 2, 3, 4, 5, 6]

				let (stream, continuation) = AsyncThrowingStream<Int, Error>.makeStream()

				continuation.onTermination = { reason in
					terminatedExpectation()
					print("Terminated: \(reason)")
				}

				for num in input {
					try await Task.sleep(for: .milliseconds(20))
					guard num < 4 else { return }
					continuation.yield(num)
					print(num)
				}
			}

			// If we don't wait a bit for the `onTermination` to complete, then `confirmation` will conclude that the
			// expectation is never called.
			// try await Task.sleep(for: .milliseconds(500))
		}
	}

If you run this as is, confirmation concludes that terminatedExpectation will never get called, therefore the test fails. However, if we uncomment the last line and add a short delay to give what I assume is ARC and stack unwinding to cleanup the Task contents, causing onTermination's closure to trigger prior to confirmation finishing, we are all good.

Proposed solution

Add a timeout to Testing.Confirmation. This would replicate my simple fix that was commented out in the sample code above.

Now, I realize this is a bit fuzzy and potentially indeterminate... What if the system is running slow enough that the cleanup doesn't happen in time for the timeout? However, I don't see any other way to refactor this test that wouldn't be susceptible to this. And the default could be set to zero so it's identical to what it does now unless deliberately overridden.

Alternatives considered

Refactor the test in some way that would alleviate this issue, but I'm not seeing a way to do so.

Additional information

No response

[mredig]

nit - I don't know how triage-needed got added, I just wanted to see if this would be a welcome addition/would like this feature.

[grynspan]

You can resolve your issue by running your Task's body directly in the confirmation. It takes an async closure.

confirmation() uses Swift concurrency and simply calls the closure you pass to it. Hence, it does not typically need a timeout because the code you are testing can be assumed to have completed before it returns—if it hasn't completed, then it should either still be running or should be suspended waiting for something else.

It is not the same mechanism as XCTestExpectation which doesn't take Swift concurrency and the suspension of tasks into account.

[grynspan]

Duplicate of #788

[grynspan]

I should clarify: we are tracking whether or not we should add a timeout, however most needs for one in Swift Testing tend to be solvable using Swift concurrency.

An experimental implementation is available at #789.

[mredig]

You can resolve your issue by running your Task's body directly in the confirmation. It takes an async closure.

confirmation() uses Swift concurrency and simply calls the closure you pass to it. Hence, it does not typically need a timeout because the code you are testing can be assumed to have completed before it returns—if it hasn't completed, then it should either still be running or should be suspended waiting for something else.

It is not the same mechanism as XCTestExpectation which doesn't take Swift concurrency and the suspension of tasks into account.

I am aware that I COULD, but I have it in its own task to make sure the stream gets cleaned up with ARC after completion. I don't recall if I tested both ways, tbh, but I don't think the stream gets cleaned up until the end of scope, which wouldn't trigger the onTermination, which would mean that the 0 confirmations would be correct... But now that I say that, I suspect it MIGHT still replicate the issue... I suppose the stream would be deallocated at the end of the confirmation scope, but I don't think the fix I put in would actually work.

Maybe I didn't run it, I don't remember anymore. I do a lot of iterations. :D

Anyways, good to know it's on the team's radar.

[grynspan]

AsyncStream is a value type, not a reference type, so it does not participate in ARC and is destroyed when its scope terminates (if not earlier.) It does contain reference-counted values, but the stream itself is guaranteed to be cleaned up unless the process terminates early.

[mredig]

Yes, but AsyncStream has a couple subtypes for storage and context that are (and have deinits that trigger cleanup, including the onTermination). :)

Honestly, I can't remember what my original thinking was anymore. Perhaps the intent was that the Task was simulating an isolated concurrency context?

I don't know. I was writing tests for my custom AsyncSequence implementation and happened upon a weird situation resembling the other issue where the expectation would fire, but the test was failing with a claim that there were no expectations completing. Once I realized it was a race condition, I simply tweaked things until it would be something that would consistently demonstrate the issue to post here.

Thank you for your explanations though!

[grynspan]

Happy to help—sorry if this wasn't a productive discussion! Please feel free to reach out in the forums if you run into further problems.

[mredig]

sorry if this wasn't a productive discussion!

Oh it was though! I learned and introspected several times :)