Unit tests and bugfix requestEpochStartInfo #3762
Conversation
Codecov Report
@@ Coverage Diff @@
## development #3762 +/- ##
===============================================
+ Coverage 74.29% 74.37% +0.07%
===============================================
Files 603 603
Lines 79463 79466 +3
===============================================
+ Hits 59039 59102 +63
+ Misses 15864 15823 -41
+ Partials 4560 4541 -19
Continue to review full report at Codecov.
|
mariusmihaic
changed the title
mariusmihaic
changed the title
|
|
||
| coreComponents, dataComponents, bootstrapComponents, statusComponents := createComponentHolderMocks() | ||
|
|
||
| tests := []struct { |
There was a problem hiding this comment.
... or could have used the t.Run pattern as those kind of tests are easier to debug
Can be left as it is
There was a problem hiding this comment.
Yes, that's right. I don't think it's worth a refactor for these nil pointer checks, will leave it as it is then
| genesisHdr := arguments.DataComponents.Blockchain().GetGenesisHeader() | ||
| if check.IfNil(genesisHdr) { |
There was a problem hiding this comment.
this is a good check, but the project is swarming with blockchain Get operations without nil checks. The blockchain was thought to be a struct that shouldn't contain nil values.
There was a problem hiding this comment.
This might've been the reason why this nil check was missed. Discovered it while setting up mocks for the tests
| @@ -349,6 +352,7 @@ func (sp *shardProcessor) requestEpochStartInfo(header data.ShardHeaderHandler, | |||
|
|
|||
| epochStartMetaHdr, err := headersPool.GetHeaderByHash(header.GetEpochStartMetaHash()) | |||
| if err != nil { | |||
| go sp.requestHandler.RequestMetaHeader(header.GetEpochStartMetaHash()) | |||
There was a problem hiding this comment.
this will trigger ~10 requests / second. This is a little too much as it will flood the network with messages. Suggestion to add a counter here and only when that counter reaches a predefined value, say 10, it will launch the request go routine
There was a problem hiding this comment.
Will remain as it is. As discussed, L361 does not have a throttle. The final throttler (output antiflooder) will kick in, if required.
There was a problem hiding this comment.
I think they are worthless as the requestHandler has a protection mechanism which not allows more than one request with the same hash in less than one second from the previous one, through testIfRequestIsNeeded method in it's first line
There was a problem hiding this comment.
Great info, will keep it as it is then
| expectedErr: process.ErrNilDataComponentsHolder, | ||
| }, | ||
| { | ||
| args: func() blproc.ArgShardProcessor { |
There was a problem hiding this comment.
almost the same number of lines of code as t.Run :D
Can be left as it is
There was a problem hiding this comment.
Will leave it like this then
Tests
requestEpochStartInfofromshardBlock.goshardBlock.goare done inbaseProcesss.go(CheckProcessorNilParameters), I've moved all nil pointer check tests + created new ones fromshardBlock.go->baseProcesss.goBugfixes
NewShardProcessorwhere arguments.DataComponents.Blockchain().GetGenesisHeader() could have been a nil pointer.requestEpochStartInfo: Before entering in the foor loop to request headers (L343), meta header is requested oncesp.requestHandler.RequestMetaHeader(header.GetEpochStartMetaHash())(L340).While inside the loop, if header is not found in pool (
headersPool.GetHeaderByHash, from various reasons we did not have a response to our query) => error is returned, but we do not make any more requests for it . One line code onL354would fix it: