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

test(sync): stage test suite #204

Merged
merged 18 commits into from
Nov 19, 2022
Merged

test(sync): stage test suite #204

merged 18 commits into from
Nov 19, 2022

Conversation

rkrasiuk
Copy link
Member

@rkrasiuk rkrasiuk commented Nov 15, 2022

Create a common test suite for stages.

The stage_test_suite macro contains the default stage tests impl:

  • call Stage::execute with empty db
  • call Stage::execute with input stage_progress equal to prev_stage_progress
  • call Stage::execute with normal input and validate the database entries
  • call Stage::unwind with empty db
  • call Stage::execute and call Stage::unwind to the original stage_progress. validate the database state after each call

@rkrasiuk rkrasiuk marked this pull request as draft November 15, 2022 11:48
@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 Nov 16, 2022
@rkrasiuk
Copy link
Member Author

@onbjerg one more thing i've noticed is that some bodies tests are flaky. full_body_download or partial_body_download can fail once a while

@rkrasiuk rkrasiuk marked this pull request as ready for review November 18, 2022 08:21
@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Merging #204 (14e1800) into main (6b336c6) will decrease coverage by 0.87%.
The diff coverage is 98.25%.

❗ Current head 14e1800 differs from pull request most recent head deb0807. Consider uploading reports for the commit deb0807 to get more accurate results

@@            Coverage Diff             @@
##             main     #204      +/-   ##
==========================================
- Coverage   67.74%   66.86%   -0.88%     
==========================================
  Files         213      217       +4     
  Lines       18142    18205      +63     
==========================================
- Hits        12290    12173     -117     
- Misses       5852     6032     +180     
Impacted Files Coverage Δ
crates/stages/src/lib.rs 100.00% <ø> (ø)
crates/stages/src/util.rs 100.00% <ø> (+1.04%) ⬆️
crates/interfaces/src/test_utils/headers.rs 88.79% <91.89%> (+2.12%) ⬆️
crates/stages/src/test_utils/runner.rs 96.55% <96.55%> (ø)
crates/stages/src/stages/bodies.rs 95.39% <96.87%> (-1.09%) ⬇️
crates/db/src/kv/mod.rs 94.83% <100.00%> (ø)
crates/net/headers-downloaders/src/linear.rs 95.43% <100.00%> (ø)
crates/stages/src/stages/headers.rs 96.98% <100.00%> (-0.45%) ⬇️
crates/stages/src/stages/tx_index.rs 96.33% <100.00%> (+1.65%) ⬆️
crates/stages/src/test_utils/macros.rs 100.00% <100.00%> (ø)
... and 32 more

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

@onbjerg
Copy link
Member

onbjerg commented Nov 18, 2022

@onbjerg one more thing i've noticed is that some bodies tests are flaky. full_body_download or partial_body_download can fail once a while

This is likely because the blocks are randomly generated and empty blocks are skipped, i.e. if a block has no ommers and no transactions we don't request it from the network. So the amount of blocks processed by the stage might deviate a bit

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

A few questions, but also unsure how to validate if the body tests do the same things as before/cover the same edge cases. Any ideas on how we could do that?

crates/stages/src/stages/bodies.rs Outdated Show resolved Hide resolved
crates/stages/src/stages/bodies.rs Show resolved Hide resolved
crates/stages/src/stages/bodies.rs Outdated Show resolved Hide resolved
crates/stages/src/stages/bodies.rs Show resolved Hide resolved
@rkrasiuk
Copy link
Member Author

@onbjerg not sure other than the manual inspection. i removed the tests that are covered by the stage_test_suite, but left all the edge cases. dedupped code in the impl of ExecuteStageTestRunner and UnwindStageTestRunner

@onbjerg
Copy link
Member

onbjerg commented Nov 18, 2022

@rkrasiuk Seems like the same lines are covered in both stages, so should be good

@gakonst
Copy link
Member

gakonst commented Nov 19, 2022

@rkrasiuk any idea why f860865 deadlocks? i reverted and merged but it seems weird to me

@gakonst gakonst merged commit 4936d46 into main Nov 19, 2022
@gakonst gakonst deleted the rkrasiuk/stage-test-suite branch November 19, 2022 01:57
@rkrasiuk
Copy link
Member Author

@gakonst yeah, for headers you need to update chain tip before await the value

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.

None yet

4 participants