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

test deadline protection and convenience methods #416

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

test deadline protection and convenience methods #416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 6 additions & 13 deletions pkg/loop/internal/ccip/test/onramp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,11 @@ package test

import (
"context"
"fmt"
"net"

"reflect"
"sync"
"testing"

"github.com/hashicorp/consul/sdk/freeport"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"google.golang.org/grpc"
Expand All @@ -24,7 +22,7 @@ func TestStaticOnRamp(t *testing.T) {
t.Parallel()

// static test implementation is self consistent
ctx := context.Background()
ctx := tests.Context(t)
assert.NoError(t, OnRampReader.Evaluate(ctx, OnRampReader))

// error when the test implementation is evaluates something that differs from the static implementation
Expand All @@ -39,19 +37,14 @@ func TestOnRampGRPC(t *testing.T) {
t.Parallel()
ctx := tests.Context(t)
// create a price registry server
port := freeport.GetOne(t)
addr := fmt.Sprintf("localhost:%d", port)
lis, err := net.Listen("tcp", addr)
require.NoError(t, err, "failed to listen on port %d", port)
t.Cleanup(func() { lis.Close() })
// we explicitly stop the server later, do not add a cleanup function here
testServer := grpc.NewServer()
lis, testServer, addr := tests.SetupServer(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need to return lis?

tests.SetupGRPCServer would be a better name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair - I'm not sure how much I like it either. I followed a similar pattern to WSRPC tests.


// handle client close and server stop
shutdown := make(chan struct{})
closer := &serviceCloser{closeFn: func() error { close(shutdown); return nil }}

onRamp := ccip.NewOnRampReaderGRPCServer(OnRampReader)
require.NoError(t, err)
onRamp = onRamp.AddDep(closer)

ccippb.RegisterOnRampReaderServer(testServer, onRamp)
Expand All @@ -65,12 +58,12 @@ func TestOnRampGRPC(t *testing.T) {
wg.Add(1)
go func() {
defer wg.Done()
<-shutdown
tests.RequireSignal(t, shutdown, "Failed to receive shutdown signal")
Copy link
Contributor

Choose a reason for hiding this comment

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

i really dislike this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was actually why I wanted to open the PR - I was worried about shutdown hanging indefinitely and preventing the test from closing (and then saw the opportunity to reduce some redundancy in the setup used by all the tests).

In my first iteration on this PR I implemented RequireSignal by hand (i.e. a for loop to check if the shutdown had received anything or if we had time'd out), and then later saw that Jordan had beat me to it a while back.

Copy link
Contributor

Choose a reason for hiding this comment

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

the test timeout will prevent them from hanging forever. perhaps requiresignal is a nice abstraction in a complicated code base, where you don't control the channel. here it is overkill.

t.Log("shutting down server")
testServer.Stop()
}()
// create a price registry client
conn, err := grpc.Dial(addr, grpc.WithTransportCredentials(insecure.NewCredentials()))
conn, err := tests.SetupClientConnWithOptsAndTimeout(t, addr, grpc.WithTransportCredentials(insecure.NewCredentials()))
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe if we change this to ~ test.InsecureDial(t, addr) and mv the require.NoError & t.cleanup calls

require.NoError(t, err, "failed to dial %s", addr)
t.Cleanup(func() { conn.Close() })
client := ccip.NewOnRampReaderGRPCClient(conn)
Expand Down
35 changes: 35 additions & 0 deletions pkg/utils/tests/tests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@ package tests

import (
"context"
"fmt"
"net"
"testing"
"time"

"github.com/hashicorp/consul/sdk/freeport"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"google.golang.org/grpc"
)

type TestingT interface {
Expand Down Expand Up @@ -68,3 +72,34 @@ func RequireSignal(t *testing.T, ch <-chan struct{}, failMsg string) {
t.Fatal(failMsg)
}
}

// Convenience method to setup a client connection with a default timeout of 5 seconds
func SetupClientConnWithOptsAndTimeout(t *testing.T, target string, opts ...grpc.DialOption) (*grpc.ClientConn, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you like this convenience method, I can refactor the other tests to consume it.

ctx, cancel := context.WithTimeout(Context(t), 5*time.Second)
t.Cleanup(cancel)

return grpc.DialContext(ctx, target, opts...)
}

// SetupServer is a convenience method to set up a grpc server for most testing
// usecases.
func SetupServer(t *testing.T, opts ...grpc.ServerOption) (net.Listener, *grpc.Server, string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you like this convenience method, I can refactor the other tests to consume it.

// Attempt to reconnect to the port which the OS may not have had time
// to clean up between tests.
var (
lis net.Listener
err error
)

port := freeport.GetOne(t)
addr := fmt.Sprintf("localhost:%d", port)
require.Eventually(t, func() bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think the eventually is needed. it's a synchronous call that has no good reason to recover.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The OS can still be freeing up the port while the next test is executed. This eventually block gives the OS some wiggle room to do that. This isn't strictly needed as you the current tests use freeport, but there may be scenarios in the future that want to use the same port across tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

freeport library handles the concern. there is no clear advantage to reusing ports across unit tests and there are clear disadvantages. we removed a number of flake integration test in core by adopting freeport instead of trying to manage it ourselves

lis, err = net.Listen("tcp", addr)

return err == nil
}, 5*time.Second, TestInterval)

t.Cleanup(func() { lis.Close() })

return lis, grpc.NewServer(opts...), addr
}