From d5df1212c2c9721d29258b684ca0dfe6a77e4e75 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 f00fa93ff..c21e1c3f0 100644
--- a/config/protocol.unit_testnet.yml
+++ b/config/protocol.unit_testnet.yml
@@ -49,9 +49,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 87f43b519..5641b9501 100644
--- a/pkg/rpc/server/server.go
+++ b/pkg/rpc/server/server.go
@@ -122,8 +122,9 @@ func New(chain core.Blockchainer, conf rpc.Config, coreServer *network.Server, l
 	}
 }
 
-// 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 c6ee3167e..7dc8fd263 100644
--- a/pkg/rpc/server/server_helper_test.go
+++ b/pkg/rpc/server/server_helper_test.go
@@ -18,7 +18,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
@@ -56,11 +56,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 8c2fb826a..9def78be5 100644
--- a/pkg/rpc/server/server_test.go
+++ b/pkg/rpc/server/server_test.go
@@ -896,9 +896,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 {