Tests: replaced toEventually with new waitUntilValue to simplify tests#2071
Conversation
There was a problem hiding this comment.
No longer need to specify types every time, now they're inferred by the method we call.
There was a problem hiding this comment.
Never mind, that only works in Xcode 14.
5cd1514 to
cc4fce1
Compare
vegaro
left a comment
There was a problem hiding this comment.
still have some files to check but looking good so far
There was a problem hiding this comment.
This can be:
let receivedError: Error? = waitUntilValue { completed in
self.orchestrator.showManageSubscription(completion: completed)
}
Correct? Not sure if that's more legible though
There was a problem hiding this comment.
Not this because of @Sendable shenanigans 🙃
There was a problem hiding this comment.
The 1 was passing because it is 1 at some point. But in practice once the operation changes this became 3. I suppose I should change this to >= 1 to match the same semantics as before, what do you think?
There was a problem hiding this comment.
Could >= 1 give a false positive? Right now, there are 3 invokes on the main thread, one of them being the restoring one, which is the one we are testing for. If that one stops being called on the main thread (total of 2 invokes on main thread), the test will still pass
There was a problem hiding this comment.
Probably, but that was an issue with the existing test as well.
There was a problem hiding this comment.
I looked into this more. This just looks like a bad test. The same operation dispatcher is used for all of Purchases, so other background tasks are calling dispatchOnMainThread as well. The test has always passed because it was effectively checking >= 1 with that toEventually. Eventually it was 1, even before restorePurchases was called, but that's not how the test was written.
Unfortunately can't think of a way to improve the test since we have no way to identify which operation was invoked.
Scratch that, of course there's a simple way haha
There was a problem hiding this comment.
Since the test is testRestorePurchasesCallsCompletionOnMainThreadWhenMissingReceipts I added this in the completion block:
expect(Thread.isMainThread) == truecc4fce1 to
5ad5c2d
Compare
5ad5c2d to
f362a08
Compare
vegaro
left a comment
There was a problem hiding this comment.
Just that question about the >= 1
…y tests
Example:
```diff
- var result: Result<Offerings, OfferingsManager.Error>?
- offeringsManager.offerings(appUserID: MockData.anyAppUserID) {
- result = $0
+ let result = waitUntilValue { completed in
+ self.offeringsManager.offerings(appUserID: MockData.anyAppUserID) {
+ completed($0)
+ }
}
- expect(result).toEventuallyNot(beNil())
```
This removes the need to _poll_ for when the value is returned, and instead it gets detected immediately, potentially providing a significant boost in test execution speed.
I've also used `Nimble`'s own `waitUntil` for cases where we don't need to return a value.
#### `toEventually` usage:
- Before this PR: 353
- After: 229
There's still a lot of legitimate usage of `toEventually`, but I'm sure I missed other places where this can also simplify the code.
This is a good start for now.
8b66ceb to
5f7d7fb
Compare
Example:
This removes the need to poll for when the value is returned, and instead it gets detected immediately, potentially providing a significant boost in test execution speed.
I've also used
Nimble's ownwaitUntilfor cases where we don't need to return a value.toEventuallyusage:There's still a lot of legitimate usage of
toEventually, but I'm sure I missed other places where this can also simplify the code. This is a good start for now.