Skip to content

test: cover index account history stage with stage_test_suite_ext tests#3383

Merged
shekhirin merged 5 commits intoparadigmxyz:mainfrom
int88:test-suite
Jul 24, 2023
Merged

test: cover index account history stage with stage_test_suite_ext tests#3383
shekhirin merged 5 commits intoparadigmxyz:mainfrom
int88:test-suite

Conversation

@int88
Copy link
Contributor

@int88 int88 commented Jun 26, 2023

fixes part of #3232

@int88
Copy link
Contributor Author

int88 commented Jun 26, 2023

@shekhirin PTAL :)

@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Merging #3383 (28410ad) into main (2e436b5) will decrease coverage by 0.05%.
The diff coverage is 91.00%.

Impacted file tree graph

Impacted Files Coverage Δ
crates/stages/src/stages/index_account_history.rs 96.24% <91.00%> (-0.51%) ⬇️

... and 213 files with indirect coverage changes

Flag Coverage Δ
integration-tests 15.51% <0.00%> (-0.59%) ⬇️
unit-tests 64.16% <91.00%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
reth binary 26.48% <ø> (+0.06%) ⬆️
blockchain tree 83.02% <ø> (+1.77%) ⬆️
pipeline 89.64% <91.00%> (+2.73%) ⬆️
storage (db) 74.26% <ø> (+0.77%) ⬆️
trie 94.65% <ø> (-0.45%) ⬇️
txpool 45.98% <ø> (-3.84%) ⬇️
networking 77.66% <ø> (-0.24%) ⬇️
rpc 57.54% <ø> (-0.53%) ⬇️
consensus 65.61% <ø> (+2.91%) ⬆️
revm 33.73% <ø> (-1.22%) ⬇️
payload builder 6.61% <ø> (-0.23%) ⬇️
primitives 88.05% <ø> (-0.22%) ⬇️

@onbjerg onbjerg added A-staged-sync Related to staged sync (pipelines and stages) C-test A change that impacts how or what we test labels Jun 26, 2023
Copy link
Member

Choose a reason for hiding this comment

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

I think what we want here is to seed the database with some random blocks and account changesets, as we do in e.g. Merkle stage:

fn seed_execution(&mut self, input: ExecInput) -> Result<Self::Seed, TestRunnerError> {

Then, in validate_execution we should dynamically calculate the shards from database data, and compare it with the contents of AccountHistory table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shekhirin updated! PTAL

@onbjerg onbjerg requested a review from shekhirin July 5, 2023 12:54
@onbjerg
Copy link
Collaborator

onbjerg commented Jul 10, 2023

Ping @shekhirin

Copy link
Member

@shekhirin shekhirin left a comment

Choose a reason for hiding this comment

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

LGTM, small nit. Sorry for the slow review 😑

Would also like to get @rakita review on this tests since he wrote the stage implementation

.collect::<Vec<_>>();
let last_chunk = chunks.pop();

chunks.into_iter().try_for_each(|list| -> Result<_, TestRunnerError> {
Copy link
Member

Choose a reason for hiding this comment

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

I don't see any ? or return Err, is this closure fallible? If not, we can do for_each

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shekhirin updated!

@shekhirin shekhirin requested a review from rakita July 12, 2023 15:42
@int88
Copy link
Contributor Author

int88 commented Jul 19, 2023

friendly ping @rakita

};
use reth_primitives::{Address, BlockNumber, H256};

stage_test_suite_ext!(IndexAccountHistoryTestRunner, index_account_history);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would move this line as it is in between all includes and put it before IndexAccountHistoryTestRunner

and remove all new lines here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rakita updated! :)

Copy link
Collaborator

@rakita rakita left a comment

Choose a reason for hiding this comment

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

one nit, but otherwise it lgtm!

Thank you for the ping and I apologize for missing this I was on vacation week before.

@rakita rakita added this pull request to the merge queue Jul 19, 2023
@onbjerg onbjerg removed this pull request from the merge queue due to a manual request Jul 19, 2023
@onbjerg onbjerg added this pull request to the merge queue Jul 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 19, 2023
@shekhirin shekhirin added this pull request to the merge queue Jul 21, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 21, 2023
@shekhirin shekhirin added this pull request to the merge queue Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-staged-sync Related to staged sync (pipelines and stages) C-test A change that impacts how or what we test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants