Fix some P-chain UTs #2117
Fix some P-chain UTs #2117
Conversation
abi87
force-pushed
the
fix_platformVM_UTs
branch
2 times, most recently
from
cf2a0f2 to
36096b7
Compare
abi87
force-pushed
the
fix_platformVM_UTs
branch
from
36096b7 to
4101651
Compare
| } | ||
|
|
||
| // Test case where primary network validator is preferred to be rewarded | ||
| func TestRewardValidatorPreferred(t *testing.T) { |
There was a problem hiding this comment.
This test seems to verify the same behaviour in TestRewardValidatorAccept. Dropped
| @@ -721,9 +721,9 @@ func TestRewardValidatorAccept(t *testing.T) { | |||
| // Fast forward clock to time for genesis validators to leave | |||
| vm.clock.Set(defaultValidateEndTime) | |||
|
|
|||
| blk, err := vm.Builder.BuildBlock(context.Background()) // should advance time | |||
There was a problem hiding this comment.
PreBanff we used ApricotProposalBlocks to advance time.
Post Banff cleanup, we don't build ApricotProposalBlocks anymore (we deleted the code to do that).
Chain time is advanced by blocks timestamp, without proposal.
This test claims to issue an proposalBlock that advances time; it is instead issuing a proposal block to reward the first validator. Error is not caught because we didn't have a assertion on the block tx type.
Fix is to skip this "advance time with proposal block" section and move directly to next section where genesis validator is rewarded.
| require.NoError(oracleBlk.Accept(context.Background())) | ||
| // Assert block tries to reward a genesis validator | ||
| rewardTx := oracleBlk.(block.Block).Txs()[0].Unsigned | ||
| require.IsType(&txs.RewardValidatorTx{}, rewardTx) |
There was a problem hiding this comment.
let's introduce the assertion that would have make us spot the error in the first place
| txID := oracleBlk.(block.Block).Txs()[0].ID() | ||
| { | ||
| onAccept, ok := vm.manager.GetState(abort.ID()) | ||
| onAbort, ok := vm.manager.GetState(abort.ID()) |
There was a problem hiding this comment.
in this test we'll reward the validator by accepting the commit block. However in this code block we check that abort block is coherent.
abi87
force-pushed
the
fix_platformVM_UTs
branch
from
3657e8c to
4101651
Compare
| oracleBlk := blk.(smcon.OracleBlock) | ||
| options, err := oracleBlk.Options(context.Background()) | ||
| require.NoError(err) | ||
|
|
||
| commit := options[0].(*blockexecutor.Block) | ||
| abort := options[0].(*blockexecutor.Block) |
There was a problem hiding this comment.
preferences were wrongly flipped
StephenButtolph
added
vm
Why this should be merged
Some P-chain UTs are wrong. They still try to advance time via an ApricotProposalBlock, which we don't even build anymore. Moreover these tests are brittle as they rely on a specifc ordering of validators to be evicted and any change to their staking duration causes the test to fail with no reason.
Thanks @joshua-kim and @marun for the discussions that brought me to find this out
How this works
Drops attempt to move time forward with proposal block (not needed anymore post Banff)
Improves assertions to be more robust
Drop a test altogether cause it's the copy of another existing one
How this was tested
Testing test is tricky. I guess manual inspection.