From 4acd117b5e28cf39709e87cf802dbee8e928d8b9 Mon Sep 17 00:00:00 2001
From: William Banfield <4561443+williambanfield@users.noreply.github.com>
Date: Tue, 9 Nov 2021 15:48:01 -0500
Subject: [PATCH] evidence: remove source of non-determinism from test (#7266)

The evidence test produces a set of mock evidence in the evidence pool of the 'Primary' node. The test then fills the evidence pools of secondaries with half of this mock evidence. Finally, the test waits until the secondary has an evidence pool as full as the primary.

The assertions that are removed here were checking that the primary and secondaries' evidence channels were empty. However, nothing in the test actually ensures that the channels are empty. The test only waits for the secondaries to have received the complete set of evidence, and the secondaries already received half of the evidence at the beginning. It's more than possible that the secondaries can receive the complete set of evidence and not finish reading the duplicate evidence off the channels.
---
 internal/evidence/reactor_test.go | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/internal/evidence/reactor_test.go b/internal/evidence/reactor_test.go
index cf8f840ea..d1ae55803 100644
--- a/internal/evidence/reactor_test.go
+++ b/internal/evidence/reactor_test.go
@@ -394,11 +394,8 @@ func TestReactorBroadcastEvidence_Pending(t *testing.T) {
 	require.Len(t, rts.pools, 2)
 	assert.EqualValues(t, numEvidence, rts.pools[primary.NodeID].Size(),
 		"primary node should have all the evidence")
-	if assert.EqualValues(t, numEvidence, rts.pools[secondary.NodeID].Size(),
-		"secondary nodes should have caught up") {
-
-		rts.assertEvidenceChannelsEmpty(t)
-	}
+	assert.EqualValues(t, numEvidence, rts.pools[secondary.NodeID].Size(),
+		"secondary nodes should have caught up")
 }
 
 func TestReactorBroadcastEvidence_Committed(t *testing.T) {
@@ -438,11 +435,6 @@ func TestReactorBroadcastEvidence_Committed(t *testing.T) {
 	// start the network and ensure it's configured
 	rts.start(t)
 
-	// without the following sleep the test consistently fails;
-	// likely because the sleep forces a context switch that lets
-	// the router process other operations.
-	time.Sleep(2 * time.Millisecond)
-
 	// The secondary reactor should have received all the evidence ignoring the
 	// already committed evidence.
 	rts.waitForEvidence(t, evList[numEvidence/2:], secondary.NodeID)
@@ -450,11 +442,8 @@ func TestReactorBroadcastEvidence_Committed(t *testing.T) {
 	require.Len(t, rts.pools, 2)
 	assert.EqualValues(t, numEvidence, rts.pools[primary.NodeID].Size(),
 		"primary node should have all the evidence")
-	if assert.EqualValues(t, numEvidence/2, rts.pools[secondary.NodeID].Size(),
-		"secondary nodes should have caught up") {
-
-		rts.assertEvidenceChannelsEmpty(t)
-	}
+	assert.EqualValues(t, numEvidence/2, rts.pools[secondary.NodeID].Size(),
+		"secondary nodes should have caught up")
 }
 
 func TestReactorBroadcastEvidence_FullyConnected(t *testing.T) {