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

Tests: Fix txid-string comparison #4792

Merged
merged 4 commits into from
Nov 16, 2022

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented Nov 15, 2022

Summary

This PR fixes the build where there was a type mismatch in an assertion between the TxID bytes and string. One existing test failed if the transaction was not committed in the next 2 rounds, so this PR adds a loop to wait if that is the case.

Test Plan

Re-run tests.

@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Merging #4792 (aed0406) into master (23890a8) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4792      +/-   ##
==========================================
- Coverage   54.68%   54.67%   -0.01%     
==========================================
  Files         414      414              
  Lines       53550    53550              
==========================================
- Hits        29286    29281       -5     
- Misses      21836    21842       +6     
+ Partials     2428     2427       -1     
Impacted Files Coverage Δ
network/wsPeer.go 66.50% <0.00%> (-2.43%) ⬇️
agreement/cryptoVerifier.go 67.60% <0.00%> (-2.12%) ⬇️
agreement/proposalManager.go 96.07% <0.00%> (-1.97%) ⬇️
catchup/service.go 68.88% <0.00%> (-0.50%) ⬇️
ledger/acctupdates.go 69.26% <0.00%> (-0.25%) ⬇️
network/wsNetwork.go 65.52% <0.00%> (+0.18%) ⬆️
ledger/testing/randomAccounts.go 57.23% <0.00%> (+0.61%) ⬆️
crypto/merkletrie/node.go 93.48% <0.00%> (+1.86%) ⬆️
crypto/merkletrie/trie.go 68.61% <0.00%> (+2.18%) ⬆️
ledger/roundlru.go 96.22% <0.00%> (+5.66%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

This looks like a good improvement, both because of the fixed comparison and because of the more responsive waiting period.

a.Equal(uint64(0), pendingTx.TotalTransactions)
// Try polling 10 rounds to ensure txn is committed.
isCommitted := false
for i := 0; i < 10; i++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using WaitForTxnConfirmation

@michaeldiamant michaeldiamant marked this pull request as ready for review November 16, 2022 13:01
Copy link
Contributor

@michaeldiamant michaeldiamant left a comment

Choose a reason for hiding this comment

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

@algochoi Thanks for supplying these fixes!

@michaeldiamant michaeldiamant merged commit 6b85209 into algorand:master Nov 16, 2022
@algochoi algochoi deleted the fix-txid-string branch November 16, 2022 20:20
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.

4 participants