ProposerVM Extend windows 1 - UTs Cleanup #2412
Conversation
| innerBlk := snowman.NewMockBlock(ctrl) | ||
| innerBlk.EXPECT().ID().Return(blkID).AnyTimes() | ||
| innerBlk.EXPECT().Height().Return(pChainHeight - 1).AnyTimes() | ||
|
|
There was a problem hiding this comment.
nit: maybe it's just me but having smaller blocks of code vs a single list of stuff makes code slighty less annoying. Happy to revert diff if this is not shared opinion
| default: | ||
| return nil, errUnknownBlock | ||
| } | ||
| } | ||
|
|
||
| proVM.Set(proVM.Time().Add(proposer.MaxBuildDelay)) |
There was a problem hiding this comment.
this time set is not really needed for the first block built on genesis.
|
|
||
| // .. create child block ... | ||
| childCoreBlk := &snowman.TestBlock{ | ||
| ParentV: prntCoreBlk.ID(), | ||
| BytesV: []byte{2}, | ||
| TimestampV: prntCoreBlk.Timestamp(), |
There was a problem hiding this comment.
this the core block of a would be post fork block, its timestamp is not relevant to proposerVM. It's height and parent may be. So we better set up the right attributes
| } | ||
| childSlb, err := block.Build( |
There was a problem hiding this comment.
previous test version built a signed and then an unsigned block. There is no reason for that. I picked building two unsigned blocks just to simplify the test (that is really about parent checks)
There was a problem hiding this comment.
Ideally we'd like the same check carried out for signed blocks. We may add these UTs in a subsequent PR
| require.NoError(err) | ||
| childProBlk.SignedBlock = childSlb | ||
| require.NoError(err) | ||
| { |
There was a problem hiding this comment.
I scoped different tests cases this way, as we do in other parts of the codebase. Makes this diff more annoying but I think it improves overall readability. Also the more tightly scoped variables, the easier the maintanability I think
| childProBlk.SignedBlock = childSlb | ||
|
|
||
| err = childProBlk.Verify(context.Background()) | ||
| require.ErrorIs(err, errPChainHeightNotMonotonic) |
There was a problem hiding this comment.
the test was not setup properly and here we were asserting the wrong error. Fixed
| } | ||
|
|
||
| // child P-Chain height must not precede parent P-Chain height | ||
| childSlb, err := block.Build( |
There was a problem hiding this comment.
once again a signed block and some unsigned blocks after. Not sure why. Moved to all unsigned blocks. To be revisited to extend UTs coverage over signed block.
| valState.GetMinimumHeightF = func(context.Context) (uint64, error) { | ||
| return pChainHeight / 50, nil | ||
| } |
There was a problem hiding this comment.
this missing was the reason of the wrong error assertion below
| BytesV: []byte{1}, | ||
| ParentV: coreGenBlk.ID(), | ||
| HeightV: coreGenBlk.Height() + 1, | ||
| TimestampV: coreGenBlk.Timestamp(), |
There was a problem hiding this comment.
these are inner blocks of would be post fork blocks. Their timestamp is irrelevant for proposerVM. Dropping it
There was a problem hiding this comment.
Thanks for taking this...caused a lot of confusion during the MaxVerify/BuildDelay change (at least for me).
| @@ -210,6 +210,8 @@ func initTestProposerVM( | |||
| require.NoError(proVM.SetState(context.Background(), snow.NormalOp)) | |||
| require.NoError(proVM.SetPreference(context.Background(), coreGenBlk.IDV)) | |||
|
|
|||
| proVM.Set(coreGenBlk.Timestamp()) | |||
There was a problem hiding this comment.
proposerVM clock should be not earlier that its genesis timestamp. We set them to be equal for simplicity here
| default: | ||
| return nil, database.ErrNotFound | ||
| } | ||
| } | ||
|
|
||
| proVM.Set(proVM.Time().Add(proposer.MaxBuildDelay)) |
There was a problem hiding this comment.
These are pre fork blocks, setting proposerVM clock is pointless. Dropped
| } | ||
| coreVM.ParseBlockF = func(context.Context, []byte) (snowman.Block, error) { | ||
| return nil, errMarshallingFailed | ||
| } | ||
| slb, err := statelessblock.Build( | ||
| proVM.preferred, | ||
| innerBlk.Timestamp(), |
There was a problem hiding this comment.
An example of the intertwining of innerBlk.Timestamp and proposerVM timestamp that we have in tests but not in production code. Dropped
abi87
changed the title
abi87
changed the title
StephenButtolph
added
testing
Why this should be merged
A few proposerVM unit tests are brittle.
This PR fixes these issues.
How this works
Just fixed the UTs
How this was tested
Manual inspection of each UT