Refactor the network tests (#314)

* Remove useless gRPC tests, add helper method for finding free ports

* Fix up tests

* Fix SimpleGossip

* Fix handshake test

* Fix failing tests, debug

* Trace -> Debug in logs

* Standardize the way tests are structured

* Fix linting errors

* Remove leftover loggers

* Remove useless prints from test

* Fix unreferenced vars after dummy print removal:

* Resolve pending linting errors

* Revert test change

* Remove the invalid linter checks

* Prune out useless event test

* Fix linting issue

* Add a nolint tag to funcs in skipped tests

* Add a nolint tag to funcs in skipped tests

* Add more safety checks to tests, add logger to failing test on GH actions

* Increase the default join timeout since it takes longer to connect

* Wait for the peers to disconnect successfully

* Remove unused waitForEvent

* Unify test checks

* Swap explicit error ignore for nolint

* Extract a common method for closing the test servers

* Remove test skips

* Remove logger

* Bump go version in GH workflow

* Improve the multijoin performance, increase the number of nodes for SimpleGossip

* Remove useless log in JoinAndWait

.github/workflows/main.yml

@@ -12,7 +12,7 @@ jobs:
       - name: Install Go
         uses: actions/setup-go@v2
         with:
-          go-version: 1.15.x
+          go-version: 1.16.x
 
       - name: Checkout code
         uses: actions/checkout@v2
@@ -23,25 +23,24 @@ jobs:
         uses: golangci/golangci-lint-action@v2
         with:
           version: v1.39
-          args: --disable staticcheck --disable errcheck
           skip-go-installation: true
   test:
     runs-on: ubuntu-latest
     steps:
       - name: Install Go
         uses: actions/setup-go@v2
         with:
-          go-version: 1.15.x
+          go-version: 1.17.x
 
       - name: Checkout code
         uses: actions/checkout@v2
         with:
           submodules: recursive
 
 
-      - name: Go build with race 
+      - name: Go build with race
         run: CGO_ENABLED=1 GOOS=linux go build -race -a -o artifacts/polygon-sdk .
-      
+
       - name: Add race artifacts directory to the path
         run: echo "$(pwd)/artifacts" >> $GITHUB_PATH
 
@@ -51,17 +50,15 @@ jobs:
       - name: Go build without race
         run: CGO_ENABLED=0 GOOS=linux go build -a -o artifacts/polygon-sdk .
 
-      - name: Replace artificats 
+      - name: Replace artificats
         run: echo "$(pwd)/artifacts" >> $GITHUB_PATH
 
-      - 
-        name: Extract branch name
+      - name: Extract branch name
         if: github.event_name != 'pull_request'
         shell: bash
         run: echo "##[set-output name=branch;]$(echo ${GITHUB_REF#refs/heads/})"
         id: extract_branch
-      - 
-        name: Prepare artifact for Deployer
+      - name: Prepare artifact for Deployer
         if: github.event_name != 'pull_request'
         uses: actions/upload-artifact@v2
         with:
@@ -72,13 +69,11 @@ jobs:
     runs-on: ubuntu-latest
     if: github.event_name != 'pull_request'
     steps:
-      - 
-        name: Extract branch name
+      - name: Extract branch name
         shell: bash
         run: echo "##[set-output name=branch;]$(echo ${GITHUB_REF#refs/heads/})"
         id: extract_branch
-      -
-        name: Notify Deployer
+      - name: Notify Deployer
         env:
           ARTIFACT_NAME: polygon-sdk_${{ github.sha }}_${{ steps.extract_branch.outputs.branch }}
           COMMITER: ${{ github.actor }}

blockchain/blockchain_test.go

@@ -510,8 +510,6 @@ func TestInsertHeaders(t *testing.T) {
 			}
 
 			if td, _ := b.GetChainTD(); cc.TD != td.Uint64() {
-				fmt.Println(td)
-				fmt.Println(cc.TD)
 				t.Fatal("bad")
 			}
 		})
@@ -557,10 +555,6 @@ func TestBlockchainWriteBody(t *testing.T) {
 	if err := b.writeBody(block); err != nil {
 		t.Fatal(err)
 	}
-
-	body, ok := b.readBody(block.Hash())
-	fmt.Println(body)
-	fmt.Println(ok)
 }
 
 func TestCalculateGasLimit(t *testing.T) {

consensus/ibft/snapshot_test.go

@@ -2,7 +2,6 @@ package ibft
 
 import (
 	"crypto/ecdsa"
-	"fmt"
 	"io/ioutil"
 	"os"
 	"strconv"
@@ -711,11 +710,6 @@ func TestSnapshot_ProcessHeaders(t *testing.T) {
 						})
 					}
 					if !resSnap.Equal(snap) {
-						fmt.Println("-- wrong result --")
-						fmt.Println(resSnap.Set)
-						fmt.Println(snap.Set)
-						fmt.Println(resSnap.Votes)
-						fmt.Println(snap.Votes)
 						t.Fatal("bad")
 					}
 				}

e2e/transaction_test.go

@@ -207,8 +207,6 @@ func TestEthTransfer(t *testing.T) {
 				Value:    testCase.amount,
 			}
 
-			fee := big.NewInt(0)
-
 			// Do the transfer
 			txnHash, err := rpcClient.Eth().SendTransaction(txnObject)
 			if testCase.shouldSucceed {
@@ -245,8 +243,9 @@ func TestEthTransfer(t *testing.T) {
 			assert.NoError(t, err)
 
 			expectedSenderBalance := previousSenderBalance
+			expectedReceiverBalance := previousReceiverBalance
 			if testCase.shouldSucceed {
-				fee = new(big.Int).Mul(
+				fee := new(big.Int).Mul(
 					big.NewInt(int64(receipt.GasUsed)),
 					big.NewInt(int64(txnObject.GasPrice)),
 				)
@@ -255,9 +254,7 @@ func TestEthTransfer(t *testing.T) {
 					previousSenderBalance,
 					new(big.Int).Add(testCase.amount, fee),
 				)
-			}
-			expectedReceiverBalance := previousReceiverBalance
-			if testCase.shouldSucceed {
+
 				expectedReceiverBalance = previousReceiverBalance.Add(
 					previousReceiverBalance,
 					testCase.amount,

helper/ipc/ipc_unix.go

@@ -1,3 +1,4 @@
+//go:build !windows
 // +build !windows
 
 package ipc
@@ -24,11 +25,15 @@ func Listen(path string) (net.Listener, error) {
 	if err := os.MkdirAll(filepath.Dir(path), 0751); err != nil {
 		return nil, err
 	}
-	os.Remove(path)
+	if removeErr := os.Remove(path); removeErr != nil {
+		return nil, removeErr
+	}
 	lis, err := net.Listen("unix", path)
 	if err != nil {
 		return nil, err
 	}
-	os.Chmod(path, 0600)
+	if chmodErr := os.Chmod(path, 0600); chmodErr != nil {
+		return nil, chmodErr
+	}
 	return lis, nil
 }

helper/keccak/keccak.go

@@ -22,6 +22,7 @@ type Keccak struct {
 // WriteRlp writes an RLP value
 func (k *Keccak) WriteRlp(dst []byte, v *fastrlp.Value) []byte {
 	k.buf = v.MarshalTo(k.buf[:0])
+	//nolint
 	k.Write(k.buf)
 	return k.Sum(dst)
 }
@@ -39,12 +40,14 @@ func (k *Keccak) Reset() {
 
 // Read hashes the content and returns the intermediate buffer.
 func (k *Keccak) Read() []byte {
+	//nolint
 	k.hash.Read(k.tmp)
 	return k.tmp
 }
 
 // Sum implements the hash interface
 func (k *Keccak) Sum(dst []byte) []byte {
+	//nolint
 	k.hash.Read(k.tmp)
 	dst = append(dst, k.tmp[:]...)
 	return dst

helper/keccak/pool.go

@@ -32,6 +32,7 @@ func (p *Pool) Put(k *Keccak) {
 // Keccak256 hashes a src with keccak-256
 func Keccak256(dst, src []byte) []byte {
 	h := DefaultKeccakPool.Get()
+	//nolint
 	h.Write(src)
 	dst = h.Sum(dst)
 	DefaultKeccakPool.Put(h)

helper/tests/testing.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"crypto/ecdsa"
 	"errors"
+	"net"
 	"testing"
 	"time"
 
@@ -98,3 +99,18 @@ func WaitForReceipt(ctx context.Context, client *jsonrpc.Eth, hash web3.Hash) (*
 	data := res.(result)
 	return data.receipt, data.err
 }
+
+// GetFreePort asks the kernel for a free open port that is ready to use
+func GetFreePort() (port int, err error) {
+	var addr *net.TCPAddr
+	if addr, err = net.ResolveTCPAddr("tcp", "localhost:0"); err == nil {
+		var l *net.TCPListener
+		if l, err = net.ListenTCP("tcp", addr); err == nil {
+			defer func(l *net.TCPListener) {
+				_ = l.Close()
+			}(l)
+			return l.Addr().(*net.TCPAddr).Port, nil
+		}
+	}
+	return
+}

jsonrpc/dispatcher.go

@@ -188,6 +188,7 @@ func (d *Dispatcher) HandleWs(reqBody []byte, conn wsConn) ([]byte, error) {
 	if req.Method == "eth_subscribe" {
 		filterID, err := d.handleSubscribe(req, conn)
 		if err != nil {
+			//nolint
 			NewRpcResponse(req.ID, "2.0", nil, err).Bytes()
 		}
 		resp, err := formatFilterResponse(req.ID, filterID)

jsonrpc/filter_manager.go

@@ -263,17 +263,23 @@ func (f *FilterManager) dispatchEvent(evnt *blockchain.Event) error {
 
 	// process old chain
 	for _, i := range evnt.OldChain {
-		processBlock(i, true)
+		if processErr := processBlock(i, true); processErr != nil {
+			f.logger.Error(fmt.Sprintf("Unable to process block, %v", processErr))
+		}
 	}
 	// process new chain
 	for _, i := range evnt.NewChain {
-		processBlock(i, false)
+		if processErr := processBlock(i, false); processErr != nil {
+			f.logger.Error(fmt.Sprintf("Unable to process block, %v", processErr))
+		}
 	}
 
 	// flush all the websocket values
-	for _, f := range f.filters {
-		if f.isWS() {
-			f.flush()
+	for _, filter := range f.filters {
+		if filter.isWS() {
+			if flushErr := filter.flush(); flushErr != nil {
+				f.logger.Error(fmt.Sprintf("Unable to process flush, %v", flushErr))
+			}
 		}
 	}
 	return nil

jsonrpc/filter_manager_test.go

@@ -69,7 +69,9 @@ func TestFilterLog(t *testing.T) {
 
 	time.Sleep(500 * time.Millisecond)
 
-	m.GetFilterChanges(id)
+	if _, fetchErr := m.GetFilterChanges(id); fetchErr != nil {
+		t.Fatalf("Unable to get filter changes, %v", fetchErr)
+	}
 }
 
 func TestFilterBlock(t *testing.T) {
@@ -110,7 +112,9 @@ func TestFilterBlock(t *testing.T) {
 	// we need to wait for the manager to process the data
 	time.Sleep(500 * time.Millisecond)
 
-	m.GetFilterChanges(id)
+	if _, fetchErr := m.GetFilterChanges(id); fetchErr != nil {
+		t.Fatalf("Unable to get filter changes, %v", fetchErr)
+	}
 
 	// emit one more event, it should not return the
 	// first three hashes
@@ -126,7 +130,9 @@ func TestFilterBlock(t *testing.T) {
 
 	time.Sleep(500 * time.Millisecond)
 
-	m.GetFilterChanges(id)
+	if _, fetchErr := m.GetFilterChanges(id); fetchErr != nil {
+		t.Fatalf("Unable to get filter changes, %v", fetchErr)
+	}
 }
 
 func TestFilterTimeout(t *testing.T) {

jsonrpc/jsonrpc.go

@@ -199,15 +199,18 @@ func (j *JSONRPC) handle(w http.ResponseWriter, req *http.Request) {
 		return
 	}
 	if req.Method == "GET" {
+		//nolint
 		w.Write([]byte("PolygonSDK JSON-RPC"))
 		return
 	}
 	if req.Method != "POST" {
+		//nolint
 		w.Write([]byte("method " + req.Method + " not allowed"))
 		return
 	}
 	data, err := ioutil.ReadAll(req.Body)
 	if err != nil {
+		//nolint
 		w.Write([]byte(err.Error()))
 		return
 	}
@@ -218,8 +221,10 @@ func (j *JSONRPC) handle(w http.ResponseWriter, req *http.Request) {
 	resp, err := j.dispatcher.Handle(data)
 
 	if err != nil {
+		//nolint
 		w.Write([]byte(err.Error()))
 	} else {
+		//nolint
 		w.Write(resp)
 	}
 	j.logger.Debug("handle", "response", string(resp))

jsonrpc/jsonrpc_test.go

@@ -1,7 +1,6 @@
 package jsonrpc
 
 import (
-	"fmt"
 	"net"
 	"testing"
 
@@ -14,9 +13,8 @@ func TestHTTPServer(t *testing.T) {
 		Store: store,
 		Addr:  &net.TCPAddr{IP: net.ParseIP("127.0.0.1"), Port: 8545},
 	}
-	srv, err := NewJSONRPC(hclog.NewNullLogger(), config)
+	_, err := NewJSONRPC(hclog.NewNullLogger(), config)
 	if err != nil {
 		t.Fatal(err)
 	}
-	fmt.Println(srv)
 }

network/discovery.go

@@ -51,7 +51,7 @@ func (ps *referencePeers) delete(id peer.ID) *referencePeer {
 	}
 	if idx != -1 {
 		deletePeer := (*ps)[idx]
-		(*ps) = append((*ps)[:idx], (*ps)[idx+1:]...)
+		*ps = append((*ps)[:idx], (*ps)[idx+1:]...)
 		return deletePeer
 	}
 	return nil
@@ -152,7 +152,7 @@ func (d *discovery) setupTable() {
 	}
 }
 
-func (d *discovery) call(peerID peer.ID) error {
+func (d *discovery) attemptToFindPeers(peerID peer.ID) error {
 	d.srv.logger.Debug("Querying a peer for near peers", "peer", peerID)
 	nodes, err := d.findPeersCall(peerID)
 	if err != nil {
@@ -237,8 +237,8 @@ func (d *discovery) handleDiscovery() {
 	// take a random peer and find peers
 	if d.peersCount() > 0 {
 		target := d.peers[rand.Intn(d.peersCount())]
-		if err := d.call(target.id); err != nil {
-			d.srv.logger.Error("failed to dial bootnode", "err", err)
+		if err := d.attemptToFindPeers(target.id); err != nil {
+			d.srv.logger.Error("failed to find peers", "err", err)
 		}
 	}
 }