Cleanup consensus engine tests #2953
Conversation
StephenButtolph
added
consensus
| func TestEngineAbandonResponse(t *testing.T) { | ||
| require := require.New(t) | ||
|
|
||
| vdr, _, sender, vm, te, gBlk := setupDefaultConfig(t) | ||
|
|
||
| sender.Default(false) | ||
|
|
||
| blk := &snowman.TestBlock{ | ||
| TestDecidable: choices.TestDecidable{ | ||
| IDV: ids.GenerateTestID(), | ||
| StatusV: choices.Unknown, | ||
| }, | ||
| ParentV: gBlk.ID(), | ||
| HeightV: 1, | ||
| BytesV: []byte{1}, | ||
| } | ||
|
|
||
| vm.GetBlockF = func(_ context.Context, blkID ids.ID) (snowman.Block, error) { | ||
| switch { | ||
| case blkID == gBlk.ID(): | ||
| return gBlk, nil | ||
| case blkID == blk.ID(): | ||
| return nil, errUnknownBlock | ||
| } | ||
| require.FailNow(errUnknownBlock.Error()) | ||
| return nil, errUnknownBlock | ||
| } | ||
|
|
||
| require.NoError(te.issue( | ||
| context.Background(), | ||
| te.Ctx.NodeID, | ||
| blk, | ||
| false, | ||
| te.metrics.issued.WithLabelValues(unknownSource), | ||
| )) | ||
| require.NoError(te.QueryFailed(context.Background(), vdr, 1)) | ||
|
|
||
| require.Empty(te.blocked) | ||
| } |
There was a problem hiding this comment.
This test made 100% no sense. te.blocked is always empty during this test... So really it's just testing that issue works... Which we already test in other tests.
| require.True(vmShutdownCalled) | ||
| } | ||
|
|
||
| func TestEngineAdd(t *testing.T) { | ||
| func TestEngineDropsAttemptToIssueBlockAfterFailedRequest(t *testing.T) { |
There was a problem hiding this comment.
Why is this change part of this PR?
There was a problem hiding this comment.
This just changed the name of the test, I felt it was more useful of a name then TestEngineAdd
| IDV: Genesis, | ||
| StatusV: choices.Accepted, | ||
| }} | ||
|
|
||
| vm.LastAcceptedF = func(context.Context) (ids.ID, error) { |
There was a problem hiding this comment.
(No action required) Given that this is a test VM, why isn't this kind of initialization an internal detail or performed in a constructor?
There was a problem hiding this comment.
Yeah this should be improved. I mainly wanted to unblock the new test in #2956 for this PR, so not trying to do a total re-write here.
| require.Equal(vdr, inVdr) | ||
| require.Equal(blk.Parent(), blkID) | ||
| var request *common.Request | ||
| sender.SendGetF = func(_ context.Context, nodeID ids.NodeID, requestID uint32, blkID ids.ID) { |
There was a problem hiding this comment.
(No action required) Would it be possible for the code under test to be refactored to enable verification of the desired behavior without having to resort to the current testvm/mock-like-thing approach?
There was a problem hiding this comment.
There are some things that can definitely be factored out... See some of the changes in #2952.
| // Handling a pull query for [parent] should result in immediately | ||
| // responding with chits for [Genesis] along with a request for [parent]. | ||
| require.NoError(engine.PullQuery(context.Background(), peerID, 15, parent.ID(), 1)) | ||
| require.True(sendChitsCalled) |
There was a problem hiding this comment.
(No action required) Is it possible to verify the desired behavior without resorting to checking for specific calls? i.e. specify inputs that can only result in the desired outputs if the expected call path were taken
There was a problem hiding this comment.
We could probably re-write the tests to simulate multiple participants. But it feels like that would be better suited to an E2E test
| GenesisID = ids.GenerateTestID() | ||
| GenesisBytes = utils.RandomBytes(32) |
There was a problem hiding this comment.
Do we even need these two vars since they can be accessed from just calling Genesis.BytesV/IDV?
| ) | ||
|
|
||
| func setup(t *testing.T, engCfg Config) (ids.NodeID, validators.Manager, *common.SenderTest, *block.TestVM, *Transitive, snowman.Block) { | ||
| func BuildChain(root *snowman.TestBlock, length int) []*snowman.TestBlock { |
There was a problem hiding this comment.
I prefer us just generating the entire chain here instead of the caller defining a genesis block and calling BuildChain to get more blocks, but I could also buy the argument that maybe this is cleaner for tests that only have one block...
Why this should be merged
The consensus engine tests are in a really bad spot. This PR cleans them up a bit. By defining the genesis globally and adding a helper function to generate a chain in a helper function.
These tests are realistically going to be completely re-written... But I want to update these tests incrementally so we can understand what's changing.
How this works
How this was tested