From 1a57f670fd0628146f936aba72b7cb2c300e7a17 Mon Sep 17 00:00:00 2001 From: Quentin McGaw Date: Mon, 4 Apr 2022 13:02:52 +0000 Subject: [PATCH] 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 | 233 ++++++++++++++++++-------------- tests/utils/retry/untilok.go | 36 +++++ 2 files changed, 165 insertions(+), 104 deletions(-) create mode 100644 tests/utils/retry/untilok.go diff --git a/tests/rpc/rpc_01-system_test.go b/tests/rpc/rpc_01-system_test.go index e0a289d8b6..b2a5dd8eec 100644 --- a/tests/rpc/rpc_01-system_test.go +++ b/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") + }) } diff --git a/tests/utils/retry/untilok.go b/tests/utils/retry/untilok.go new file mode 100644 index 0000000000..c571127da4 --- /dev/null +++ b/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()) +}