Skip to content

ETCM-645: Add it-test for checkpointing #931

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

Merged
merged 3 commits into from
Mar 22, 2021
Merged

Conversation

AnastasiiaL
Copy link
Contributor

Description

Integration tests added to simulate peers exchanging the checkpoints

Copy link
Contributor

@jvdp jvdp left a comment

Choose a reason for hiding this comment

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

So the tests look OK to me, superficially. (This FakePeer interface seems convenient.) Have you found a way to break them that indicates they're testing the right thing?

@@ -329,7 +329,7 @@ class BlockImporter(
Right(Nil)
case UnknownBranch =>
val currentBlock = blocks.head.number.min(startingBlockNumber)
val goingBackTo = currentBlock - syncConfig.branchResolutionRequestSize
val goingBackTo = (currentBlock - syncConfig.branchResolutionRequestSize).max(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this change for? Reverting this doesn't seem to break anything. (Also goes for the branchResolutionRequestSize = 30 changes.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

before the change val goingBackTo could be set to negative numbers, this shouldn't be the case as the resolution of the branches can't start from before the genesis block.

I actually had this happening during tests -> hence the fix.

As for branchResolutionRequestSize values changes in tests, I just aligned them with the value we use in application.conf.

@AnastasiiaL
Copy link
Contributor Author

So the tests look OK to me, superficially. (This FakePeer interface seems convenient.) Have you found a way to break them that indicates they're testing the right thing?

not sure what you mean with breaking the tests, but changing the test setup breaks them easily

@bsuieric bsuieric self-requested a review February 26, 2021 08:05
@jvdp
Copy link
Contributor

jvdp commented Feb 26, 2021

not sure what you mean with breaking the tests, but changing the test setup breaks them easily

By disabling / breaking the feature they're supposed to test, I meant.

@AnastasiiaL
Copy link
Contributor Author

not sure what you mean with breaking the tests, but changing the test setup breaks them easily

By disabling / breaking the feature they're supposed to test, I meant.

checkpointing is a huuuge piece of functionality, touching a lot of layers, it's not something that can be commented out. I just added the tests that were supposed to be there long time ago

@AnastasiiaL AnastasiiaL force-pushed the feature/ETCM-645 branch 2 times, most recently from 7be749c to 6770cfa Compare March 1, 2021 12:45
@AnastasiiaL AnastasiiaL merged commit 23534aa into develop Mar 22, 2021
@dzajkowski dzajkowski deleted the feature/ETCM-645 branch April 9, 2021 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants