You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
I have read CONTRIBUTING and have done my best to follow them.
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:
Quick: none
Nimble: 13.2.0
Xcode Version: 15.0.1
Swift Version: Xcode Default
Please also mention which package manager you used and its version. Delete the
other package managers in this list:
Swift Package Manager - Swift 5.9.0
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 closureprivatefunc memoizedClosure<T>(_ closure:@escaping()asyncthrows->T)->(Bool)asyncthrows->T{varcache:Result<T,Error>?return{ withoutCaching in
if withoutCaching || cache ==nil{
cache =Result(catching:{tryawaitclosure()})}returntry 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. 🙏
The text was updated successfully, but these errors were encountered:
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 anAsyncExpectation
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
inExpression
andAsyncExpression
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 ofAsyncExpression
like this:and the
memoizedClosure
function ofExpression
accordingly. If you think that would be a good addition, I'll gladly make a PR for that. 🙏The text was updated successfully, but these errors were encountered: