[p2p] Peer behaviour test tweaks (#3662)

* Peer behaviour test tweaks:

    Address the remaining test issues mentioned in ##3552 notably:
    + switch to p2p_test package
    + Use `Error` instead of `Errorf` when not using formatting
    + Add expected/got errors to
    `TestMockPeerBehaviourReporterConcurrency` test

* Peer behaviour equal behaviours test

    + slices of PeerBehaviours should be compared as histograms to
    ensure they have the same set of PeerBehaviours at the same
    freequncy.

* TestEqualPeerBehaviours:

    + Add tests for the equivalence between sets of PeerBehaviours

p2p/peer_behaviour_test.go

@@ -1,58 +1,121 @@
-package p2p
+package p2p_test
 
 import (
-	"net"
 	"sync"
 	"testing"
+
+	"github.com/tendermint/tendermint/p2p"
 )
 
 // TestMockPeerBehaviour tests the MockPeerBehaviour' ability to store reported
 // peer behaviour in memory indexed by the peerID
 func TestMockPeerBehaviourReporter(t *testing.T) {
-	peer := newMockPeer(net.IP{127, 0, 0, 1})
-	pr := NewMockPeerBehaviourReporter()
+	var peerID p2p.ID = "MockPeer"
+	pr := p2p.NewMockPeerBehaviourReporter()
 
-	behaviours := pr.GetBehaviours(peer.ID())
+	behaviours := pr.GetBehaviours(peerID)
 	if len(behaviours) != 0 {
-		t.Errorf("Expected to have no behaviours reported")
+		t.Error("Expected to have no behaviours reported")
 	}
 
-	pr.Report(peer.ID(), PeerBehaviourBadMessage)
-	behaviours = pr.GetBehaviours(peer.ID())
+	pr.Report(peerID, p2p.PeerBehaviourBadMessage)
+	behaviours = pr.GetBehaviours(peerID)
 	if len(behaviours) != 1 {
-		t.Errorf("Expected the peer have one reported behaviour")
+		t.Error("Expected the peer have one reported behaviour")
 	}
 
-	if behaviours[0] != PeerBehaviourBadMessage {
-		t.Errorf("Expected PeerBehaviourBadMessage to have been reported")
+	if behaviours[0] != p2p.PeerBehaviourBadMessage {
+		t.Error("Expected PeerBehaviourBadMessage to have been reported")
 	}
 }
 
 type scriptedBehaviours struct {
-	PeerID     ID
-	Behaviours []PeerBehaviour
+	PeerID     p2p.ID
+	Behaviours []p2p.PeerBehaviour
 }
 
 type scriptItem struct {
-	PeerID    ID
-	Behaviour PeerBehaviour
+	PeerID    p2p.ID
+	Behaviour p2p.PeerBehaviour
 }
 
-func equalBehaviours(a []PeerBehaviour, b []PeerBehaviour) bool {
-	if len(a) != len(b) {
+// equalBehaviours returns true if a and b contain the same PeerBehaviours with
+// the same freequency and otherwise false.
+func equalBehaviours(a []p2p.PeerBehaviour, b []p2p.PeerBehaviour) bool {
+	aHistogram := map[p2p.PeerBehaviour]int{}
+	bHistogram := map[p2p.PeerBehaviour]int{}
+
+	for _, behaviour := range a {
+		aHistogram[behaviour] += 1
+	}
+
+	for _, behaviour := range b {
+		bHistogram[behaviour] += 1
+	}
+
+	if len(aHistogram) != len(bHistogram) {
 		return false
 	}
-	same := make([]PeerBehaviour, len(a))
 
-	for i, aBehaviour := range a {
-		for _, bBehaviour := range b {
-			if aBehaviour == bBehaviour {
-				same[i] = aBehaviour
-			}
+	for _, behaviour := range a {
+		if aHistogram[behaviour] != bHistogram[behaviour] {
+			return false
 		}
 	}
 
-	return len(same) == len(a)
+	for _, behaviour := range b {
+		if bHistogram[behaviour] != aHistogram[behaviour] {
+			return false
+		}
+	}
+
+	return true
+}
+
+// TestEqualPeerBehaviours tests that equalBehaviours can tell that two slices
+// of peer behaviours can be compared for the behaviours they contain and the
+// freequencies that those behaviours occur.
+func TestEqualPeerBehaviours(t *testing.T) {
+	equals := []struct {
+		left  []p2p.PeerBehaviour
+		right []p2p.PeerBehaviour
+	}{
+		// Empty sets
+		{[]p2p.PeerBehaviour{}, []p2p.PeerBehaviour{}},
+		// Single behaviours
+		{[]p2p.PeerBehaviour{p2p.PeerBehaviourVote}, []p2p.PeerBehaviour{p2p.PeerBehaviourVote}},
+		// Equal Frequencies
+		{[]p2p.PeerBehaviour{p2p.PeerBehaviourVote, p2p.PeerBehaviourVote},
+			[]p2p.PeerBehaviour{p2p.PeerBehaviourVote, p2p.PeerBehaviourVote}},
+		// Equal frequencies different orders
+		{[]p2p.PeerBehaviour{p2p.PeerBehaviourVote, p2p.PeerBehaviourBlockPart},
+			[]p2p.PeerBehaviour{p2p.PeerBehaviourBlockPart, p2p.PeerBehaviourVote}},
+	}
+
+	for _, test := range equals {
+		if !equalBehaviours(test.left, test.right) {
+			t.Errorf("Expected %#v and %#v to be equal", test.left, test.right)
+		}
+	}
+
+	unequals := []struct {
+		left  []p2p.PeerBehaviour
+		right []p2p.PeerBehaviour
+	}{
+		// Comparing empty sets to non empty sets
+		{[]p2p.PeerBehaviour{}, []p2p.PeerBehaviour{p2p.PeerBehaviourVote}},
+		// Different behaviours
+		{[]p2p.PeerBehaviour{p2p.PeerBehaviourVote}, []p2p.PeerBehaviour{p2p.PeerBehaviourBlockPart}},
+		// Same behaviour with different frequencies
+		{[]p2p.PeerBehaviour{p2p.PeerBehaviourVote},
+			[]p2p.PeerBehaviour{p2p.PeerBehaviourVote, p2p.PeerBehaviourVote}},
+	}
+
+	for _, test := range unequals {
+		if equalBehaviours(test.left, test.right) {
+			t.Errorf("Expected %#v and %#v to be unequal", test.left, test.right)
+		}
+	}
 }
 
 // TestPeerBehaviourConcurrency constructs a scenario in which
@@ -61,15 +124,15 @@ func equalBehaviours(a []PeerBehaviour, b []PeerBehaviour) bool {
 // be used within a Reactor Receive method tests to ensure thread safety.
 func TestMockPeerBehaviourReporterConcurrency(t *testing.T) {
 	behaviourScript := []scriptedBehaviours{
-		{"1", []PeerBehaviour{PeerBehaviourVote}},
-		{"2", []PeerBehaviour{PeerBehaviourVote, PeerBehaviourVote, PeerBehaviourVote, PeerBehaviourVote}},
-		{"3", []PeerBehaviour{PeerBehaviourBlockPart, PeerBehaviourVote, PeerBehaviourBlockPart, PeerBehaviourVote}},
-		{"4", []PeerBehaviour{PeerBehaviourVote, PeerBehaviourVote, PeerBehaviourVote, PeerBehaviourVote}},
-		{"5", []PeerBehaviour{PeerBehaviourBlockPart, PeerBehaviourVote, PeerBehaviourBlockPart, PeerBehaviourVote}},
+		{"1", []p2p.PeerBehaviour{p2p.PeerBehaviourVote}},
+		{"2", []p2p.PeerBehaviour{p2p.PeerBehaviourVote, p2p.PeerBehaviourVote, p2p.PeerBehaviourVote, p2p.PeerBehaviourVote}},
+		{"3", []p2p.PeerBehaviour{p2p.PeerBehaviourBlockPart, p2p.PeerBehaviourVote, p2p.PeerBehaviourBlockPart, p2p.PeerBehaviourVote}},
+		{"4", []p2p.PeerBehaviour{p2p.PeerBehaviourVote, p2p.PeerBehaviourVote, p2p.PeerBehaviourVote, p2p.PeerBehaviourVote}},
+		{"5", []p2p.PeerBehaviour{p2p.PeerBehaviourBlockPart, p2p.PeerBehaviourVote, p2p.PeerBehaviourBlockPart, p2p.PeerBehaviourVote}},
 	}
 
 	var receiveWg sync.WaitGroup
-	pr := NewMockPeerBehaviourReporter()
+	pr := p2p.NewMockPeerBehaviourReporter()
 	scriptItems := make(chan scriptItem)
 	done := make(chan int)
 	numConsumers := 3
@@ -108,8 +171,10 @@ func TestMockPeerBehaviourReporterConcurrency(t *testing.T) {
 	receiveWg.Wait()
 
 	for _, items := range behaviourScript {
-		if !equalBehaviours(pr.GetBehaviours(items.PeerID), items.Behaviours) {
-			t.Errorf("Expected peer %s to have behaved \n", items.PeerID)
+		reported := pr.GetBehaviours(items.PeerID)
+		if !equalBehaviours(reported, items.Behaviours) {
+			t.Errorf("Expected peer %s to have behaved \nExpected: %#v \nGot %#v \n",
+				items.PeerID, items.Behaviours, reported)
 		}
 	}
 }