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

Algod: Use future consensus version to test with no low resources #5362

Merged

Conversation

jasonpaulos
Copy link
Contributor

@jasonpaulos jasonpaulos commented May 6, 2023

Summary

#5328 changed the initial txn counter from 0 to 1000, and simulate tests relied on this starting at 0 to predict app and asset IDs. Since we were testing with the current consensus version, not future, we didn't see any problems.

Since it's best to fix tests at development time instead of release time, I changed the relevant testing infrastructure to use consensus future instead of current. I've also fixed the simulate tests to work with a higher initial txn counter.

Note: this PR is not exhaustive. There seem to be quite a lot of tests which use the current consensus version, so it's possible more issues like this are present.

Test Plan

Tests modified.

@jasonpaulos jasonpaulos self-assigned this May 6, 2023

initBlock := bookkeeping.Block{
Copy link
Contributor Author

@jasonpaulos jasonpaulos May 6, 2023

Choose a reason for hiding this comment

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

This code is the same as bookkeeping.MakeGenesisBlock, so instead of adding to the duplication to account for changing the initial txn counter, I make it call that function directly

@codecov
Copy link

codecov bot commented May 6, 2023

Codecov Report

Merging #5362 (90e45cb) into master (9e980d3) will increase coverage by 0.03%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master    #5362      +/-   ##
==========================================
+ Coverage   55.28%   55.31%   +0.03%     
==========================================
  Files         454      454              
  Lines       63772    63754      -18     
==========================================
+ Hits        35254    35267      +13     
+ Misses      26115    26083      -32     
- Partials     2403     2404       +1     
Impacted Files Coverage Δ
ledger/testing/initState.go 18.60% <0.00%> (+5.48%) ⬆️
daemon/algod/api/server/v2/test/helpers.go 74.74% <100.00%> (ø)

... and 16 files with indirect coverage changes

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

Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

lgtm, the initState.go file change seems to be directly adapting MakeGenesisBlock.

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