sovereign chain block unit tests #6675
Conversation
| coreComponents, dataComponents, bootstrapComponents, statusComponents := createComponentHolderMocks() | ||
| arguments := createSovereignMockArguments(coreComponents, dataComponents, bootstrapComponents, statusComponents) | ||
| sovArgs := createArgsSovereignChainBlockProcessor(arguments) |
There was a problem hiding this comment.
- Please make a simple func that creates all args for sovereign, which doesn't require any input, like it was before.
- Compress these 3 lines into that function from abive and use it in these tests
| return arguments | ||
| } | ||
|
|
||
| func createSovereignMockArguments( |
There was a problem hiding this comment.
| func createSovereignMockArguments( | |
| func createShardBlockProcesorArgsForSovereign( |
|
|
||
| headerHandler, err := scbp.CreateNewHeader(round, 1) | ||
| require.Nil(t, err) | ||
| require.NotNil(t, headerHandler) |
There was a problem hiding this comment.
Can we actually check the header with a full &block.Sovereign... ?
|
|
||
| h, err := scbp.CreateNewHeader(round, nonce) | ||
| require.Nil(t, err) | ||
| require.IsType(t, &block.SovereignChainHeader{}, h) |
There was a problem hiding this comment.
Same remark as above, better to check directly with a header
| coreComponents, dataComponents, bootstrapComponents, statusComponents := createComponentHolderMocks() | ||
| arguments := createSovereignMockArguments(coreComponents, dataComponents, bootstrapComponents, statusComponents) | ||
| sovArgs := createArgsSovereignChainBlockProcessor(arguments) | ||
| scbp, err := blproc.NewSovereignChainBlockProcessor(sovArgs) |
There was a problem hiding this comment.
- Same remark regarding only 1 func to create everything
- You can move
scbp, err := blproc.NewSovereignChainBlockProcessor(sovArgs)outside these tests and reuse that instance
| require.Equal(t, process.ErrAccountStateDirty, err) | ||
| require.Equal(t, expectedBusyIdleSequencePerCall, busyIdleCalled) | ||
| }) | ||
| t.Run("create block started should error", func(t *testing.T) { |
There was a problem hiding this comment.
Please also mention why you make it fail (e.g. tx coordinator fails bcz...)
| scbp, err := blproc.NewSovereignChainBlockProcessor(sovArgs) | ||
| require.Nil(t, err) | ||
|
|
||
| doesHaveTime := func() bool { |
There was a problem hiding this comment.
This func can be moved outside these funcs.
| } | ||
|
|
||
| hdr, bodyHandler, err := scbp.CreateBlock(sovHeader, doesHaveTime) | ||
| require.False(t, check.IfNil(bodyHandler)) |
There was a problem hiding this comment.
Check directly with miniblocks and skip using these False.Nil.
Applies also for expected header. If you require it to be equal to your expected object, there is no need to also check it is not nil
| require.Equal(t, expectedErr, err) | ||
| require.Equal(t, expectedBusyIdleSequencePerCall, busyIdleCalled) | ||
| }) | ||
| t.Run("should work with sovereign header and epoch start rewriting the epoch value", func(t *testing.T) { |
There was a problem hiding this comment.
Please rename this func name, we do not rewrite, I think you mean we change epoch
| return true | ||
| } | ||
|
|
||
| hdr, bodyHandler, err := scbp.CreateBlock(expectedSovHeader, doesHaveTime) |
There was a problem hiding this comment.
Same remarks as above
Reasoning behind the pull request
Proposed changes
CreateNewHeaderfullCreateBlockpartialTesting procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?