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

testing: fix bug in TestAssetGroupCreateSendDestroy #3631

Conversation

tsachiherman
Copy link
Contributor

Summary

Fix a bug in TestAssetGroupCreateSendDestroy. The test uses two nodes, sending transaction to one and checking the state on the other. Before checking, the test need to verify that both nodes successfully reached the same round number.

Test Plan

This is a test.

@codecov-commenter
Copy link

codecov-commenter commented Feb 15, 2022

Codecov Report

Merging #3631 (2e2f638) into master (73ff45d) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3631      +/-   ##
==========================================
- Coverage   48.06%   48.04%   -0.03%     
==========================================
  Files         381      381              
  Lines       62080    62080              
==========================================
- Hits        29838    29825      -13     
- Misses      28822    28828       +6     
- Partials     3420     3427       +7     
Impacted Files Coverage Δ
cmd/algoh/blockWatcher.go 77.77% <0.00%> (-3.18%) ⬇️
ledger/tracker.go 74.45% <0.00%> (-1.30%) ⬇️
ledger/acctupdates.go 65.46% <0.00%> (-0.95%) ⬇️
data/abi/abi_type.go 87.67% <0.00%> (-0.95%) ⬇️
data/transactions/verify/txn.go 44.15% <0.00%> (-0.87%) ⬇️
network/wsNetwork.go 62.79% <0.00%> (-0.20%) ⬇️
catchup/service.go 69.38% <0.00%> (ø)
network/wsPeer.go 68.33% <0.00%> (+0.27%) ⬆️
network/requestTracker.go 71.12% <0.00%> (+0.86%) ⬆️

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 73ff45d...2e2f638. Read the comment docs.

Copy link
Contributor

@algonautshant algonautshant left a comment

Choose a reason for hiding this comment

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

Looks great.
Just one comment to confirm my understanding.

a.NoError(err)

// wait for client1 to reach the same round as client0
_, err = client1.WaitForRound(status0.LastRound)
Copy link
Contributor

Choose a reason for hiding this comment

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

So, I understand that the idea here is that both nodes agree on the block with all the transactions, client0 adds the block in its ledger, returns confirmed on line 638.

However, it could be possible that client1 has not yet added the block and advanced the round, and the client1.AssetInformation(assetID1) fails.

So, by client1.WaitForRound(status0.LastRound) you are making sure that client1 has also added the block before calling client1.AssetInformation(assetID1).

Is this right?

@tsachiherman
Copy link
Contributor Author

tsachiherman commented Feb 15, 2022 via email

@tsachiherman tsachiherman merged commit 04e69d4 into algorand:master Feb 15, 2022
@tsachiherman tsachiherman deleted the tsachi/fixTestAssetGroupCreateSendDestroy branch February 15, 2022 04:07
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.

3 participants