Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(tests): refactor tests/utils/request_utils.go #2385

Merged
merged 7 commits into from
Mar 31, 2022
Merged

chore(tests): refactor tests/utils/request_utils.go #2385

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e3dd868
chore(tests): refactor request utils
qdm12 Mar 14, 2022
f0a3487
Use `defer cancel()` for `getResponse` contexts
qdm12 Mar 28, 2022
93aaf5b
Simplify `endpoint` building using `fmt.Sprintf`
qdm12 Mar 28, 2022
1490e42
Fix rebase issues
qdm12 Mar 28, 2022
5d96081
Add missing `defer`
qdm12 Mar 29, 2022
318ed49
Fix total time counting for `PostRPCWithRetry`
qdm12 Mar 30, 2022
13fcf3e
Update tests/utils/gossamer_utils.go
qdm12 Mar 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions tests/rpc/rpc_00_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package rpc

import (
"context"
"fmt"
"os"
"reflect"
Expand Down Expand Up @@ -38,13 +39,14 @@ type testCase struct {
skip bool
}

func getResponse(t *testing.T, test *testCase) interface{} {
func getResponse(ctx context.Context, t *testing.T, test *testCase) interface{} {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this function not just accept a time.Duration for timeout and handle context creation and cancellation within this function? Doesn't look like the contexts are used for anything other than timeout functionality.

Copy link
Contributor Author

@qdm12 qdm12 Mar 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes for now, but the idea I have at the back of my head is to use it with cancel() in the future once it's more event-driven.

Context offer the flexibility of timeouts + event driven cancellation, and inherit from each other, so we should really be using them in e2e tests instead of timers/signal channels which would make a mess of a code especially regarding 'cascaded cancelation'.

Also contexts are part of the std library, and most standard library use them like http.NewRequestWithContext or exec.CommandContext

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further down the line in my branches, this context is actually used to cancel operations, for example if a node crashes unexpectedly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd argue that we shouldn't be using context for cancellation. Dave Cheney wrote about it in his blog.

The documentation for the context package strongly recommends that context.Context is only for request scoped values:

With regards to cascading cancellation, you can just have a cancel channel that is closed and can be listened on by multiple goroutines.

I know using contexts for cancellation is a common pattern. It's something to keep an eye on for Go 2. They may remove the cancellation from the context package.

Copy link
Contributor Author

@qdm12 qdm12 Mar 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting blog post, and I 100% agree context values are the fruit of demoniac developers 👿 (except they're 100% needed for i.e. a server metrics given restrictive signatures of handlers).

It's also sad they didn't figure out how to wait for a goroutine to finish on context cancellation, forcing to still have another signal/error channel to wait.

Now for the closing of a signal channel, there are 3 limitations I can think of:

  • Handling timers to close a channel is painful (always need to check to empty the timer channel as well), and even more when nesting them.
  • You cannot close a channel multiple times, so you would have to pretty much re-invent contexts for this (check if the channel is closed etc.)
  • The standard library accepts contexts to cancel IO operations, but not close channels and often have no easy Stop method. One could use a signal channel to cancel a context down the callers, but it feels wrong to me.

if test.skip {
t.Skip("RPC endpoint not yet implemented")
return nil
}

respBody, err := utils.PostRPC(test.method, utils.NewEndpoint(currentPort), test.params)
endpoint := utils.NewEndpoint(currentPort)
respBody, err := utils.PostRPC(ctx, endpoint, test.method, test.params)
require.NoError(t, err)

target := reflect.New(reflect.TypeOf(test.expected)).Interface()
Expand Down
6 changes: 5 additions & 1 deletion tests/rpc/rpc_01-system_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package rpc

import (
"context"
"testing"
"time"

Expand Down Expand Up @@ -97,7 +98,10 @@ func TestSystemRPC(t *testing.T) {

for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
target := getResponse(t, test)
ctx := context.Background()
getResponseCtx, cancel := context.WithTimeout(ctx, time.Second)
defer cancel()
target := getResponse(getResponseCtx, t, test)

switch v := target.(type) {
case *modules.SystemHealthResponse:
Expand Down
6 changes: 5 additions & 1 deletion tests/rpc/rpc_02-author_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package rpc

import (
"bytes"
"context"
"fmt"
"testing"
"time"
Expand Down Expand Up @@ -139,7 +140,10 @@ func TestAuthorRPC(t *testing.T) {

for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
_ = getResponse(t, test)
ctx := context.Background()
getResponseCtx, cancel := context.WithTimeout(ctx, time.Second)
defer cancel()
_ = getResponse(getResponseCtx, t, test)
})
}

Expand Down
7 changes: 6 additions & 1 deletion tests/rpc/rpc_03-chain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package rpc

import (
"context"
"log"
"testing"
"time"
Expand Down Expand Up @@ -72,7 +73,11 @@ func TestChainRPC(t *testing.T) {
test.params = "[\"" + chainBlockHeaderHash + "\"]"
}

target := getResponse(t, test)
ctx := context.Background()

getResponseCtx, cancel := context.WithTimeout(ctx, time.Second)
defer cancel()
target := getResponse(getResponseCtx, t, test)

switch v := target.(type) {
case *modules.ChainBlockHeaderResponse:
Expand Down
6 changes: 5 additions & 1 deletion tests/rpc/rpc_04-offchain_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package rpc

import (
"context"
"testing"
"time"

Expand Down Expand Up @@ -43,7 +44,10 @@ func TestOffchainRPC(t *testing.T) {

for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
_ = getResponse(t, test)
ctx := context.Background()
getResponseCtx, cancel := context.WithTimeout(ctx, time.Second)
defer cancel()
_ = getResponse(getResponseCtx, t, test)
})
}

Expand Down
31 changes: 26 additions & 5 deletions tests/rpc/rpc_05-state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package rpc

import (
"context"
"fmt"
"testing"
"time"
Expand Down Expand Up @@ -33,7 +34,11 @@ func TestStateRPCResponseValidation(t *testing.T) {

time.Sleep(time.Second) // give server a second to start

blockHash, err := utils.GetBlockHash(t, nodes[0], "")
ctx := context.Background()

getBlockHashCtx, cancel := context.WithTimeout(ctx, time.Second)
blockHash, err := utils.GetBlockHash(getBlockHashCtx, t, nodes[0], "")
cancel()
require.NoError(t, err)

testCases := []*testCase{
Expand Down Expand Up @@ -118,7 +123,10 @@ func TestStateRPCResponseValidation(t *testing.T) {

for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
_ = getResponse(t, test)
ctx := context.Background()
getResponseCtx, cancel := context.WithTimeout(ctx, time.Second)
defer cancel()
_ = getResponse(getResponseCtx, t, test)
})
}

Expand All @@ -142,7 +150,11 @@ func TestStateRPCAPI(t *testing.T) {

time.Sleep(5 * time.Second) // Wait for block production

blockHash, err := utils.GetBlockHash(t, nodes[0], "")
ctx := context.Background()

getBlockHashCtx, cancel := context.WithTimeout(ctx, time.Second)
blockHash, err := utils.GetBlockHash(getBlockHashCtx, t, nodes[0], "")
cancel()
require.NoError(t, err)

const (
Expand Down Expand Up @@ -319,7 +331,11 @@ func TestStateRPCAPI(t *testing.T) {
// Cases for valid block hash in RPC params
for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
respBody, err := utils.PostRPC(test.method, utils.NewEndpoint(nodes[0].RPCPort), test.params)
ctx := context.Background()
postRPCCtx, cancel := context.WithTimeout(ctx, time.Second)
endpoint := utils.NewEndpoint(nodes[0].RPCPort)
respBody, err := utils.PostRPC(postRPCCtx, endpoint, test.method, test.params)
cancel()
require.NoError(t, err)

require.Contains(t, string(respBody), test.expected)
Expand Down Expand Up @@ -351,7 +367,12 @@ func TestRPCStructParamUnmarshal(t *testing.T) {
params: `[["0xf2794c22e353e9a839f12faab03a911bf68967d635641a7087e53f2bff1ecad3c6756fee45ec79ead60347fffb770bcdf0ec74da701ab3d6495986fe1ecc3027"],"0xa32c60dee8647b07435ae7583eb35cee606209a595718562dd4a486a07b6de15", null]`, //nolint:lll
}
t.Run(test.description, func(t *testing.T) {
respBody, err := utils.PostRPC(test.method, utils.NewEndpoint(nodes[0].RPCPort), test.params)
ctx := context.Background()

postRPCCtx, cancel := context.WithTimeout(ctx, time.Second)
endpoint := utils.NewEndpoint(nodes[0].RPCPort)
respBody, err := utils.PostRPC(postRPCCtx, endpoint, test.method, test.params)
cancel()
require.NoError(t, err)
require.NotContains(t, string(respBody), "json: cannot unmarshal")
fmt.Println(string(respBody))
Expand Down
6 changes: 5 additions & 1 deletion tests/rpc/rpc_06-engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package rpc

import (
"context"
"testing"
"time"

Expand Down Expand Up @@ -38,7 +39,10 @@ func TestEngineRPC(t *testing.T) {

for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
_ = getResponse(t, test)
ctx := context.Background()
getResponseCtx, cancel := context.WithTimeout(ctx, time.Second)
defer cancel()
_ = getResponse(getResponseCtx, t, test)
})
}

Expand Down
6 changes: 5 additions & 1 deletion tests/rpc/rpc_07-payment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package rpc

import (
"context"
"testing"
"time"

Expand Down Expand Up @@ -33,7 +34,10 @@ func TestPaymentRPC(t *testing.T) {

for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
_ = getResponse(t, test)
ctx := context.Background()
getResponseCtx, cancel := context.WithTimeout(ctx, time.Second)
defer cancel()
_ = getResponse(getResponseCtx, t, test)
})
}

Expand Down
6 changes: 5 additions & 1 deletion tests/rpc/rpc_08-contracts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package rpc

import (
"context"
"testing"
"time"

Expand Down Expand Up @@ -38,7 +39,10 @@ func TestContractsRPC(t *testing.T) {

for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
_ = getResponse(t, test)
ctx := context.Background()
getResponseCtx, cancel := context.WithTimeout(ctx, time.Second)
defer cancel()
_ = getResponse(getResponseCtx, t, test)
})
}

Expand Down
6 changes: 5 additions & 1 deletion tests/rpc/rpc_09-babe_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package rpc

import (
"context"
"testing"
"time"

Expand Down Expand Up @@ -33,7 +34,10 @@ func TestBabeRPC(t *testing.T) {

for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
_ = getResponse(t, test)
ctx := context.Background()
getResponseCtx, cancel := context.WithTimeout(ctx, time.Second)
defer cancel()
_ = getResponse(getResponseCtx, t, test)
})
}

Expand Down
8 changes: 7 additions & 1 deletion tests/rpc/system_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
package rpc

import (
"context"
"fmt"
"reflect"
"strconv"
"testing"
Expand Down Expand Up @@ -53,7 +55,11 @@ func TestStableNetworkRPC(t *testing.T) {

for _, test := range testsCases {
t.Run(test.description, func(t *testing.T) {
respBody, err := utils.PostRPC(test.method, "http://"+utils.HOSTNAME+":"+utils.PORT, "{}")
ctx := context.Background()

endpoint := fmt.Sprintf("http://%s:%s", utils.HOSTNAME, utils.PORT)
const params = "{}"
respBody, err := utils.PostRPC(ctx, endpoint, test.method, params)
require.NoError(t, err)

target := reflect.New(reflect.TypeOf(test.expected)).Interface()
Expand Down
35 changes: 28 additions & 7 deletions tests/stress/grandpa_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package stress

import (
"context"
"os"
"testing"
"time"
Expand All @@ -25,11 +26,16 @@ func TestStress_Grandpa_OneAuthority(t *testing.T) {

time.Sleep(time.Second * 10)

compareChainHeadsWithRetry(t, nodes)
prev, _ := compareFinalizedHeads(t, nodes)
ctx := context.Background()

const getChainHeadTimeout = time.Second
compareChainHeadsWithRetry(ctx, t, nodes, getChainHeadTimeout)

const getFinalizedHeadTimeout = time.Second
prev, _ := compareFinalizedHeads(ctx, t, nodes, getFinalizedHeadTimeout)

time.Sleep(time.Second * 10)
curr, _ := compareFinalizedHeads(t, nodes)
curr, _ := compareFinalizedHeads(ctx, t, nodes, getFinalizedHeadTimeout)
require.NotEqual(t, prev, curr)
}

Expand All @@ -48,9 +54,13 @@ func TestStress_Grandpa_ThreeAuthorities(t *testing.T) {
require.Len(t, errList, 0)
}()

ctx := context.Background()

numRounds := 5
for i := 1; i < numRounds+1; i++ {
fin, err := compareFinalizedHeadsWithRetry(t, nodes, uint64(i))
const getFinalizedHeadByRoundTimeout = time.Second
fin, err := compareFinalizedHeadsWithRetry(ctx, t,
nodes, uint64(i), getFinalizedHeadByRoundTimeout)
require.NoError(t, err)
t.Logf("finalised hash in round %d: %s", i, fin)
}
Expand All @@ -70,9 +80,13 @@ func TestStress_Grandpa_SixAuthorities(t *testing.T) {
require.Len(t, errList, 0)
}()

ctx := context.Background()

numRounds := 10
for i := 1; i < numRounds+1; i++ {
fin, err := compareFinalizedHeadsWithRetry(t, nodes, uint64(i))
const getFinalizedHeadByRoundTimeout = time.Second
fin, err := compareFinalizedHeadsWithRetry(ctx, t, nodes,
uint64(i), getFinalizedHeadByRoundTimeout)
require.NoError(t, err)
t.Logf("finalised hash in round %d: %s", i, fin)
}
Expand All @@ -95,9 +109,13 @@ func TestStress_Grandpa_NineAuthorities(t *testing.T) {
require.Len(t, errList, 0)
}()

ctx := context.Background()

numRounds := 3
for i := 1; i < numRounds+1; i++ {
fin, err := compareFinalizedHeadsWithRetry(t, nodes, uint64(i))
const getFinalizedHeadByRoundTimeout = time.Second
fin, err := compareFinalizedHeadsWithRetry(ctx, t, nodes,
uint64(i), getFinalizedHeadByRoundTimeout)
require.NoError(t, err)
t.Logf("finalised hash in round %d: %s", i, fin)
}
Expand Down Expand Up @@ -129,9 +147,12 @@ func TestStress_Grandpa_CatchUp(t *testing.T) {
require.NoError(t, err)
nodes = append(nodes, node)

ctx := context.Background()

numRounds := 10
for i := 1; i < numRounds+1; i++ {
fin, err := compareFinalizedHeadsWithRetry(t, nodes, uint64(i))
const getFinalizedHeadByRoundTimeout = time.Second
fin, err := compareFinalizedHeadsWithRetry(ctx, t, nodes, uint64(i), getFinalizedHeadByRoundTimeout)
require.NoError(t, err)
t.Logf("finalised hash in round %d: %s", i, fin)
}
Expand Down
Loading