Implement unit tests for Block Synchronization Mechanism - Closes #4375 #4476
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2snEM6
force-pushed
the
fcs_unit_tests
branch
2 times, most recently
from
49721e1
to
c77a977
Compare
2snEM6
force-pushed
the
bsm_unit_tests
branch
2 times, most recently
from
d08f368
to
931e4fa
Compare
shuse2
changed the title
2snEM6
force-pushed
the
fcs_unit_tests
branch
3 times, most recently
from
ac3ff3e
to
61adee1
Compare
2snEM6
force-pushed
the
bsm_unit_tests
branch
from
442e6df
to
53059c6
Compare
…idnt return any common block and continue trying
…omply with LIP updates
2snEM6
force-pushed
the
bsm_unit_tests
branch
from
53059c6
to
490ba93
Compare
2snEM6
force-pushed
the
bsm_unit_tests
branch
from
c99feff
to
ec29c3a
Compare
shuse2
requested changes
Nov 8, 2019
| async _handleBlockProcessingError(lastCommonBlock, peerId) { | ||
| // If the list of blocks has not been fully applied | ||
| this.logger.debug('Failed to apply obtained blocks from peer'); | ||
| const [tipBeforeApplying] = await this.storage.entities.TempBlock.get( |
There was a problem hiding this comment.
tipBeforeApplying can be undefined?
There was a problem hiding this comment.
True, in that case I guess we should restart mechanism only? and cleanup temp table just in case?
There was a problem hiding this comment.
I think if it fails to get then it's already cleaned up?
but in that case, i think we can just restart
There was a problem hiding this comment.
Maybe temp table is full and there is an error accessing database, hmmm
...ain/synchronizer/block_synchronization_mechanism/block_synchronization_mechanism.spec.bak.js
Outdated
Show resolved
Hide resolved
...ain/synchronizer/block_synchronization_mechanism/block_synchronization_mechanism.spec.bak.js
Outdated
Show resolved
Hide resolved
2snEM6
force-pushed
the
bsm_unit_tests
branch
from
ec29c3a
to
b79eed1
Compare
ManuGowda
suggested changes
Nov 8, 2019
There was a problem hiding this comment.
- Add missing test for isValidFor
- Implement the following missing behaviour
async _requestAndValidateLastBlock(peerId) {
this.logger.debug({ peerId }, 'Requesting tip of the chain from peer');
const { data } = await this.channel.invoke('network:requestFromPeer', {
procedure: 'getLastBlock',
peerId,
});
- Mock peer to return empty data and handle the scenario
- Update the title
should give up requesting the last common block after 3 tries, and then ban the peer and restart the mechanism
| tipBeforeApplyingInstance, // Previous tip of the chain | ||
| ); | ||
|
|
||
| const newTipHasPreference = forkStatus === FORK_STATUS_DIFFERENT_CHAIN; |
There was a problem hiding this comment.
any reason not to use directly in if condition?
There was a problem hiding this comment.
Yes, it is easier to read and to understand what the condition means, especially because the FORK_STATUS_DIFFERENT_CHAIN is not the best name to represent that behavior and I didn't want to change it in this PR
ManuGowda
suggested changes
Nov 8, 2019
...s/chain/synchronizer/block_synchronization_mechanism/block_synchronization_mechanism.spec.js
Outdated
Show resolved
Hide resolved
2snEM6
force-pushed
the
bsm_unit_tests
branch
from
f3f303f
to
cccbf02
Compare
2snEM6
force-pushed
the
bsm_unit_tests
branch
from
ebee456
to
92956f6
Compare
2snEM6
commented
Nov 8, 2019
framework/src/modules/chain/synchronizer/block_synchronization_mechanism.js
Show resolved
Hide resolved
shuse2
approved these changes
Nov 8, 2019
ManuGowda
approved these changes
Nov 11, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was the problem?
There were no unit tests for Block Synchronization Mechanism
How did I solve it?
By adding tests
How to manually test it?
Run
block_synchronization_mechanism.spec.jswith JestReview checklist