-
Notifications
You must be signed in to change notification settings - Fork 22
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.
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
Flesh out client tests #252
Conversation
require.NoError(err) | ||
} | ||
|
||
func TestCheck(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if these should be Example
s and not Test
s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're tests in the other repositories, and I want them to be exercised by the test harness to demonstrate that they currently work with the library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Example
s get run with go test
they just also show up in the docs as examples
@ecordell i'm also a little worried that I have a flake in the |
v1/client_test.go
Outdated
require.NoError(err) | ||
|
||
// Validate export | ||
exportResponse, err := client.PermissionsServiceClient.ExportBulkRelationships(context.Background(), &v1.ExportBulkRelationshipsRequest{}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since these are meant as examples, maybe it would be good to make a context with cancellation and pass it in?
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)
ctx = context.WithTimeout(ctx, 5*time.Second)
etc
v1/client.go
Outdated
@@ -11,6 +11,9 @@ import ( | |||
// | |||
// Clients are backed by a gRPC client and as such are thread-safe. | |||
type Client struct { | |||
// Provide a handle on the underlying connection to enable cleanup | |||
// behaviors (among others) | |||
Conn *grpc.ClientConn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's make this conn
(private) and then add a top level func (Client) Close() error
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Description
We had a user asking in Discord about how to consume a server stream, and I was going to point them at the tests when I realized that we don't actually have those tests in this repo. This is more about having a mechanism for documentation than about any concern about correctness.
Changes
uuid
as an explicit dep for generating test tokensConn
as a member of the client struct so that callers can do things like teardown logicTesting
Review, see that things are green