Skip to content

Commit 0d328d3

Browse files
authored
TestTxGossipingAndAggregation fix (#1411)
<!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. --> ## Overview This test was failing periodically due to `FinalizeBlock` being called too many times. The test verifies that at least N number of blocks were produced, which means that more than N number of blocks could have been produced which would lead to `FinalizeBlock` being called more than N times. This updates the test to check for `FinalizeBlock` being called at least N times as well to match the check for the blocks. The other alternate fix would be to enforce that only N blocks were produced. I'm not sure if that is the preferred way to write the test or not. Unless we can control the block production by manually triggering a block to be produced, checking for at least N blocks an calls will be more robust. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Improved the test verification process by introducing a new function to streamline the checking of specific method calls during integration tests. - **Chores** - Removed outdated commented code for cleaner and more maintainable codebase. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 5db5840 commit 0d328d3

1 file changed

Lines changed: 24 additions & 23 deletions

File tree

node/full_node_integration_test.go

Lines changed: 24 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -182,37 +182,38 @@ func TestTxGossipingAndAggregation(t *testing.T) {
182182
for _, node := range nodes {
183183
assert.NoError(node.Stop())
184184
}
185+
186+
// Now that the nodes have stopped, it should be safe to access the mock
187+
// calls outside of the mutex controlled methods.
188+
//
189+
// The reason we do this is because in the beginning of the test, we
190+
// check that we have produced at least N blocks, which means we could
191+
// have over produced. So when checking the calls, we also want to check
192+
// that we called FinalizeBlock at least N times.
185193
aggApp := apps[0]
186194
apps = apps[1:]
187195

188-
aggApp.AssertNumberOfCalls(t, "FinalizeBlock", numBlocksToWaitFor)
196+
checkCalls := func(app *mocks.Application, numBlocks int) error {
197+
calls := app.Calls
198+
numCalls := 0
199+
for _, call := range calls {
200+
if call.Method == "FinalizeBlock" {
201+
numCalls++
202+
}
203+
}
204+
if numBlocks > numCalls {
205+
return fmt.Errorf("expected at least %d calls to FinalizeBlock, got %d calls", numBlocks, numCalls)
206+
}
207+
return nil
208+
}
209+
210+
require.NoError(checkCalls(aggApp, numBlocksToWaitFor))
189211
aggApp.AssertExpectations(t)
190212

191213
for i, app := range apps {
192-
app.AssertNumberOfCalls(t, "FinalizeBlock", numBlocksToWaitFor)
214+
require.NoError(checkCalls(app, numBlocksToWaitFor))
193215
app.AssertExpectations(t)
194216

195-
// assert that we have most of the blocks from aggregator
196-
beginCnt := 0
197-
endCnt := 0
198-
commitCnt := 0
199-
// prepareProposal := 0
200-
// processProposal := 0
201-
for _, call := range app.Calls {
202-
switch call.Method {
203-
case "FinalizeBlock":
204-
beginCnt++
205-
case "CheckTx":
206-
endCnt++
207-
case "Commit":
208-
commitCnt++
209-
// case "PrepareProposal":
210-
// prepareProposal++
211-
// case "ProcessProposal":
212-
// processProposal++
213-
}
214-
}
215-
216217
// assert that all blocks known to node are same as produced by aggregator
217218
for h := uint64(1); h <= nodes[i].Store.Height(); h++ {
218219
aggBlock, err := nodes[0].Store.GetBlock(h)

0 commit comments

Comments
 (0)