Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convenient and High-fidelity Transaction Processing Tests #2730

Merged
merged 5 commits into from
Aug 13, 2021

Conversation

jannotti
Copy link
Contributor

This refactors some of the ledger code so that it's convenient
to write short tests that operate on a fairly complete ledger,
including rewards payouts. Previously, tests used genesis(), but
genesis() build the genesis block internally "by hand" rather
than using MakeGenesisBlock, so it missed some details (like
setting up RewardsState). Presumably, this was because
MakeGenesisBlock was in the data package, and could not be
imported. That is the motivation behind moving it, and some
related code, to bookkeeping (where various Genesis related code
already existed).

The txntest packaged is motivated purely by a desire for more
concise tests. It allows for the construction of
transaction.Transaction objects concisely, and we can add all
sort of conveneince routines here that would not make sense in
the production code (turning these into SignedTxns,
SignedTxnWithADs, TransactionGroups, etc).

This refactors some of the `ledger` code so that it's convenient
to write short tests that operate on a fairly complete ledger,
including rewards payouts.  Previously, tests used genesis(), but
genesis() build the genesis block internally "by hand" rather
than using MakeGenesisBlock, so it missed some details (like
setting up RewardsState). Presumably, this was because
MakeGenesisBlock was in the `data` package, and could not be
imported.  That is the motivation behind moving it, and some
related code, to bookkeeping (where various Genesis related code
already existed).

The txntest packaged is motivated purely by a desire for more
concise tests.  It allows for the construction of
transaction.Transaction objects concisely, and we can add all
sort of conveneince routines here that would not make sense in
the production code (turning these into SignedTxns,
SignedTxnWithADs, TransactionGroups, etc).
Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't spend some time after merging it and unify some tests to new functions?

ledger/eval_test.go Outdated Show resolved Hide resolved
data/ledger.go Show resolved Hide resolved
data/txntest/txn.go Show resolved Hide resolved
Copy link
Contributor

@algochoi algochoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like the change! I think it will make testing transactions much simpler

return ad
}

func (eval *BlockEvaluator) txn(t testing.TB, txn *txntest.Txn) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to rename this to something more detailed than txn or txns? Unfortunately I don't have a much better suggestion myself than signAndTestTxn.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't sign them. It's a big lie.

I might not object to testTxn, except there is already testTransaction(). testTransaction() does a bunch of tests, but does not actually push it through the evaluator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, just read the comment above about the unsigned SignedTxn(). Maybe something like prepareAndTestTxn? If not, we can leave it

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Let's someone from other teams be familiar with new test harness.

@algorandskiy
Copy link
Contributor

@tolikzinovyev mind checking how does TestModifiedAssetHoldings look with new testing api?

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2021

Codecov Report

Merging #2730 (7e44635) into master (848f132) will increase coverage by 0.01%.
The diff coverage is 2.08%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2730      +/-   ##
==========================================
+ Coverage   47.11%   47.13%   +0.01%     
==========================================
  Files         350      349       -1     
  Lines       56321    56321              
==========================================
+ Hits        26538    26545       +7     
+ Misses      26810    26804       -6     
+ Partials     2973     2972       -1     
Impacted Files Coverage Δ
data/bookkeeping/genesis.go 0.00% <0.00%> (ø)
data/ledger.go 24.08% <0.00%> (+4.56%) ⬆️
node/node.go 3.78% <0.00%> (ø)
daemon/algod/api/server/v2/test/helpers.go 77.09% <100.00%> (ø)
ledger/roundlru.go 90.56% <0.00%> (-5.67%) ⬇️
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
ledger/acctupdates.go 62.13% <0.00%> (-0.51%) ⬇️
ledger/eval.go 76.12% <0.00%> (+0.14%) ⬆️
crypto/merkletrie/node.go 93.48% <0.00%> (+1.86%) ⬆️
crypto/merkletrie/trie.go 68.61% <0.00%> (+2.18%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 848f132...7e44635. Read the comment docs.

@jannotti jannotti merged commit 6a63559 into algorand:master Aug 13, 2021
michaeldiamant added a commit to michaeldiamant/go-algorand that referenced this pull request Feb 4, 2022
Adds commentary based on context found in algorand#2730.
jannotti pushed a commit that referenced this pull request Feb 8, 2022
Adds commentary based on context found in #2730.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants