chore(tests): refactor TestSystemRPC

- Add empty skipped tests for missing cases
- Split each subtest individually
- Keep on retrying until main context is canceled
- Fix `networkState` test case
- Assert more fields
- Fix #2161 and #807

tests/rpc/rpc_01-system_test.go

@@ -5,146 +5,171 @@ package rpc
 
 import (
 	"context"
-	"reflect"
 	"testing"
 	"time"
 
 	"github.com/ChainSafe/gossamer/dot/rpc/modules"
+	"github.com/ChainSafe/gossamer/lib/common"
 	libutils "github.com/ChainSafe/gossamer/lib/utils"
 	"github.com/ChainSafe/gossamer/tests/utils"
 	"github.com/ChainSafe/gossamer/tests/utils/config"
 	"github.com/ChainSafe/gossamer/tests/utils/node"
+	"github.com/ChainSafe/gossamer/tests/utils/retry"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+const peerIDRegex = `^[a-zA-Z0-9]{52}$`
+
 func TestSystemRPC(t *testing.T) {
 	if utils.MODE != rpcSuite {
 		t.Log("Going to skip RPC suite tests")
 		return
 	}
 
-	testCases := []*testCase{
-		{ //TODO
-			description: "test system_name",
-			method:      "system_name",
-			skip:        true,
-		},
-		{ //TODO
-			description: "test system_version",
-			method:      "system_version",
-			skip:        true,
-		},
-		{ //TODO
-			description: "test system_chain",
-			method:      "system_chain",
-			skip:        true,
-		},
-		{ //TODO
-			description: "test system_properties",
-			method:      "system_properties",
-			skip:        true,
-		},
-		{
-			description: "test system_health",
-			method:      "system_health",
-			expected: modules.SystemHealthResponse{
-				Peers:           2,
-				IsSyncing:       true,
-				ShouldHavePeers: true,
-			},
-			params: "{}",
-		},
-		{
-			description: "test system_peers",
-			method:      "system_peers",
-			expected:    modules.SystemPeersResponse{},
-			params:      "{}",
-		},
-		{
-			description: "test system_network_state",
-			method:      "system_networkState",
-			expected: modules.SystemNetworkStateResponse{
-				NetworkState: modules.NetworkStateString{
-					PeerID: "",
-				},
-			},
-			params: "{}",
-		},
-		{ //TODO
-			description: "test system_addReservedPeer",
-			method:      "system_addReservedPeer",
-			skip:        true,
-		},
-		{ //TODO
-			description: "test system_removeReservedPeer",
-			method:      "system_removeReservedPeer",
-			skip:        true,
-		},
-		{ //TODO
-			description: "test system_nodeRoles",
-			method:      "system_nodeRoles",
-			skip:        true,
-		},
-		{ //TODO
-			description: "test system_accountNextIndex",
-			method:      "system_accountNextIndex",
-			skip:        true,
-		},
-	}
+	const numberOfNodes = 3
 
 	genesisPath := libutils.GetGssmrGenesisRawPathTest(t)
 	config := config.CreateDefault(t)
-	nodes := node.MakeNodes(t, 3, node.SetGenesis(genesisPath), node.SetConfig(config))
+	nodes := node.MakeNodes(t, numberOfNodes, node.SetGenesis(genesisPath), node.SetConfig(config))
 
 	ctx, cancel := context.WithCancel(context.Background())
 	nodes.InitAndStartTest(ctx, t, cancel)
 
-	time.Sleep(time.Second) // give server a second to start
+	t.Run("system_health", func(t *testing.T) {
+		t.Parallel()
 
-	for _, test := range testCases {
-		t.Run(test.description, func(t *testing.T) {
-			if test.skip {
-				t.SkipNow()
-			}
+		const method = "system_health"
+		const params = "{}"
+
+		expected := modules.SystemHealthResponse{
+			Peers:           numberOfNodes - 1,
+			IsSyncing:       true,
+			ShouldHavePeers: true,
+		}
 
+		var response modules.SystemHealthResponse
+		err := retry.UntilOK(ctx, time.Second, func() (ok bool) {
 			getResponseCtx, getResponseCancel := context.WithTimeout(ctx, time.Second)
-			defer getResponseCancel()
-			target := reflect.New(reflect.TypeOf(test.expected)).Interface()
-			err := getResponse(getResponseCtx, test.method, test.params, target)
-			require.NoError(t, err)
+			err := getResponse(getResponseCtx, method, params, &response)
+			getResponseCancel()
 
-			switch v := target.(type) {
-			case *modules.SystemHealthResponse:
-				t.Log("Will assert SystemHealthResponse", "target", target)
+			require.NoError(t, err)
+			return response.Peers == expected.Peers
+		})
+		require.NoError(t, err)
 
-				require.Equal(t, test.expected.(modules.SystemHealthResponse).IsSyncing, v.IsSyncing)
-				require.Equal(t, test.expected.(modules.SystemHealthResponse).ShouldHavePeers, v.ShouldHavePeers)
-				require.GreaterOrEqual(t, v.Peers, test.expected.(modules.SystemHealthResponse).Peers)
+		assert.Equal(t, expected, response)
+	})
 
-			case *modules.SystemNetworkStateResponse:
-				t.Log("Will assert SystemNetworkStateResponse", "target", target)
+	t.Run("system_peers", func(t *testing.T) {
+		t.Parallel()
 
-				require.NotNil(t, v.NetworkState)
-				require.NotNil(t, v.NetworkState.PeerID)
+		const method = "system_peers"
+		const params = "{}"
 
-			case *modules.SystemPeersResponse:
-				t.Log("Will assert SystemPeersResponse", "target", target)
+		var response modules.SystemPeersResponse
+		retry.UntilOK(ctx, time.Second, func() (ok bool) {
+			getResponseCtx, getResponseCancel := context.WithTimeout(ctx, time.Second)
+			err := getResponse(getResponseCtx, method, params, &response)
+			getResponseCancel()
 
-				require.NotNil(t, v)
+			require.NoError(t, err)
 
-				//TODO: #807
-				//this assertion requires more time on init to be enabled
-				//require.GreaterOrEqual(t, len(v.Peers), 2)
+			if len(response) != numberOfNodes-1 {
+				return false
+			}
 
-				for _, vv := range *v {
-					require.NotNil(t, vv.PeerID)
-					require.NotNil(t, vv.Roles)
-					require.NotNil(t, vv.BestHash)
-					require.NotNil(t, vv.BestNumber)
+			for _, peer := range response {
+				if peer.PeerID == "" {
+					return false
 				}
-
 			}
-
+			return true
 		})
-	}
+
+		// Check randomly generated peer IDs and clear this field
+		for i := range response {
+			assert.Regexp(t, peerIDRegex, response[i].PeerID)
+			response[i].PeerID = ""
+		}
+
+		expectedBestHash := common.Hash{
+			0xb5, 0xd8, 0xb5, 0xdd, 0xc7, 0xbb, 0x57, 0x64,
+			0x00, 0x14, 0x28, 0x38, 0x5f, 0x23, 0xf2, 0x93,
+			0x52, 0x11, 0xc7, 0x0c, 0xd8, 0xce, 0x6d, 0x0e,
+			0x10, 0x96, 0xc7, 0xa7, 0xc3, 0x2b, 0x92, 0x6c}
+		expectedResponse := modules.SystemPeersResponse{
+			{Roles: 4, BestHash: expectedBestHash, BestNumber: 0},
+			{Roles: 4, BestHash: expectedBestHash, BestNumber: 0},
+		}
+		assert.Equal(t, expectedResponse, response)
+	})
+
+	t.Run("system_networkState", func(t *testing.T) {
+		t.Parallel()
+
+		const method = "system_networkState"
+		const params = "{}"
+
+		getResponseCtx, getResponseCancel := context.WithTimeout(ctx, time.Second)
+		defer getResponseCancel()
+		var response modules.SystemNetworkStateResponse
+		err := getResponse(getResponseCtx, method, params, &response)
+
+		require.NoError(t, err)
+
+		assert.Regexp(t, peerIDRegex, response.NetworkState.PeerID)
+		response.NetworkState.PeerID = ""
+
+		assert.NotEmpty(t, response.NetworkState.Multiaddrs)
+		for _, addr := range response.NetworkState.Multiaddrs {
+			assert.Regexp(t, "^/ip[4|6]/.+/tcp/[0-9]{1,5}/p2p/[a-zA-Z0-9]{52}$", addr)
+		}
+		response.NetworkState.Multiaddrs = nil
+
+		// Ensure we don't need to assert other fields
+		expectedResponse := modules.SystemNetworkStateResponse{}
+		assert.Equal(t, expectedResponse, response)
+	})
+
+	t.Run("system_name", func(t *testing.T) {
+		t.Parallel()
+		t.Skip("test not implemented")
+	})
+
+	t.Run("system_version", func(t *testing.T) {
+		t.Parallel()
+		t.Skip("test not implemented")
+	})
+
+	t.Run("system_chain", func(t *testing.T) {
+		t.Parallel()
+		t.Skip("test not implemented")
+	})
+
+	t.Run("system_properties", func(t *testing.T) {
+		t.Parallel()
+		t.Skip("test not implemented")
+	})
+
+	t.Run("system_addReservedPeer", func(t *testing.T) {
+		t.Parallel()
+		t.Skip("test not implemented")
+	})
+
+	t.Run("system_removeReservedPeer", func(t *testing.T) {
+		t.Parallel()
+		t.Skip("test not implemented")
+	})
+
+	t.Run("system_nodeRoles", func(t *testing.T) {
+		t.Parallel()
+		t.Skip("test not implemented")
+	})
+
+	t.Run("system_accountNextIndex", func(t *testing.T) {
+		t.Parallel()
+		t.Skip("test not implemented")
+	})
 }

tests/utils/retry/untilok.go

@@ -0,0 +1,36 @@
+package retry
+
+import (
+	"context"
+	"fmt"
+	"time"
+)
+
+// UntilOK retries the function `f` until it succeeds.
+// It waits `retryWait` after each failed call to `f`.
+// If the context `ctx` is canceled, the function returns
+// immediately an error stating the number of failed tries,
+// for how long it retried and the context error.
+func UntilOK(ctx context.Context, retryWait time.Duration,
+	f func() (ok bool)) (err error) {
+	failedTries := 0
+	for ctx.Err() == nil {
+		ok := f()
+		if ok {
+			return nil
+		}
+
+		failedTries++
+		waitCtx, waitCancel := context.WithTimeout(ctx, retryWait)
+		<-waitCtx.Done()
+		waitCancel()
+	}
+
+	totalRetryTime := time.Duration(failedTries) * retryWait
+	tryWord := "try"
+	if failedTries > 1 {
+		tryWord = "tries"
+	}
+	return fmt.Errorf("failed after %d %s during %s (%w)",
+		failedTries, tryWord, totalRetryTime, ctx.Err())
+}