-
Notifications
You must be signed in to change notification settings - Fork 471
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
Re-enable catchpoint expect test #4209
Re-enable catchpoint expect test #4209
Conversation
Codecov Report
@@ Coverage Diff @@
## feature/320-rounds #4209 +/- ##
======================================================
- Coverage 55.22% 55.21% -0.02%
======================================================
Files 395 395
Lines 50073 50073
======================================================
- Hits 27653 27647 -6
- Misses 20056 20061 +5
- Partials 2364 2365 +1
Continue to review full report at Codecov.
|
@@ -2283,7 +2283,7 @@ func performOnlineAccountsTableMigration(ctx context.Context, tx *sql.Tx, log fu | |||
|
|||
// insert entries into online accounts table | |||
if ba.Status == basics.Online { | |||
if !normBal.Valid { | |||
if ba.MicroAlgos.Raw > 0 && !normBal.Valid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in e2e tests we have online accounts with zero balance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did they get set up with zero balances?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was caught by catchpointCatchupTest.exp
with the following genesis:
"Genesis": {
"NetworkName": "catchpointcatchupnetwork",
"ConsensusProtocol": "catchpointtestingprotocol",
"LastPartKeyRound": 3000,
"Wallets": [
{
"Name": "Wallet1",
"Stake": 100,
"Online": true
},
{
"Name": "Wallet2",
"Stake": 0,
"Online": true
}
]
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we allow this combo, we need to support it in code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I see. and in this case it is OK to keep accounts like this out of the online accounts table.
Adjust new catchpoint-related parameters to let the test run properly:
CatchpointLookback
to the same value asMaxBalLookback
MaxAcctLookback
to a small value to allow second stage of the catchpoint generation complete fast.