reworking unit tests

privval/signer_remote.go

@@ -10,7 +10,7 @@ import (
 // SignerRemote implements PrivValidator.
 // It uses a net.Conn to request signatures from an external process.
 type SignerRemote struct {
-	endpoint SignerValidatorEndpoint
+	endpoint *SignerValidatorEndpoint
 
 	// memoized
 	consensusPubKey crypto.PubKey
@@ -20,7 +20,7 @@ type SignerRemote struct {
 var _ types.PrivValidator = (*SignerRemote)(nil)
 
 // NewSignerRemote returns an instance of SignerRemote.
-func NewSignerRemote(endpoint SignerValidatorEndpoint) (*SignerRemote, error) {
+func NewSignerRemote(endpoint *SignerValidatorEndpoint) (*SignerRemote, error) {
 
 	// TODO: Fix this
 	//// retrieve and memoize the consensus public key once.

privval/signer_remote_test.go

@@ -2,29 +2,212 @@ package privval
 
 import (
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"github.com/tendermint/tendermint/libs/common"
 	"github.com/tendermint/tendermint/types"
 	"testing"
+	"time"
 )
 
-func TestSignerGetPubKey(t *testing.T) {
-	for _, tc := range getTestCases(t) {
+type signerTestCase struct {
+	chainID       string
+	mockPV        types.PrivValidator
+	signer        *SignerRemote
+	signerService *SignerServiceEndpoint // TODO: Replace once it is encapsulated
+}
+
+func getSignerTestCases(t *testing.T) []signerTestCase {
+	testCases := make([]signerTestCase, 0)
+
+	for _, dtc := range getDialerTestCases(t) {
+		chainID := common.RandStr(12)
+		mockPV := types.NewMockPV()
+
+		ve, se := getMockEndpoints(t, chainID, mockPV, dtc.addr, dtc.dialer)
+		sr, err := NewSignerRemote(ve)
+		assert.NoError(t, err)
+
+		tc := signerTestCase{
+			chainID:       chainID,
+			mockPV:        mockPV,
+			signer:        sr,
+			signerService: se,
+		}
+
+		testCases = append(testCases, tc)
+	}
+
+	return testCases
+}
+
+func TestSignerClose(t *testing.T) {
+	for _, tc := range getSignerTestCases(t) {
 		func() {
-			chainID := common.RandStr(12)
-			mockPV := types.NewMockPV()
-
-			validatorEndpoint, serviceEndpoint := getMockEndpoints(t, chainID, mockPV, tc.addr, tc.dialer)
-
-			sr, err := NewSignerRemote(*validatorEndpoint)
+			err := tc.signer.Close()
 			assert.NoError(t, err)
 
-			defer validatorEndpoint.Stop()
-			defer serviceEndpoint.Stop()
+			// FIXME: An error is logged but OnStop hides it
+			err = tc.signer.endpoint.Stop()
+			assert.NoError(t, err)
 
-			clientKey := sr.GetPubKey()
-			expectedPubKey := mockPV.GetPubKey()
+			//// FIXME: An error is logged but OnStop hides it
+			//err = tc.signerService.Stop()
+			//assert.NoError(t, err)
+		}()
+	}
+}
 
-			assert.Equal(t, expectedPubKey, clientKey)
+func TestSignerGetPubKey(t *testing.T) {
+	for _, tc := range getSignerTestCases(t) {
+		func() {
+			// FIXME: There are some errors logged that need to be checked
+			defer tc.signer.Close()
+			defer tc.signerService.OnStop()
+
+			pubKey := tc.signer.GetPubKey()
+			expectedPubKey := tc.mockPV.GetPubKey()
+
+			assert.Equal(t, expectedPubKey, pubKey)
+
+			addr := tc.signer.GetPubKey().Address()
+			expectedAddr := tc.mockPV.GetPubKey().Address()
+
+			assert.Equal(t, expectedAddr, addr)
+		}()
+	}
+}
+
+func TestSignerProposal(t *testing.T) {
+	for _, tc := range getSignerTestCases(t) {
+		func() {
+			ts := time.Now()
+			want := &types.Proposal{Timestamp: ts}
+			have := &types.Proposal{Timestamp: ts}
+
+			defer tc.signer.Close()
+			defer tc.signerService.OnStop()
+
+			require.NoError(t, tc.mockPV.SignProposal(tc.chainID, want))
+			require.NoError(t, tc.signer.SignProposal(tc.chainID, have))
+
+			assert.Equal(t, want.Signature, have.Signature)
+		}()
+	}
+}
+
+func TestSignerVote(t *testing.T) {
+	for _, tc := range getSignerTestCases(t) {
+		func() {
+			ts := time.Now()
+			want := &types.Vote{Timestamp: ts, Type: types.PrecommitType}
+			have := &types.Vote{Timestamp: ts, Type: types.PrecommitType}
+
+			defer tc.signer.Close()
+			defer tc.signerService.OnStop()
+
+			require.NoError(t, tc.mockPV.SignVote(tc.chainID, want))
+			require.NoError(t, tc.signer.SignVote(tc.chainID, have))
+
+			assert.Equal(t, want.Signature, have.Signature)
+		}()
+	}
+}
+
+func TestSignerVoteResetDeadline(t *testing.T) {
+	for _, tc := range getSignerTestCases(t) {
+		func() {
+			ts := time.Now()
+			want := &types.Vote{Timestamp: ts, Type: types.PrecommitType}
+			have := &types.Vote{Timestamp: ts, Type: types.PrecommitType}
+
+			defer tc.signer.Close()
+			defer tc.signerService.OnStop()
+
+			time.Sleep(testTimeoutReadWrite2o3)
+
+			require.NoError(t, tc.mockPV.SignVote(tc.chainID, want))
+			require.NoError(t, tc.signer.SignVote(tc.chainID, have))
+			assert.Equal(t, want.Signature, have.Signature)
+
+			// FIXME: Lots of ping errors that do not bubble up
+			// TODO: Clarify what is actually being tested
+
+			// This would exceed the deadline if it was not extended by the previous message
+			time.Sleep(testTimeoutReadWrite2o3)
+
+			require.NoError(t, tc.mockPV.SignVote(tc.chainID, want))
+			require.NoError(t, tc.signer.SignVote(tc.chainID, have))
+			assert.Equal(t, want.Signature, have.Signature)
+		}()
+	}
+}
+
+func TestSignerVoteKeepAlive(t *testing.T) {
+	for _, tc := range getSignerTestCases(t) {
+		func() {
+			ts := time.Now()
+			want := &types.Vote{Timestamp: ts, Type: types.PrecommitType}
+			have := &types.Vote{Timestamp: ts, Type: types.PrecommitType}
+
+			defer tc.signer.Close()
+			defer tc.signerService.OnStop()
+
+			time.Sleep(testTimeoutReadWrite * 2)
+
+			require.NoError(t, tc.mockPV.SignVote(tc.chainID, want))
+			require.NoError(t, tc.signer.SignVote(tc.chainID, have))
+			assert.Equal(t, want.Signature, have.Signature)
+
+			// FIXME: Lots of ping errors that do not bubble up
+			// TODO: Clarify what is actually being tested and how it differs from TestSignerVoteResetDeadline
+		}()
+	}
+}
+
+func TestSignerSignProposalErrors(t *testing.T) {
+	for _, tc := range getSignerTestCases(t) {
+		func() {
+			ts := time.Now()
+			proposal := &types.Proposal{Timestamp: ts}
+
+			tc.signerService.privVal = types.NewErroringMockPV()
+			tc.mockPV = types.NewErroringMockPV()
+
+			defer tc.signer.Close()
+			defer tc.signerService.OnStop()
+
+			err := tc.signer.SignProposal(tc.chainID, proposal)
+			require.Equal(t, err.(*RemoteSignerError).Description, types.ErroringMockPVErr.Error())
+
+			err = tc.mockPV.SignProposal(tc.chainID, proposal)
+			require.Error(t, err)
+
+			err = tc.signer.SignProposal(tc.chainID, proposal)
+			require.Error(t, err)
+		}()
+	}
+}
+
+func TestSignerSignVoteErrors(t *testing.T) {
+	for _, tc := range getSignerTestCases(t) {
+		func() {
+			ts := time.Now()
+			vote := &types.Vote{Timestamp: ts, Type: types.PrecommitType}
+
+			tc.signerService.privVal = types.NewErroringMockPV()
+			tc.mockPV = types.NewErroringMockPV()
+
+			defer tc.signer.Close()
+			defer tc.signerService.OnStop()
+
+			err := tc.signer.SignVote(tc.chainID, vote)
+			require.Equal(t, err.(*RemoteSignerError).Description, types.ErroringMockPVErr.Error())
+
+			err = tc.mockPV.SignVote(tc.chainID, vote)
+			require.Error(t, err)
+
+			err = tc.signer.SignVote(tc.chainID, vote)
+			require.Error(t, err)
 		}()
 	}
 }

privval/signer_validator_endpoint.go

@@ -195,6 +195,9 @@ func (ve *SignerValidatorEndpoint) readMessage() (msg RemoteSignerMsg, err error
 
 func (ve *SignerValidatorEndpoint) writeMessage(msg RemoteSignerMsg) (err error) {
 	// TODO: Check connection status
+	if ve.conn == nil {
+		return fmt.Errorf("endpoint is not connected")
+	}
 
 	_, err = cdc.MarshalBinaryLengthPrefixedWriter(ve.conn, msg)
 	if _, ok := err.(timeoutError); ok {

privval/signer_validator_endpoint_test.go

@@ -1,7 +1,6 @@
 package privval
 
 import (
-	"fmt"
 	"net"
 	"testing"
 	"time"
@@ -24,7 +23,7 @@ var (
 	testTimeoutHeartbeat3o2 = 6 * time.Millisecond // 3/2 of the other one
 )
 
-type socketTestCase struct {
+type dialerTestCase struct {
 	addr   string
 	dialer SocketDialer
 }
@@ -83,146 +82,6 @@ func TestSignerRemoteRetryTCPOnly(t *testing.T) {
 	}
 }
 
-//func TestSocketPVAddress(t *testing.T) {
-//	for _, tc := range getTestCases(t) {
-//		// Execute the test within a closure to ensure the deferred statements
-//		// are called between each for loop iteration, for isolated test cases.
-//		func() {
-//			var (
-//				chainID                            = common.RandStr(12)
-//				validatorEndpoint, serviceEndpoint = getMockEndpoints(t, chainID, types.NewMockPV(), tc.addr, tc.dialer)
-//			)
-//			defer validatorEndpoint.Stop()
-//			defer serviceEndpoint.Stop()
-//
-//			serviceAddr := serviceEndpoint.privVal.GetPubKey().Address()
-//			validatorAddr := validatorEndpoint.GetPubKey().Address()
-//
-//			assert.Equal(t, serviceAddr, validatorAddr)
-//		}()
-//	}
-//}
-//
-//
-//func TestSocketPVProposal(t *testing.T) {
-//	for _, tc := range getTestCases(t) {
-//		func() {
-//			var (
-//				chainID                            = common.RandStr(12)
-//				validatorEndpoint, serviceEndpoint = getMockEndpoints(
-//					t,
-//					chainID,
-//					types.NewMockPV(),
-//					tc.addr,
-//					tc.dialer)
-//
-//				ts             = time.Now()
-//				privProposal   = &types.Proposal{Timestamp: ts}
-//				clientProposal = &types.Proposal{Timestamp: ts}
-//			)
-//			defer validatorEndpoint.Stop()
-//			defer serviceEndpoint.Stop()
-//
-//			require.NoError(t, serviceEndpoint.privVal.SignProposal(chainID, privProposal))
-//			require.NoError(t, validatorEndpoint.SignProposal(chainID, clientProposal))
-//
-//			assert.Equal(t, privProposal.Signature, clientProposal.Signature)
-//		}()
-//	}
-//}
-//
-//func TestSocketPVVote(t *testing.T) {
-//	for _, tc := range getTestCases(t) {
-//		func() {
-//			var (
-//				chainID                            = common.RandStr(12)
-//				validatorEndpoint, serviceEndpoint = getMockEndpoints(
-//					t,
-//					chainID,
-//					types.NewMockPV(),
-//					tc.addr,
-//					tc.dialer)
-//
-//				ts    = time.Now()
-//				vType = types.PrecommitType
-//				want  = &types.Vote{Timestamp: ts, Type: vType}
-//				have  = &types.Vote{Timestamp: ts, Type: vType}
-//			)
-//			defer validatorEndpoint.Stop()
-//			defer serviceEndpoint.Stop()
-//
-//			require.NoError(t, serviceEndpoint.privVal.SignVote(chainID, want))
-//			require.NoError(t, validatorEndpoint.SignVote(chainID, have))
-//			assert.Equal(t, want.Signature, have.Signature)
-//		}()
-//	}
-//}
-//
-//func TestSocketPVVoteResetDeadline(t *testing.T) {
-//	for _, tc := range getTestCases(t) {
-//		func() {
-//			var (
-//				chainID                            = common.RandStr(12)
-//				validatorEndpoint, serviceEndpoint = getMockEndpoints(
-//					t,
-//					chainID,
-//					types.NewMockPV(),
-//					tc.addr,
-//					tc.dialer)
-//
-//				ts    = time.Now()
-//				vType = types.PrecommitType
-//				want  = &types.Vote{Timestamp: ts, Type: vType}
-//				have  = &types.Vote{Timestamp: ts, Type: vType}
-//			)
-//			defer validatorEndpoint.Stop()
-//			defer serviceEndpoint.Stop()
-//
-//			time.Sleep(testTimeoutReadWrite2o3)
-//
-//			require.NoError(t, serviceEndpoint.privVal.SignVote(chainID, want))
-//			require.NoError(t, validatorEndpoint.SignVote(chainID, have))
-//			assert.Equal(t, want.Signature, have.Signature)
-//
-//			// This would exceed the deadline if it was not extended by the previous message
-//			time.Sleep(testTimeoutReadWrite2o3)
-//
-//			require.NoError(t, serviceEndpoint.privVal.SignVote(chainID, want))
-//			require.NoError(t, validatorEndpoint.SignVote(chainID, have))
-//			assert.Equal(t, want.Signature, have.Signature)
-//		}()
-//	}
-//}
-//
-//func TestSocketPVVoteKeepalive(t *testing.T) {
-//	for _, tc := range getTestCases(t) {
-//		func() {
-//			var (
-//				chainID                            = common.RandStr(12)
-//				validatorEndpoint, serviceEndpoint = getMockEndpoints(
-//					t,
-//					chainID,
-//					types.NewMockPV(),
-//					tc.addr,
-//					tc.dialer)
-//
-//				ts    = time.Now()
-//				vType = types.PrecommitType
-//				want  = &types.Vote{Timestamp: ts, Type: vType}
-//				have  = &types.Vote{Timestamp: ts, Type: vType}
-//			)
-//			defer validatorEndpoint.Stop()
-//			defer serviceEndpoint.Stop()
-//
-//			time.Sleep(testTimeoutReadWrite * 2)
-//
-//			require.NoError(t, serviceEndpoint.privVal.SignVote(chainID, want))
-//			require.NoError(t, validatorEndpoint.SignVote(chainID, have))
-//			assert.Equal(t, want.Signature, have.Signature)
-//		}()
-//	}
-//}
-//
 //func TestSocketPVDeadline(t *testing.T) {
 //	for _, tc := range getTestCases(t) {
 //		func() {
@@ -254,67 +113,7 @@ func TestSignerRemoteRetryTCPOnly(t *testing.T) {
 //		}()
 //	}
 //}
-//
-//func TestRemoteSignVoteErrors(t *testing.T) {
-//	for _, tc := range getTestCases(t) {
-//		func() {
-//			var (
-//				chainID                            = common.RandStr(12)
-//				validatorEndpoint, serviceEndpoint = getMockEndpoints(
-//					t,
-//					chainID,
-//					types.NewErroringMockPV(),
-//					tc.addr,
-//					tc.dialer)
-//
-//				ts    = time.Now()
-//				vType = types.PrecommitType
-//				vote  = &types.Vote{Timestamp: ts, Type: vType}
-//			)
-//			defer validatorEndpoint.Stop()
-//			defer serviceEndpoint.Stop()
-//
-//			err := validatorEndpoint.SignVote("", vote)
-//			require.Equal(t, err.(*RemoteSignerError).Description, types.ErroringMockPVErr.Error())
-//
-//			err = serviceEndpoint.privVal.SignVote(chainID, vote)
-//			require.Error(t, err)
-//			err = validatorEndpoint.SignVote(chainID, vote)
-//			require.Error(t, err)
-//		}()
-//	}
-//}
-//
-//func TestRemoteSignProposalErrors(t *testing.T) {
-//	for _, tc := range getTestCases(t) {
-//		func() {
-//			var (
-//				chainID                            = common.RandStr(12)
-//				validatorEndpoint, serviceEndpoint = getMockEndpoints(
-//					t,
-//					chainID,
-//					types.NewErroringMockPV(),
-//					tc.addr,
-//					tc.dialer)
-//
-//				ts       = time.Now()
-//				proposal = &types.Proposal{Timestamp: ts}
-//			)
-//			defer validatorEndpoint.Stop()
-//			defer serviceEndpoint.Stop()
-//
-//			err := validatorEndpoint.SignProposal("", proposal)
-//			require.Equal(t, err.(*RemoteSignerError).Description, types.ErroringMockPVErr.Error())
-//
-//			err = serviceEndpoint.privVal.SignProposal(chainID, proposal)
-//			require.Error(t, err)
-//
-//			err = validatorEndpoint.SignProposal(chainID, proposal)
-//			require.Error(t, err)
-//		}()
-//	}
-//}
-//
+
 //func TestErrUnexpectedResponse(t *testing.T) {
 //	for _, tc := range getTestCases(t) {
 //		func() {
@@ -431,24 +230,6 @@ func TestSignerRemoteRetryTCPOnly(t *testing.T) {
 
 ///////////////////////////////////
 
-func getTestCases(t *testing.T) []socketTestCase {
-	tcpAddr := fmt.Sprintf("tcp://%s", testFreeTCPAddr(t))
-	unixFilePath, err := testUnixAddr()
-	require.NoError(t, err)
-	unixAddr := fmt.Sprintf("unix://%s", unixFilePath)
-
-	return []socketTestCase{
-		{
-			addr:   tcpAddr,
-			dialer: DialTCPFn(tcpAddr, testTimeoutReadWrite, ed25519.GenPrivKey()),
-		},
-		{
-			addr:   unixAddr,
-			dialer: DialUnixFn(unixFilePath),
-		},
-	}
-}
-
 func newSignerValidatorEndpoint(logger log.Logger, addr string, timeoutReadWrite time.Duration) *SignerValidatorEndpoint {
 	proto, address := common.ProtocolAndAddress(addr)
 

privval/socket_dialers_test.go

@@ -1,6 +1,8 @@
 package privval
 
 import (
+	"fmt"
+	"github.com/stretchr/testify/require"
 	"testing"
 	"time"
 
@@ -9,6 +11,24 @@ import (
 	cmn "github.com/tendermint/tendermint/libs/common"
 )
 
+func getDialerTestCases(t *testing.T) []dialerTestCase {
+	tcpAddr := fmt.Sprintf("tcp://%s", testFreeTCPAddr(t))
+	unixFilePath, err := testUnixAddr()
+	require.NoError(t, err)
+	unixAddr := fmt.Sprintf("unix://%s", unixFilePath)
+
+	return []dialerTestCase{
+		{
+			addr:   tcpAddr,
+			dialer: DialTCPFn(tcpAddr, testTimeoutReadWrite, ed25519.GenPrivKey()),
+		},
+		{
+			addr:   unixAddr,
+			dialer: DialUnixFn(unixFilePath),
+		},
+	}
+}
+
 func TestIsConnTimeoutForFundamentalTimeouts(t *testing.T) {
 	// Generate a networking timeout
 	dialer := DialTCPFn(testFreeTCPAddr(t), time.Millisecond, ed25519.GenPrivKey())