6

Is there a way to wait for an async call to be finished when this call is wrapped in another method?

class Owner{
   let dataManager = MockDataManager()
   var data: String? = nil

   func refresh() {
        Task {
            self.data = await dataManager.fetchData()
        }
    }
}

class MockDataManager {
    var testData: String = "test"
    func fetchData() async -> String {
       testData
    }
}

class OwnerTests: SKTestCase {
    private var owner = Owner()
    
    func testRefresh() {
        owner.refresh()
        XCTAssertEqual(owner.data, "test") // fail. the value is still nil
    }
}

With callbacks, the tests used to work if everything under the hood was replaced with synchronous calls but here it looks like i am missing an operation to wait for a change to owner.data

George
  • 332
  • 1
  • 4
  • 16
  • Related: https://stackoverflow.com/questions/71261288/xctest-wait-for-async-call-to-finish-in-synchronous-function – Cristik Jun 15 '22 at 15:22
  • 2
    Note that this isn't thread-safe code. If `refresh` is called multiple times, `data` may be corrupted. It feels like you really mean to have an `actor` here, which would also likely address some of your testing concerns. It may be helpful to show the code for "With callbacks, the tests used to work if everything under the hood was replaced with synchronous calls." In what way was that true (and was that code thread-safe)? Code with undefined behavior is definitely going to be very hard to test as a starting point. – Rob Napier Jun 16 '22 at 04:36
  • Hi George, did you find a solution? – Ivan M. Aug 11 '22 at 14:49

4 Answers4

4

Late to this party, but I agree with @Cristik regarding not changing the signature of a function just to accommodate testing. In the chat room conversation, @Cristik also pointed out a valid reason why a function can be set up to invoke an async function but yet not define its signature as async:

  • the Owner class may be in the nature of a view model (in an MVVM pattern) that exposes read-only observable/bindable (say @Published let) properties, that are bindable from (a) view(s), and the refresh function allows the view to request data update following a user event;
  • the refresh function isn't expected to return any data to the view when invoked, rather the view model (Owner) object will update the observable properties with the data returned while the views bound to (i.e. observing) the properties will be automatically updated.

In this case there's absolutely no need to mark the Owner.refresh() function as async and, thus forcing the view(s) to wrap their invocation of the refresh function in an async or Task (or .task modifier in SwiftUI) construct.

That said, I had similar situation and here's how I implemented the unit (not integration) test:

func testRefreshFunctionFetchesDataAndPopulatesFields() {        
  let expectation = XCTestExpectation(
    description: "Owner fetches data and updates properties."
  )

  // `Owner` is the "subject under test", so use protocol-driven development 
  // and dependency injection to enable focusing on testing just the SUT 
  // unencumbered by peculiarities of the dependency 
  let owner = Owner(mockDataManager: DataManagerProtocol())
        
  // Verify initial state
  XCTAssertNil(owner.data)

  owner.refresh()
        
  let asyncWaitDuration = 0.5 // <= could be even less than 0.5 seconds even
  DispatchQueue.main.asyncAfter(deadline: .now() + asyncWaitDuration) {
    expectation.fulfill()
            
    // Verify state after
    XCTAssertEqual(owner.data, "someString")
  }

  wait(for: [expectation], timeout: asyncWaitDuration)
}

Hope this helps.

Ugo
  • 587
  • 8
  • 12
  • Thanks this helped me! The only thing I changed was `wait(for: [expectation], timeout: asyncWaitDuration)` to `wait(for: [expectation], timeout: 10.0)` as this made my tests more reliable. – Brudus Sep 14 '22 at 19:24
  • 1
    It's an interesting idea. Although the test is rather flaky. As @Brudus wrote in the comment, he had to increase the time to 10 seconds and what if one has hundreds of such tests.. – Pavel Stepanov Jun 23 '23 at 18:06
2

The fact that refresh detaches some async code to do its job, is an implementation detail, and your tests should not care about the implementation details.

Instead, focus on the behaviour of the unit. For example, in the scenario you posted, the expected behaviour is that sometime after refresh is called, owner.data should become "test". This is what you should assert against.

Your current test code follows the above good practice, only that, as you observed, it fails because it doesn't wait until the property ends up being set. So, try to fix this, but without caring how the async part is implemented. This will make your tests more robust, and your code easier to refactor.

One possible approach for validating the async update is to use a custom XCTestExpectation:

final class PropertyExpectation<T: AnyObject, V: Equatable>: XCTNSPredicateExpectation {
    init(object: T, keyPath: KeyPath<T, V>, expectedValue: V) {
        let predicate = NSPredicate(block: { _, _ in
            return object[keyPath: keyPath] == expectedValue
        })
        super.init(predicate: predicate, object: nil)
    }
}

func testRefresh() {
    let exp = PropertyExpectation(object: owner, keyPath: \.data, expectedValue: "test")
    owner.refresh()

    wait(for: [exp], timeout: 5)
}

Alternatively, you can use a 3rd party library that comes with support for async assertions, like Nimble:

func testRefresh() {
    owner.refresh()
    expect(self.owner.data).toEventually(equal("test"))
}

As a side note, since your code is multithreaded, strongly recommending to add some synchronization in place, in order to avoid data races. The idiomatic way in regards to the structured concurrency is to convert your class into an actor, however, depending on how you're consuming the class from other parts of the code, it might not be a trivial task. Regardless, you should fix the data races conditions sooner rather than later.

Cristik
  • 30,989
  • 25
  • 91
  • 127
1

I would like to contribute a solution for a more restricted situation where XCTestExpectation doesn't work, and that's when a view model is bound to the @MainActor, and you can't make every function call async (relying on property didSet). Waiting for expectations will also block the task in question, even a detached task won't help, the task will always execute after the test function. Storing and later awaiting the task solves the problem:

@MainActor
class ViewModel {

  var task : Task<Void, Never>?

  @Published var value1: Int = 0 {
    didSet {
      task = Task {
        await update2()
      }
    }
  }

  @Published var value2: Int = 0

  func update2() async {
    value2 = value1 + 1
  }
}

And then in the test:

func testExample() {
    viewModel.value1 = 1
    let _ = await viewModel.task?.result
    XCTAssertEqual(viewModel.value2, 2)
}
Ivan M.
  • 2,941
  • 3
  • 13
  • 10
0
func refresh() async {
    self.data = await dataManager.fetchData()
}

then in the test await owner.refresh()

If you really need to wait synchronously for the async task, you can see this question Swift await/async - how to wait synchronously for an async task to complete?

aciniglio
  • 1,777
  • 1
  • 16
  • 18
  • I assume the asker doesn't want to convert to `async`, since that's likely the reason they created the `Task`. – Cristik Jun 15 '22 at 14:51
  • Cristik - He doesn't have to get rid of the old, non-`async` rendition, yet, if he doesn't want to. Personally, I do precisely what aciniglio advised, and just change that the old, non-`async` one to call this one, e.g. `Task { await refresh() }`. And I'd mark the non-`async` one as “deprecated” so I don’t forget to go back and clean up pre-concurrency implementations). That way, you have both renditions until I finish the migration to Swift concurrency. This pattern is shown repeatedly in [Swift concurrency: Update a sample app](https://developer.apple.com/videos/play/wwdc2021/10194/). – Rob Jun 15 '22 at 22:33
  • FWIW, the OP’s non-`async` rendition, without any completion handler, should be retired anyway (or at least add an optional completion handler). It’s inadvisable to have asynchronous methods that (a) do not offer the caller know when the asynchronous process is done; and (b) have nothing to indicate that they’re launching an asynchronous process. – Rob Jun 15 '22 at 22:37
  • 1
    @Rob it also depends on what API that class wants to expose, perhaps the class doesn't want to expose the async mechanism and wants to keep it an implementation detail, which should be fine, and we should not force changes in production code just for the sake of (unit) testing. But this is more of a philosophical discussion :) – Cristik Jun 16 '22 at 04:00
  • 1
    Yeah, we’ll have to agree to disagree on this one. There is zero downside to making this an async method, but has plenty of advantages (both for tests and, when you get around to it, the app, too). One should not contort oneself around the absence of a fundamental feature that should be present in all asynchronous code. – Rob Jun 16 '22 at 05:34
  • 2
    @Rob my point is that one should not change the signature of a function just to accommodate testing. Yes, if it makes sense for the project, do it, but if the only reasoning is to enable testing, then that's a big NO from me. – Cristik Jun 16 '22 at 07:58
  • Let us [continue this discussion in chat](https://chat.stackoverflow.com/rooms/245664/discussion-between-rob-and-cristik). – Rob Jun 16 '22 at 14:45