tests: use more realistic BlockEvaluator in ledger/ledger_test.go #6499
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6499 +/- ##
==========================================
+ Coverage 47.60% 47.69% +0.08%
==========================================
Files 666 657 -9
Lines 88508 87909 -599
==========================================
- Hits 42135 41926 -209
+ Misses 43609 43210 -399
- Partials 2764 2773 +9 ☔ View full report in Codecov by Sentry. |
cce
changed the title
jannotti
left a comment
jannotti
left a comment
There was a problem hiding this comment.
I can live with it as is. I had two thoughts.
| return l.evaluatorToBlock(t, eval) | ||
| } | ||
|
|
||
| func (l *Ledger) addBlockTxns(t *testing.T, accounts map[basics.Address]basics.AccountData, stxns []transactions.SignedTxn, ad transactions.ApplyData) error { |
There was a problem hiding this comment.
Maybe add comment that this isn't for groups. The stxns seem like they could be.
| badTx = correctPay | ||
| badTx.GenesisID = "invalid" | ||
| a.Error(l.appendUnvalidatedTx(t, initAccounts, initSecrets, badTx, ad), "added tx with invalid genesis ID") | ||
| a.ErrorContains(l.appendUnvalidatedTx(t, initAccounts, initSecrets, badTx, ad), "tx.GenesisID <invalid> does not match expected", "added tx with invalid genesis ID") |
There was a problem hiding this comment.
I find the "message" argument on this (and below) to be kind of pointless, and even a little confusing. I think they made sense when the call was a.Error() to explain, but now the text of the error is pretty descriptive!
algorandskiy
left a comment
algorandskiy
left a comment
There was a problem hiding this comment.
LGTM, couple minor comments below
| } | ||
|
|
||
| func (l *Ledger) appendUnvalidatedSignedTx(t *testing.T, initAccounts map[basics.Address]basics.AccountData, stx transactions.SignedTxn, ad transactions.ApplyData) error { | ||
| // appendInvalidSignedTx adds an invalid transaction using the old fake block method that bypasses BlockEvaluator. |
There was a problem hiding this comment.
Not sure I understand the "old fake block" portion. appendUnvalidated -> evaluatorToBlock calls ledger.Validate
| return l.evaluatorToBlock(t, eval) | ||
| } | ||
|
|
||
| func (l *Ledger) addBlockTxns(t *testing.T, accounts map[basics.Address]basics.AccountData, stxns []transactions.SignedTxn, ad transactions.ApplyData) error { |
There was a problem hiding this comment.
is there any difference in add vs append methods? a bit confusing naming
3 participants
This pull request refactors and improves the ledger test utilities and error checking in transaction validation tests. The main changes introduce clearer separation between validated and intentionally invalid transaction paths, enhance error assertions to check specific error messages, and update block creation for deterministic test behavior. These improvements help make the tests more robust and easier to understand and maintain.
Test utility refactoring and improvements:
ledger/ledger_test.goto allow explicit construction and addition of intentionally invalid transactions and blocks, such asappendInvalidTx,appendInvalidSignedTx, andaddBlockTxnsInvalid. These bypass normal validation to facilitate negative test cases. [1] [2] [3]addEmptyValidatedBlockto use the new block evaluator pattern for consistency and clarity in block addition.Test assertion improvements:
ErrorContainsto check for specific error messages, making tests more precise and easier to debug. This applies to transaction validity checks throughoutTestLedgerSingleTxandtestLedgerSingleTxApplyData. [1] [2] [3] [4] [5]Compatibility and maintenance:
ledger/ledger_test.goto include thecommitteepackage, supporting new block proposer logic in the refactored test helpers.Minor compatibility fix:
ledger/applications_test.go, updated a call fromappendUnvalidatedTxtoappendInvalidTxto use the new helper for constructing invalid application call transactions.