swiftlang / swift-testing

A modern, expressive testing package for Swift
Apache License 2.0
1.71k stars 69 forks source link

Add support for function call order expection #297

Open Kyle-Ye opened 6 months ago

Kyle-Ye commented 6 months ago

Description

For async call or blocking call, we want to test the executing order. I'd like to propose an API set similar to confirmation

Expected behavior

public struct Order4 {
   public let 1: Child
   public let 2: Child
   public let 3: Child
   public let 4: Child

   public struct Child {
      ...
   }
}
extension Order.Child {
  public func callAsFunction() {
     ...
  }
}

func order2<R>(_ body: (Order2) async throw -> R)
func order3<R>(_ body: (Order3) async throw -> R)
func order4<R>(_ body: (Order4) async throw -> R)

@Test
func orderTest() {
  order3 { order in
    order.1()
    await Task {
      ...
      lock.wait()
      order.3()
    }
    await Task {
      ...
      order.2()
      lock.broadcast()
    }
  }
}

Actual behavior

No response

Steps to reproduce

No response

swift-testing version/commit hash

No response

Swift & OS version (output of swift --version && uname -a)

No response

grynspan commented 6 months ago

Most scenarios that might need something like this are also solved by Swift concurrency, which is why we didn't implement an interface for it. For instance, if you expect functions A, B, and C to execute in that order, async/await means it's usually possible to force that to happen just by writing your calls to them in the right order.

Is there a scenario you're thinking of where that isn't possible today?

Kyle-Ye commented 6 months ago

Most scenarios that might need something like this are also solved by Swift concurrency, which is why we didn't implement an interface for it. For instance, if you expect functions A, B, and C to execute in that order, async/await means it's usually possible to force that to happen just by writing your calls to them in the right order.

Is there a scenario you're thinking of where that isn't possible today?

Yes. For normal async/await Swift concurrency scenarios, there is no problem.

But for C-Style like POSIX lock API, it does not support very well.

The Task and await code are just pseudo code to create 2 pthread. The core issue of the above example is to test the lock's behavior where Swift concurrency is not available.

However, such scenario should be very rare. Almost only a few low-level libraries that need to deal directly with the C language need such usage.

If upstream does not plan to support this feature, I totally understand the concern.

grynspan commented 6 months ago

swift-testing is, of course, a Swift testing library. C code that behaves well when imported into Swift is testable too, but esoteric C behaviour is beyond the library's purview. That's not a no to this feature request though: do you have a real-world example of C code that you need to use from Swift which you cannot turn into async/await code using continuations, tasks, task groups, etc.?

Something we could look at doing is using variadic generics and adding an overload of confirmation() à la:

await confirmation("first thing happened", "second thing happened", ..., strictlyOrdered: true) { first, second, ... in
}

This would help with the "arrow of death" pattern too.

Kyle-Ye commented 6 months ago

swift-testing is, of course, a Swift testing library. C code that behaves well when imported into Swift is testable too, but esoteric C behaviour is beyond the library's purview. That's not a no to this feature request though: do you have a real-world example of C code that you need to use from Swift which you cannot turn into async/await code using continuations, tasks, task groups, etc.?

The example is the lock API usage in almost every large iOS projects (NSLock, os_unfair_lock and so on).

We have a wide usage of them in our ObjectiveC and Swift codebase. And we probably can convert them into Swift Concurrency but it's not on the plan.

From https://developer.apple.com/videos/play/wwdc2021/10254/?time=1507 Primitives like os_unfair_locks and NSLocks are also safe but caution is required when using them. Using a lock in synchronous code is safe when used for data synchronization around a tight, well-known critical section.

The core issue is we'd like to write test case for them easily. The current test implementation is a little tricky and unstable.

Something we could look at doing is using variadic generics and adding an overload of confirmation() à la:

+1 for this idea/implementation. Look forward to seeing the landing PR.

grynspan commented 6 months ago

So, for NSLock and os_unfair_lock/OSAllocatedUnfairLock, the primitive locking/unlocking functions are barred from use in async contexts and you should use the withLock {} API instead. This API has a built-in guarantee that the order of operations is correct, which eliminates the need for you to write a test that checks the order of operations. Hooray! 🥳

I can certainly understand that there is code out there that cannot use the provided withLock {} API (swift-testing itself can't for layering/dependency reasons) but since these APIs must not be used in an async context anyway, simply ordering your calls correctly is sufficient to ensure they execute in the right order.

That's not me saying "no, we won't build this feature", to be clear! But if we have a concrete, real-world use case that isn't trivially solved without the API, then that makes this more urgent to implement (vs. being a nice addition to the API, but low-priority.)

Kyle-Ye commented 6 months ago

So, for NSLock and os_unfair_lock/OSAllocatedUnfairLock, the primitive locking/unlocking functions are barred from use in async contexts and you should use the withLock {} API instead. This API has a built-in guarantee that the order of operations is correct, which eliminates the need for you to write a test that checks the order of operations. Hooray! 🥳

Sorry for the misunderstanding caused here. What we actually want to test here is something lower level than withLock.

The example is our cross-platform lock/condition implementation and we want to test the wait and broadcast API here.

https://developer.apple.com/documentation/foundation/nscondition/1407950-wait https://developer.apple.com/documentation/foundation/nscondition/1415094-broadcast

// Thread 1
func thread_1() {
    ...
    lock.wait()
    order.3()
    ...
}

// Thread 2
func thread_2() {
    ...
    lock.wait()
    order.4()
    ...
}

// Thread 3
func thread_3() {
    ...
    order.2()
    lock.broadcast()
}

@Test
func orderTest() {
    ...
    order.1()
    // Kicks thread 1 and thread 2's work
    ...
    // Kicks thread 3
    ...
}

After broadercast is called we'd like to confirm the wait is over and make it continue to process its task.

grynspan commented 6 months ago

I would not recommend using NSCondition in Swift, especially in concurrent code. I know that's not a very satisfactory answer, but NSCondition is generally incompatible with Swift concurrency because it may cause a thread owned by Swift's concurrent thread pool to block indefinitely or even deadlock.

It is generally safe to assume that NSCondition itself already has unit tests in Foundation that check that it behaves as designed, so you don't need to write your own unit tests just to check NSCondition itself.

Consider reimplementing your code to use Swift concurrency instead of a condition lock, and you may find that you (again!) don't need a dedicated interface in swift-testing to ensure your code operates correctly. 🙃

Kyle-Ye commented 6 months ago

It is generally safe to assume that NSCondition itself already has unit tests in Foundation that check that it behaves as designed, so you don't need to write your own unit tests just to check NSCondition itself.

Got it. But the problem is we are using our own implementation of XXCondition instead of Apple's Foundation.NSCondition. So I think we'd better add test case for it.

I would not recommend using NSCondition in Swift, especially in concurrent code.

Got it. But that's a fairly old code in the codebase we cannot have a rewrite. I guess the best solution is just do not make the new Swift code to have a dependency on it.

Anyway thanks for your time on this issue. If upstream has plans to support such API later, we can keep this issue open, otherwise let's just close it.

grynspan commented 6 months ago

We can keep this issue open as there's still value in the proposed API.