-
Notifications
You must be signed in to change notification settings - Fork 75
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
Conversation
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.
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) |
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.
What is this change for? Reverting this doesn't seem to break anything. (Also goes for the branchResolutionRequestSize = 30
changes.)
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.
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.
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 |
7be749c
to
6770cfa
Compare
6770cfa
to
675b893
Compare
Description
Integration tests added to simulate peers exchanging the checkpoints