test(flaky): Log checkExpiring failure #1018
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #1018 +/- ##
=========================================
Coverage 83.18% 83.18%
=========================================
Files 91 91
Lines 15349 15349
=========================================
Hits 12768 12768
Misses 2581 2581 |
| @@ -122,7 +122,8 @@ proc checkExpiringInternal(cond: proc(): bool {.raises: [], gcsafe.} ): Future[b | |||
| await sleepAsync(1.millis) | |||
|
|
|||
| template checkExpiring*(code: untyped): untyped = | |||
| check await checkExpiringInternal(proc(): bool = code) | |||
| let result = await checkExpiringInternal(proc(): bool = code) | |||
| assert result, "[TIMEOUT] Test failed due to the check timeout. Consider adjusting it." | |||
There was a problem hiding this comment.
I believe this is gonna change the current output that shows the current and expected values.
There was a problem hiding this comment.
@AlejandroCabeza could you please make sure the current behavior isn't changed? Isn't it possible to add this message to the check proc?
There was a problem hiding this comment.
As far as I know, there's no way to add a message to the check template; hence the change. That's why I suggested the possibility of PRing the unittest library with said change, in the case somebody's able to implement it.
In any case, and answering your first comment: I think this shouldn't change the output, at least it doesn't seem that way. Having a closer look at checkExpiring you can notice the previous check was done over the returning value of checkExpiringInternal, which in turn is a simplified bool.
I believe the better output message that check generally outputs is due to it receving the raw conditions and parsing the syntax tree, which is not possible as this is currently defined. Due to that, I also believe assert will not change in any way the behaviour previously defined.
There was a problem hiding this comment.
You're right, it shouldn't change the current behavior. But it'd probably be better to make checkExpiring behave like check, if it's possible to do that. Timeout isn't the only possible failure if the conditions are always false. Does it make sense?
There was a problem hiding this comment.
It'd be nice to make it behave like so, but I don't consider myself capable of manipulating Nim's syntax trees just yet.
There was a problem hiding this comment.
My last comment is wrong. When the conditions are always false, it'll always fail with a timeout, but we won't see what the different values are.
There was a problem hiding this comment.
Because if it fails with a timeout it's because the async proc won't have completed, meaning there are no values to check against.
There was a problem hiding this comment.
we can try to do it in another PR.
AlejandroCabeza
force-pushed
the
log-check-expiring
branch
from
189a6ee
to
77519a7
Compare
Changed
checktoassertto leverage the capability of returning a message on failure, ascheckdoes not have that capability (that I know of) and using logs (e.g.: error, debug) was not working for me.