Quick / Nimble

A Matcher Framework for Swift and Objective-C
https://quick.github.io/Nimble/documentation/nimble/
Apache License 2.0
4.79k stars 595 forks source link

Memoize errors #1118

Open fabianmuecke opened 5 months ago

fabianmuecke commented 5 months ago

[!NOTE] This is more of a feature request, than a bug report.

I noticed that errors thrown by an expression do not get memoized the way result values do. I believe this would be very simple to change, but before I make a PR for that, I'd like to know, if you'd consider merging that or if such a change in behavior is not desired.

What did you do?

I wrote an extension function which allows to add a failure on timeout of an AsyncExpectation. For this to work properly, the expression needs to be evaluated at least twice - once for the possible timeout failure, and once for all other test conditions. I then used this extension function with an AsyncExpectation that expected an error to be thrown and later checked the call count property of a mock used by this test.

What did you expect to happen?

I expected the call count of the mock to be 6.

What actually happened instead?

I noticed that the call count was 12, so twice as high as I had expected, which lead me to believe, that the error leads to the expression being evaluated twice instead of just once in case of an error. A quick look at the implementations of memoizedClosure in Expression and AsyncExpression confirmed my suspicion. Errors are not cached.

Environment

List the software versions you're using:

Please also mention which package manager you used and its version. Delete the other package managers in this list:

Suggested improvement

My suggestion would be to change e.g. the memoizedClosure function of AsyncExpression like this:

// Memoizes the given closure, only calling the passed
// closure once; even if repeat calls to the returned closure
private func memoizedClosure<T>(_ closure: @escaping () async throws -> T) -> (Bool) async throws -> T {
    var cache: Result<T, Error>?
    return { withoutCaching in
        if withoutCaching || cache == nil {
            cache = Result(catching: { try await closure() })
        }
        return try cache!.get()
    }
}

and the memoizedClosure function of Expression accordingly. If you think that would be a good addition, I'll gladly make a PR for that. 🙏