From 612500ed96a4f058b24ffd85da170a496b468bf1 Mon Sep 17 00:00:00 2001
From: Roman Khimov <roman@nspcc.ru>
Date: Sat, 9 May 2020 23:59:21 +0300
Subject: [PATCH] rpc/server: start and shutdown Server in tests

It will be important for proper subscription testing and it doesn't hurt even
though technically we've got two http servers listening after this change (one
is a regular Server's http.Server and one is httptest's Server). Reusing
rpc.Server would be nice, but it requires some changes to Start sequence to
start Listener with net.Listen and then communicate back its resulting
Addr. It's not very convenient especially given that no other code needs it,
so doing these changes just for a bit cleaner testing seems like and
overkill.

Update config appropriately. Update Start comment along the way.
---
 config/protocol.unit_testnet.yml     | 3 ++-
 pkg/rpc/server/server.go             | 5 +++--
 pkg/rpc/server/server_helper_test.go | 6 ++++--
 pkg/rpc/server/server_test.go        | 3 ++-
 4 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/config/protocol.unit_testnet.yml b/config/protocol.unit_testnet.yml
index 2c809c523..37424cc3d 100644
--- a/config/protocol.unit_testnet.yml
+++ b/config/protocol.unit_testnet.yml
@@ -47,9 +47,10 @@ ApplicationConfiguration:
   AttemptConnPeers: 5
   MinPeers: 1
   RPC:
+    Address: 127.0.0.1
     Enabled: true
     EnableCORSWorkaround: false
-    Port: 20332
+    Port: 0 # let the system choose port dynamically
   Prometheus:
     Enabled: false #since it's not useful for unit tests.
     Port: 2112
diff --git a/pkg/rpc/server/server.go b/pkg/rpc/server/server.go
index 09c8dd11d..2d7a9e226 100644
--- a/pkg/rpc/server/server.go
+++ b/pkg/rpc/server/server.go
@@ -123,8 +123,9 @@ func New(chain blockchainer.Blockchainer, conf rpc.Config, coreServer *network.S
 	}
 }
 
-// Start creates a new JSON-RPC server
-// listening on the configured port.
+// Start creates a new JSON-RPC server listening on the configured port. It's
+// supposed to be run as a separate goroutine (like http.Server's Serve) and it
+// returns its errors via given errChan.
 func (s *Server) Start(errChan chan error) {
 	if !s.config.Enabled {
 		s.log.Info("RPC server is not enabled")
diff --git a/pkg/rpc/server/server_helper_test.go b/pkg/rpc/server/server_helper_test.go
index b65897ab3..efec61f10 100644
--- a/pkg/rpc/server/server_helper_test.go
+++ b/pkg/rpc/server/server_helper_test.go
@@ -17,7 +17,7 @@ import (
 	"go.uber.org/zap/zaptest"
 )
 
-func initServerWithInMemoryChain(t *testing.T) (*core.Blockchain, *httptest.Server) {
+func initServerWithInMemoryChain(t *testing.T) (*core.Blockchain, *Server, *httptest.Server) {
 	var nBlocks uint32
 
 	net := config.ModeUnitTestNet
@@ -55,11 +55,13 @@ func initServerWithInMemoryChain(t *testing.T) (*core.Blockchain, *httptest.Serv
 	server, err := network.NewServer(serverConfig, chain, logger)
 	require.NoError(t, err)
 	rpcServer := New(chain, cfg.ApplicationConfiguration.RPC, server, logger)
+	errCh := make(chan error, 2)
+	go rpcServer.Start(errCh)
 
 	handler := http.HandlerFunc(rpcServer.handleHTTPRequest)
 	srv := httptest.NewServer(handler)
 
-	return chain, srv
+	return chain, &rpcServer, srv
 }
 
 type FeerStub struct{}
diff --git a/pkg/rpc/server/server_test.go b/pkg/rpc/server/server_test.go
index 1d28ab001..b3f64686f 100644
--- a/pkg/rpc/server/server_test.go
+++ b/pkg/rpc/server/server_test.go
@@ -875,9 +875,10 @@ func TestRPC(t *testing.T) {
 // calls. Some tests change the chain state, thus we reinitialize the chain from
 // scratch here.
 func testRPCProtocol(t *testing.T, doRPCCall func(string, string, *testing.T) []byte) {
-	chain, httpSrv := initServerWithInMemoryChain(t)
+	chain, rpcSrv, httpSrv := initServerWithInMemoryChain(t)
 
 	defer chain.Close()
+	defer rpcSrv.Shutdown()
 
 	e := &executor{chain: chain, httpSrv: httpSrv}
 	for method, cases := range rpcTestCases {