Skip to content

Conversation

@algonathan
Copy link
Contributor

@algonathan algonathan commented Feb 14, 2022

Summary

I've encountered an issue with the participation keys expiration test, which might cause flakiness.

Test Plan

I've managed to reproduce the same error as shown in
https://app.circleci.com/pipelines/github/algorand/go-algorand/5013/workflows/8c1f10b2-c827-4c0a-81a3-cfe18ec0c794/jobs/78980
By halting the secondary prior to the request of sclient for the round the rich client has.

@algonathan algonathan self-assigned this Feb 14, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2022

Codecov Report

Merging #3621 (8a2c0d4) into master (212edb3) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3621      +/-   ##
==========================================
- Coverage   48.06%   48.03%   -0.03%     
==========================================
  Files         381      381              
  Lines       62080    62080              
==========================================
- Hits        29836    29819      -17     
- Misses      28822    28835      +13     
- Partials     3422     3426       +4     
Impacted Files Coverage Δ
cmd/algoh/blockWatcher.go 77.77% <0.00%> (-3.18%) ⬇️
network/wsPeer.go 65.83% <0.00%> (-2.23%) ⬇️
crypto/merkletrie/trie.go 66.42% <0.00%> (-2.19%) ⬇️
crypto/merkletrie/node.go 91.62% <0.00%> (-1.87%) ⬇️
catchup/service.go 68.64% <0.00%> (-0.75%) ⬇️
network/wsNetwork.go 62.79% <0.00%> (-0.20%) ⬇️
ledger/acctupdates.go 66.41% <0.00%> (+0.94%) ⬆️

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 212edb3...8a2c0d4. Read the comment docs.

Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

The changes here aren't bad, and probably would fix the failure; however, I do have few comments :

  1. The test contains several calls to GetBalanceAndRound; most of them ignores the balance completely. Why don't we replace these with GetStatus() instead.
  2. The added sClient.WaitForRound(richAccountLatestRound) makes sense, however, I would suggest replacing it with sClient.WaitForRound(lastValidRound+1) which aligns with the round number that the primary node was waited for.

@tsachiherman tsachiherman changed the title Participation key tests testing: fix participation key expiration tests Feb 14, 2022
Copy link
Contributor

@tsachiherman tsachiherman 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. thanks for the changes.

@tsachiherman tsachiherman merged commit 73ff45d into algorand:master Feb 14, 2022
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