yield_resume: add more integration tests #11287
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #11287 +/- ##
==========================================
+ Coverage 70.99% 71.00% +0.01%
==========================================
Files 781 781
Lines 155507 155507
Branches 155507 155507
==========================================
+ Hits 110400 110420 +20
+ Misses 40330 40316 -14
+ Partials 4777 4771 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
|
||
| let mut tx_hashes = vec![]; | ||
|
|
||
| for i in 0..25 { |
There was a problem hiding this comment.
Is there not a good way to avoid hardcoding the number of transactions here? I feel like this test might lose its behaviour if the compute costs are reduced in the future.
There was a problem hiding this comment.
Unfortunately I couldn't think of a way to congest the chain which doesn't involve specifying some fixed number of transactions greater than 1.
| ); | ||
|
|
||
| // Allow two blocks for the function call to be executed | ||
| for i in 3..5 { |
There was a problem hiding this comment.
| for i in 3..5 { | |
| for i in 3..NEXT_BLOCK_HEIGHT_AFTER_SETUP { |
perhaps? I would also introduce a variable for the 3. An assert that the variabl does not exceed the constant wouldn't hurt either.
There was a problem hiding this comment.
I simply added a check at the end of prepare_env which gets the height of the head of the chain from one of the test clients and compares with NEXT_BLOCK_HEIGHT_AFTER_SETUP.
I think changing these constants to variables doesn't achieve much:
- It obfuscates what the loop wants to do (advance two blocks)
- The height
3is never referenced outside theprepare_envfunction
| } | ||
|
|
||
| /// In this test we introduce congestion to delay the yield timeout so that we can invoke | ||
| /// yield resume after the timeout height has passed. |
There was a problem hiding this comment.
We look for the invocation of the continuation function here, and not a transaction with the yield_resume host function after the timeout has occurred, right?
There was a problem hiding this comment.
Yes, we verify that both things occur.
The transaction invoking yield_create during env setup uses promise_return so that its final outcome is whatever the continuation function returns. The continuation function returns an indicator of whether it was invoked by timeout or with a user payload. At the end of the test we verify that the original transaction has the right outcome.
This PR adds several unit tests covering different scenarios for yield timeout delivery. There is also a minor bugfix for the case in which yield_resume is invoked in the same chunk in which the timeout is processed.
This PR adds several unit tests covering different scenarios for yield timeout delivery. There is also a minor bugfix for the case in which yield_resume is invoked in the same chunk in which the timeout is processed.
This reverts commit c637180.
This PR adds several unit tests covering different scenarios for yield timeout delivery. There is also a minor bugfix for the case in which yield_resume is invoked in the same chunk in which the timeout is processed.
This PR adds several unit tests covering different scenarios for yield timeout delivery.
There is also a minor bugfix for the case in which yield_resume is invoked in the same chunk in which the timeout is processed.