Polling confirmations (under Experimental spi)

[younata]

This implements polling confirmations, as described in ST-NNNN.

Motivation:

Being able to monitor changes in the background is something of immense value. Swift Testing already provides an API for this in the confirmation api. However, I've found the confirmation to be hard to work with at times - it requires you to set up a callback for when something changes, which is not always possible or even the right thing to do. Polling provides a very general approach to monitoring all kinds of changes.

Modifications:

This adds a new set of macros for handling polling. A new public enum for the 2 separate polling behaviors, and new types to actually implement polling. All under the experimental spi.

Checklist:

• Code and documentation should follow the style of the Style Guide.
• If public symbols are renamed or modified, DocC references should be updated.

[plemarquand]

Overall looking really great. Just one comment and a bunch of documentation nits.

[jerryjrchen]

The link in the description links to the old path for the proposal doc. I think it should be updated to point to https://github.com/younata/swift-evolution/blob/younata/testing-polling-expectations/proposals/testing/NNNN-polling-confirmations.md instead

(I think I have edit permissions to make this description change but wasn't sure if that would be polite to do 😅 )

[jerryjrchen]

Overall this looks great to me, and I mostly had a bunch of nitpicks about the docs. I'll caveat by saying I've only read through the proposal recently and skimmed the pitch forum post, so I hope I didn't rehash a bunch of previous questions :)

[jerryjrchen]

Might also be worth mentioning that duration > interval based on the current implementation.

Although from a user perspective, should duration == interval be permissible? I would interpret that basically a single poll (so perhaps not very useful), but still a possible use case.

[younata]

Thanks for bringing this up!

I'll update the documentation & add some tests mentioning this.

I don't see any real harm in disallowing the case where duration == interval. It is not very useful - at least not at the moment - but to your point, it is a possible use case someone might have.

[jerryjrchen]

As obvious as it might sound, I think it would help to explicitly note that nil maps to the false stop condition. That could either go here or maybe in the docs for PollingStopCondition. Based on my reading of the docs for firstPass, I assumed it would only be used with Bool and not with optional return types as well:

  /// Evaluates the expression until the first time it returns true.
  /// If it does not pass once by the time the timeout is reached, then a
  /// failure will be reported.
  case firstPass

[younata]

That's a good point. I'm going to update the docs for PollingStopCondition to be about pass/fail, and then have the documentation for both forms of confirmation specifically mention what is considered a pass/fail.

I'm also adding a test that explicitly documents the case where you return false when you pass in a closure returning Optional<Bool>.

[jerryjrchen]

Should the isolation parameter for this method also get forwarded to poll?

I'm not too familiar with actor isolated parameters so it's totally possible I'm missing something here.

[jerryjrchen]

Nit: would it make sense to say duration instead of timeout here? I don't see "timeout" appearing elsewhere in the polling confirmation API or documentation.

[jerryjrchen]

I realised too late my note below is overly pedantic and I actually think this function is fairly clear as-is. I kinda already typed it out and figured I might as well share it though :)

I'd find it a bit clearer to check for the "timeout" scenario here compared to the call site in poll:

Suggested change

	return .continuePolling
	return wasLastPollingAttempt ? .failed : .continuePolling

Or alternatively an early check on the last polling attempt which would simplify the switch:

    guard !wasLastPollingAttempt else {
      return result.map { .succeeded($0) } ?? .failed
    }

    switch self {
    // ...

[younata]

I appreciate the feedback, and while pedantic, I do agree that makes it easier to read.

I think this is a good suggestion for clarity purposes. It does make the last line in poll redundant (return .failed after the for loop exits), but necessary to satisfy the compiler. I'll add a comment to the end of that method explaining this.

[jerryjrchen]

Would it also be worth mentioning that time to run the body isn't accounted for in duration? I can see someone unfamiliar with the full details of polling confirmations writing something like:

      try await confirmation(until: .firstPass, within: .seconds(10)) {
        tenSecondLongOperation()
      }

And then being surprised it runs much longer than 10 seconds total.

[jerryjrchen]

Thanks for the awesome comprehensive tests!

[jerryjrchen]

I think this await is unnecessary:

Suggested change

	#expect(await count == 10)
	#expect(count == 10)

[younata]

I'm going to instead inline the await incrementor.count back into this and in the other places.

The reason this was separated out was to make debugging the test failure easier - the failure message when incrementor.count wasn't equal to 10 did not mention what the then-current value of incrementor was. But that's not necessary any longer.

[jerryjrchen]

Nice use of exit tests!