Skip to content

Commit 9bdc8f6

Browse files
Manav-AggarwalManav-Aggarwal
authored
Use assert when stopping nodes in defer methods in tests instead of require (#1400)
<!-- 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 Closes: #1401 <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. --> ## Checklist <!-- Please complete the checklist to ensure that the PR is ready to be reviewed. IMPORTANT: PRs should be left in Draft until the below checklist is completed. --> - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Refactor** - Transitioned from using `require` to `assert` for error handling in test functions to improve test resilience and reporting. - **Tests** - Enhanced test reliability by adding deferred node stoppage handling across various integration tests. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent 9498447 commit 9bdc8f6

3 files changed

Lines changed: 61 additions & 60 deletions

File tree

‎node/full_client_test.go‎

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -325,7 +325,7 @@ func TestGetBlock(t *testing.T) {
325325
err := rpc.node.Start()
326326
require.NoError(err)
327327
defer func() {
328-
require.NoError(rpc.node.Stop())
328+
assert.NoError(rpc.node.Stop())
329329
}()
330330
block := types.GetRandomBlock(1, 10)
331331
err = rpc.node.Store.SaveBlock(block, &types.Commit{})
@@ -351,7 +351,7 @@ func TestGetCommit(t *testing.T) {
351351
err := rpc.node.Start()
352352
require.NoError(err)
353353
defer func() {
354-
require.NoError(rpc.node.Stop())
354+
assert.NoError(rpc.node.Stop())
355355
}()
356356
for _, b := range blocks {
357357
err = rpc.node.Store.SaveBlock(b, &types.Commit{})
@@ -445,7 +445,7 @@ func TestGetBlockByHash(t *testing.T) {
445445
err := rpc.node.Start()
446446
require.NoError(err)
447447
defer func() {
448-
require.NoError(rpc.node.Stop())
448+
assert.NoError(rpc.node.Stop())
449449
}()
450450
block := types.GetRandomBlock(1, 10)
451451
err = rpc.node.Store.SaveBlock(block, &types.Commit{})
@@ -514,7 +514,7 @@ func TestTx(t *testing.T) {
514514
err = rpc.node.Start()
515515
require.NoError(err)
516516
defer func() {
517-
require.NoError(rpc.node.Stop())
517+
assert.NoError(rpc.node.Stop())
518518
}()
519519
tx1 := cmtypes.Tx("tx1")
520520
res, err := rpc.BroadcastTxSync(ctx, tx1)
@@ -565,7 +565,7 @@ func TestUnconfirmedTxs(t *testing.T) {
565565
err := rpc.node.Start()
566566
require.NoError(err)
567567
defer func() {
568-
require.NoError(rpc.node.Stop())
568+
assert.NoError(rpc.node.Stop())
569569
}()
570570

571571
for _, tx := range c.txs {
@@ -604,7 +604,7 @@ func TestUnconfirmedTxsLimit(t *testing.T) {
604604
err := rpc.node.Start()
605605
require.NoError(err)
606606
defer func() {
607-
require.NoError(rpc.node.Stop())
607+
assert.NoError(rpc.node.Stop())
608608
}()
609609

610610
tx1 := cmtypes.Tx("tx1")
@@ -786,12 +786,12 @@ func TestMempool2Nodes(t *testing.T) {
786786
require.NoError(waitForFirstBlock(node1, Store))
787787

788788
defer func() {
789-
require.NoError(node1.Stop())
789+
assert.NoError(node1.Stop())
790790
}()
791791
err = node2.Start()
792792
require.NoError(err)
793793
defer func() {
794-
require.NoError(node2.Stop())
794+
assert.NoError(node2.Stop())
795795
}()
796796
require.NoError(waitForAtLeastNBlocks(node2, 1, Store))
797797
timeoutCtx, timeoutCancel := context.WithTimeout(context.Background(), 3*time.Second)
@@ -878,7 +878,7 @@ func TestStatus(t *testing.T) {
878878
err = node.Start()
879879
require.NoError(err)
880880
defer func() {
881-
require.NoError(node.Stop())
881+
assert.NoError(node.Stop())
882882
}()
883883

884884
resp, err := rpc.Status(context.Background())
@@ -970,7 +970,7 @@ func TestFutureGenesisTime(t *testing.T) {
970970
require.NoError(err)
971971

972972
defer func() {
973-
require.NoError(node.Stop())
973+
assert.NoError(node.Stop())
974974
}()
975975
wg.Wait()
976976

@@ -989,7 +989,7 @@ func TestHealth(t *testing.T) {
989989
err := rpc.node.Start()
990990
require.NoError(err)
991991
defer func() {
992-
require.NoError(rpc.node.Stop())
992+
assert.NoError(rpc.node.Stop())
993993
}()
994994

995995
resultHealth, err := rpc.Health(context.Background())
@@ -1009,8 +1009,7 @@ func TestNetInfo(t *testing.T) {
10091009
err := rpc.node.Start()
10101010
require.NoError(err)
10111011
defer func() {
1012-
err := rpc.node.Stop()
1013-
require.NoError(err)
1012+
assert.NoError(rpc.node.Stop())
10141013
}()
10151014

10161015
netInfo, err := rpc.NetInfo(context.Background())

‎node/full_node_integration_test.go‎

Lines changed: 44 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ func TestAggregatorMode(t *testing.T) {
6464
err = node.Start()
6565
assert.NoError(err)
6666
defer func() {
67-
require.NoError(node.Stop())
67+
assert.NoError(node.Stop())
6868
}()
6969
assert.True(node.IsRunning())
7070

@@ -89,15 +89,31 @@ func TestTxGossipingAndAggregation(t *testing.T) {
8989
require := require.New(t)
9090

9191
clientNodes := 4
92-
nodes, apps := createAndStartNodes(clientNodes, t)
92+
aggCtx, aggCancel := context.WithCancel(context.Background())
93+
defer aggCancel()
94+
ctx, cancel := context.WithCancel(context.Background())
95+
defer cancel()
96+
nodes, apps := createNodes(aggCtx, ctx, clientNodes+1, getBMConfig(), t)
97+
startNodes(nodes, apps, t)
98+
99+
// wait for nodes to start up and sync up till numBlocksToWaitFor
100+
numBlocksToWaitFor := 5
101+
for i := 1; i < len(nodes); i++ {
102+
require.NoError(waitForAtLeastNBlocks(nodes[i], numBlocksToWaitFor, Store))
103+
}
104+
105+
// Stop all the nodes before checking the calls to ABCI methods were done correctly
106+
for _, node := range nodes {
107+
assert.NoError(node.Stop())
108+
}
93109
aggApp := apps[0]
94110
apps = apps[1:]
95111

96-
aggApp.AssertNumberOfCalls(t, "FinalizeBlock", 2)
112+
aggApp.AssertNumberOfCalls(t, "FinalizeBlock", numBlocksToWaitFor)
97113
aggApp.AssertExpectations(t)
98114

99115
for i, app := range apps {
100-
app.AssertNumberOfCalls(t, "FinalizeBlock", 1)
116+
app.AssertNumberOfCalls(t, "FinalizeBlock", numBlocksToWaitFor)
101117
app.AssertExpectations(t)
102118

103119
// assert that we have most of the blocks from aggregator
@@ -120,13 +136,6 @@ func TestTxGossipingAndAggregation(t *testing.T) {
120136
// processProposal++
121137
}
122138
}
123-
aggregatorHeight := nodes[0].Store.Height()
124-
adjustedHeight := int(aggregatorHeight - 3) // 3 is completely arbitrary
125-
assert.GreaterOrEqual(beginCnt, adjustedHeight)
126-
assert.GreaterOrEqual(endCnt, adjustedHeight)
127-
assert.GreaterOrEqual(commitCnt, adjustedHeight)
128-
// assert.GreaterOrEqual(prepareProposal, adjustedHeight)
129-
// assert.GreaterOrEqual(processProposal, adjustedHeight)
130139

131140
// assert that all blocks known to node are same as produced by aggregator
132141
for h := uint64(1); h <= nodes[i].Store.Height(); h++ {
@@ -177,7 +186,7 @@ func TestLazyAggregator(t *testing.T) {
177186

178187
assert.NoError(node.Start())
179188
defer func() {
180-
require.NoError(node.Stop())
189+
assert.NoError(node.Stop())
181190
}()
182191
assert.True(node.IsRunning())
183192

@@ -203,6 +212,7 @@ func TestLazyAggregator(t *testing.T) {
203212
func TestFastDASync(t *testing.T) {
204213
// Test setup, create require and contexts for aggregator and client nodes
205214
require := require.New(t)
215+
assert := assert.New(t)
206216
aggCtx, aggCancel := context.WithCancel(context.Background())
207217
defer aggCancel()
208218
ctx, cancel := context.WithCancel(context.Background())
@@ -228,7 +238,7 @@ func TestFastDASync(t *testing.T) {
228238
// Start node 1
229239
require.NoError(node1.Start())
230240
defer func() {
231-
require.NoError(node1.Stop())
241+
assert.NoError(node1.Stop())
232242
}()
233243

234244
// Wait for node 1 to sync the first numberOfBlocksToSyncTill
@@ -237,7 +247,7 @@ func TestFastDASync(t *testing.T) {
237247
// Now that node 1 has already synced, start the second node
238248
require.NoError(node2.Start())
239249
defer func() {
240-
require.NoError(node2.Stop())
250+
assert.NoError(node2.Stop())
241251
}()
242252

243253
// Start and launch the timer in a go routine to ensure that the test
@@ -293,6 +303,7 @@ func TestFastDASync(t *testing.T) {
293303
// TestSingleAggregatorTwoFullNodesBlockSyncSpeed tests the scenario where the chain's block time is much faster than the DA's block time. In this case, the full nodes should be able to use block sync to sync blocks much faster than syncing from the DA layer, and the test should conclude within block time
294304
func TestSingleAggregatorTwoFullNodesBlockSyncSpeed(t *testing.T) {
295305
require := require.New(t)
306+
assert := assert.New(t)
296307
aggCtx, aggCancel := context.WithCancel(context.Background())
297308
defer aggCancel()
298309
ctx, cancel := context.WithCancel(context.Background())
@@ -326,16 +337,16 @@ func TestSingleAggregatorTwoFullNodesBlockSyncSpeed(t *testing.T) {
326337

327338
require.NoError(node1.Start())
328339
defer func() {
329-
require.NoError(node1.Stop())
340+
assert.NoError(node1.Stop())
330341
}()
331342
require.NoError(waitForFirstBlock(node1, Store))
332343
require.NoError(node2.Start())
333344
defer func() {
334-
require.NoError(node2.Stop())
345+
assert.NoError(node2.Stop())
335346
}()
336347
require.NoError(node3.Start())
337348
defer func() {
338-
require.NoError(node3.Stop())
349+
assert.NoError(node3.Stop())
339350
}()
340351

341352
require.NoError(waitForAtLeastNBlocks(node2, numberOfBlocksTSyncTill, Store))
@@ -372,6 +383,7 @@ func TestHeaderExchange(t *testing.T) {
372383

373384
func TestSubmitBlocksToDA(t *testing.T) {
374385
require := require.New(t)
386+
assert := assert.New(t)
375387

376388
clientNodes := 1
377389
ctx, cancel := context.WithCancel(context.Background())
@@ -389,7 +401,7 @@ func TestSubmitBlocksToDA(t *testing.T) {
389401
seq := nodes[0]
390402
require.NoError(seq.Start())
391403
defer func() {
392-
require.NoError(seq.Stop())
404+
assert.NoError(seq.Stop())
393405
}()
394406

395407
timer := time.NewTimer(5 * seq.nodeConfig.DABlockTime)
@@ -407,6 +419,7 @@ func TestSubmitBlocksToDA(t *testing.T) {
407419

408420
func testSingleAggregatorSingleFullNode(t *testing.T, source Source) {
409421
require := require.New(t)
422+
assert := assert.New(t)
410423

411424
aggCtx, aggCancel := context.WithCancel(context.Background())
412425
defer aggCancel()
@@ -420,14 +433,14 @@ func testSingleAggregatorSingleFullNode(t *testing.T, source Source) {
420433

421434
require.NoError(node1.Start())
422435
defer func() {
423-
require.NoError(node1.Stop())
436+
assert.NoError(node1.Stop())
424437
}()
425438

426439
require.NoError(waitForFirstBlock(node1, source))
427440
require.NoError(node2.Start())
428441

429442
defer func() {
430-
require.NoError(node2.Stop())
443+
assert.NoError(node2.Stop())
431444
}()
432445

433446
require.NoError(waitForAtLeastNBlocks(node2, 2, source))
@@ -436,6 +449,7 @@ func testSingleAggregatorSingleFullNode(t *testing.T, source Source) {
436449

437450
func testSingleAggregatorTwoFullNode(t *testing.T, source Source) {
438451
require := require.New(t)
452+
assert := assert.New(t)
439453

440454
aggCtx, aggCancel := context.WithCancel(context.Background())
441455
defer aggCancel()
@@ -450,16 +464,16 @@ func testSingleAggregatorTwoFullNode(t *testing.T, source Source) {
450464

451465
require.NoError(node1.Start())
452466
defer func() {
453-
require.NoError(node1.Stop())
467+
assert.NoError(node1.Stop())
454468
}()
455469
require.NoError(waitForFirstBlock(node1, source))
456470
require.NoError(node2.Start())
457471
defer func() {
458-
require.NoError(node2.Stop())
472+
assert.NoError(node2.Stop())
459473
}()
460474
require.NoError(node3.Start())
461475
defer func() {
462-
require.NoError(node3.Stop())
476+
assert.NoError(node3.Stop())
463477
}()
464478

465479
require.NoError(waitForAtLeastNBlocks(node2, 2, source))
@@ -470,6 +484,7 @@ func testSingleAggregatorTwoFullNode(t *testing.T, source Source) {
470484

471485
func testSingleAggregatorSingleFullNodeTrustedHash(t *testing.T, source Source) {
472486
require := require.New(t)
487+
assert := assert.New(t)
473488

474489
aggCtx, aggCancel := context.WithCancel(context.Background())
475490
defer aggCancel()
@@ -483,7 +498,7 @@ func testSingleAggregatorSingleFullNodeTrustedHash(t *testing.T, source Source)
483498

484499
require.NoError(node1.Start())
485500
defer func() {
486-
require.NoError(node1.Stop())
501+
assert.NoError(node1.Stop())
487502
}()
488503

489504
require.NoError(waitForFirstBlock(node1, source))
@@ -494,7 +509,7 @@ func testSingleAggregatorSingleFullNodeTrustedHash(t *testing.T, source Source)
494509
node2.nodeConfig.TrustedHash = trustedHash.Hash().String()
495510
require.NoError(node2.Start())
496511
defer func() {
497-
require.NoError(node2.Stop())
512+
assert.NoError(node2.Stop())
498513
}()
499514

500515
require.NoError(waitForAtLeastNBlocks(node1, 2, source))
@@ -503,6 +518,7 @@ func testSingleAggregatorSingleFullNodeTrustedHash(t *testing.T, source Source)
503518

504519
func testSingleAggregatorSingleFullNodeSingleLightNode(t *testing.T) {
505520
require := require.New(t)
521+
assert := assert.New(t)
506522

507523
aggCtx, aggCancel := context.WithCancel(context.Background())
508524
defer aggCancel()
@@ -527,38 +543,22 @@ func testSingleAggregatorSingleFullNodeSingleLightNode(t *testing.T) {
527543

528544
require.NoError(sequencer.Start())
529545
defer func() {
530-
require.NoError(sequencer.Stop())
546+
assert.NoError(sequencer.Stop())
531547
}()
532548
require.NoError(fullNode.Start())
533549
defer func() {
534-
require.NoError(fullNode.Stop())
550+
assert.NoError(fullNode.Stop())
535551
}()
536552
require.NoError(lightNode.Start())
537553
defer func() {
538-
require.NoError(lightNode.Stop())
554+
assert.NoError(lightNode.Stop())
539555
}()
540556

541557
require.NoError(waitForAtLeastNBlocks(sequencer.(*FullNode), 2, Header))
542558
require.NoError(verifyNodesSynced(sequencer, fullNode, Header))
543559
require.NoError(verifyNodesSynced(fullNode, lightNode, Header))
544560
}
545561

546-
// Creates a starts the given number of client nodes along with an aggregator node. Uses the given flag to decide whether to have the aggregator produce malicious blocks.
547-
func createAndStartNodes(clientNodes int, t *testing.T) ([]*FullNode, []*mocks.Application) {
548-
aggCtx, aggCancel := context.WithCancel(context.Background())
549-
defer aggCancel()
550-
ctx, cancel := context.WithCancel(context.Background())
551-
defer cancel()
552-
nodes, apps := createNodes(aggCtx, ctx, clientNodes+1, getBMConfig(), t)
553-
startNodes(nodes, apps, t)
554-
defer func() {
555-
for _, n := range nodes {
556-
assert.NoError(t, n.Stop())
557-
}
558-
}()
559-
return nodes, apps
560-
}
561-
562562
// Starts the given nodes using the given wait group to synchronize them
563563
// and wait for them to gossip transactions
564564
func startNodes(nodes []*FullNode, apps []*mocks.Application, t *testing.T) {

0 commit comments

Comments
 (0)