prefetcher: add Eval vs prefetcher alignment tests #3729
Conversation
| } | ||
|
|
||
| accounts, apps, assets, creators := prefetch(t, l, txn) | ||
| runEval(t, l, txn) |
There was a problem hiding this comment.
shouldn't runEval clear out the previously loaded requestedBalances ? otherwise, it wouldn't be able to properly compare the groups.
There was a problem hiding this comment.
Each test has its own l.
There was a problem hiding this comment.
yes, of course. But when you call the runEval, you should have a clean slate. Otherwise, you're testing commulative sum rather than distinct groups.
( don't forget that the prefetcher is calling the ledger to perform the loading of the resources.. which fills up the requestedBalances fields.. )
There was a problem hiding this comment.
right, will fix it
Codecov Report
@@ Coverage Diff @@
## master #3729 +/- ##
=======================================
Coverage 49.68% 49.68%
=======================================
Files 392 392
Lines 68588 68591 +3
=======================================
+ Hits 34076 34078 +2
- Misses 30766 30768 +2
+ Partials 3746 3745 -1
Continue to review full report at Codecov.
|
| if !stxn.Txn.AssetSender.IsZero() { | ||
| loadAccountsAddResourceTask(nil, basics.CreatableIndex(stxn.Txn.XferAsset), basics.AssetCreatable, task, resourceTasks, queue) |
There was a problem hiding this comment.
in the case of AssetTransferTx with non-zero AssetSender, you've removed the loading of the Txn.Sender asset resource loading ( i.e. clawback ). I'm pretty sure that in case of a clawback, we need to load the sender resources ( so we can claw them back ).
There was a problem hiding this comment.
Clawback will claw back AssetSender resources, not Sender.
| @@ -32,6 +32,22 @@ import ( | |||
| "github.com/algorand/go-algorand/test/partitiontest" | |||
| ) | |||
|
|
|||
| func acctAddrPtr(i int) (o *basics.Address) { | |||
There was a problem hiding this comment.
these function names were fine when these were local to a single benchmark. However, if you've moving these to the global space, please make sure the method name concisely describe the function behavior; i.e. makeTestingAddressPtr(addressSeed int)
tsachiherman
changed the title
There was a problem hiding this comment.
Reviewing the changes here - I think that the test itself is well written, and cover what we need.
One thing that I'd like to address here is the usage of global variable/function names. In this PR you've introduced few of them. While it's true that these apply only for testing, as the package grows, the global namespace pollution grows along. Since this is one of our most critical packages, I'd like to make sure it's very well maintained. When we have global variables defined in a test file, we need to name these in a way so that it won't collide with other tests. Examples for names that we should avoid in this PR are : "proto", "feeSink", "run".
If you were to make these scope-dependent, or just name them differently, i.e. makeTestingAddress, it would be just fine.
Agree, but I think the right solution is to split |
## Summary Tests for eval prefetcher checking that the prefetcher loads the same data (with some exceptions) that the evaluator requests. Closes algorand/go-algorand-internal#1922. ## Test Plan This is tests.
Summary
Tests for eval prefetcher checking that the prefetcher loads the same data (with some exceptions) that the evaluator requests.
Closes https://github.com/algorand/go-algorand-internal/issues/1922.
Test Plan
This is tests.