Add unit tests for client.go #36

aulorbe · 2024-07-09T23:57:42Z

Problem

We do not have unit tests for client.go. In an effort to work towards v1.0 readiness, we need them.

Solution

Add unit tests for key functions/methods in client.go, as outlined in this Asana ticket.

Add commands to Justfile to allow contributors to run unit tests or integration tests, if they don't want to run all tests with the usual just test cmd.

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Infrastructure change (CI configs, etc)
Non-code change (docs, etc)
None of the above: (explain here)

Test Plan

CI passes.

aulorbe · 2024-07-10T00:01:13Z

justfile

+    set -o allexport
+    source .env
+    set +o allexport
+    go test -count=1 -v -run TestIntegrationClient ./pinecone


This runs all tests that are methods on the ClientTestsIntegration object.

The idea here is that we can have separate structs for integration tests vs. unit tests. Right now, we only have one for integration tests, but we could have one for unit tests in the future. If we do that, we would change the test-unit command in the justfile to call whatever function we create that actually implements suite.Run(....), as we do here by calling TestIntegrationClient.

I know you have another PR for adding tests to index connection as well - is there a way to just use "Integration" for this like the test-unit command? It feels like this will only run integration tests covered by TestIntegrationClient rather than any that include Integration.

You're right -- for some reason in my head there was a reason I did it differently for integration vs unit tests, but now I can't remember... and it doesn't seem to matter! I'll change :)

aulorbe · 2024-07-10T00:01:24Z

justfile

+    set -o allexport
+    source .env
+    set +o allexport
+    go test -v -run Unit ./pinecone


This runs all tests that contain Unit in the title.

(Strategy lifted from Subtests section of testing documentation here https://pkg.go.dev/testing#hdr-Subtests_and_Sub_benchmarks, which basically just uses a regex to run groups of tests.)

(See this comment for more thoughts.)

austin-denoble

Nice work! Thanks for splitting out integration and unit tests a bit better, plus adding all this coverage for various things.

Main comment of mine is how we're triggering tests in the justfile and whether that'll work for index_connection as well.

austin-denoble · 2024-07-17T16:22:02Z

justfile

+    set -o allexport
+    source .env
+    set +o allexport
+    go test -count=1 -v -run TestIntegrationClient ./pinecone


I know you have another PR for adding tests to index connection as well - is there a way to just use "Integration" for this like the test-unit command? It feels like this will only run integration tests covered by TestIntegrationClient rather than any that include Integration.

austin-denoble · 2024-07-17T16:30:53Z

pinecone/client_test.go

@@ -11,14 +12,18 @@ import (
 	"strings"
 	"testing"

+	"github.com/google/go-cmp/cmp"
+	"github.com/pinecone-io/go-pinecone/internal/gen/control"
+


nit: These imports seem unordered 🤔 did you run gofmt?

I did! But I'll do it again :)

[we spoke offline & this is actually correct; leaving as-is]

austin-denoble · 2024-07-17T16:34:48Z

pinecone/client_test.go

+		err      int
+		expected *PineconeError
+	}{
+		{ // TODO: should really add a negative case here, but triggering a marshalling error is hard


Just curious, what would a negative case be for this?

uhh idk , ignore this, apologies

It's no problem, was just curious

austin-denoble · 2024-07-17T16:50:11Z

pinecone/client_test.go

+		}, {
+			name:     "Confirm http prefix is added",
+			url:      "http://pinecone-api.io",
+			expected: "http://pinecone-api.io", // TODO: should this be https?


Nope, it's expected that this should be http if they explicitly provided it. This is helpful for developing against something like Pinecone local.

austin-denoble · 2024-07-17T16:51:12Z

pinecone/client_test.go

+	// Save the current environment variable value and defer restoring it
+	originalHostEnv := os.Getenv("PINECONE_CONTROLLER_HOST")
+	defer os.Setenv("PINECONE_CONTROLLER_HOST", originalHostEnv)


Oh right we can defer to do this. I should update some of the other tests...

austin-denoble · 2024-07-17T16:53:55Z

pinecone/client_test.go

+			// Set environment variable for the test case
+			os.Setenv("PINECONE_CONTROLLER_HOST", tc.envHost)


Should this only be set if tc.envHost != ""?

no because then the 3rd test fails, since we set the env var "PINECONE_CONTROLLER_HOST" to whatever the value is of tc.envHost.

For the 3rd test, tc.envHost is equal to "". So then when we asserts that tc.expectedHost is equal to client.restClient.Server, it fails because "" is not equal to https://api.pinecone.io/

austin-denoble

LGTM

## Problem We do not have unit tests for the private functions inside `index_connection.go`, as outlined in [this Asana ticket](https://app.asana.com/0/1203260648987893/1207762835203548/f). ## Solution Add 'em! Note: I've adjusted the `Justfile` and renamed the integration test components in accordance with the strategy outlined in [this PR](#36) (unit tests for `client.go`). ## Type of Change - [ ] Bug fix (non-breaking change which fixes an issue) - [x] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update - [ ] Infrastructure change (CI configs, etc) - [x] Non-code change (docs, etc) - [ ] None of the above: (explain here) ## Test Plan CI passes.

@aulorbe

…efactor `Client.Index()`, bump `grpc` dep to 1.64.0 (#35) We currently do not give users many options for configuring the underlying `grpc.ClientConn` that's used by the `VectorServiceClient` for interacting with the data plane. This will be important for allowing configuration of things like timeouts and proxies, and also for allowing us to unit test aspects of the data plane layer. We have an open issue to allow more configuration of our gRPC implementation: #13 I ended up implementing a few different things here. Ultimately, I feel like these changes move us more in the right direction for allowing configuration of our gRPC service for data plane operations. I decided on leaving things a bit open-ended for the consumer. We give them the option to pass variadic `grpc.DialOption` arguments when calling `Client.Index(NewIndexConnParams{...})`. This makes providing these options optional, and you can continue calling the function without them as before. If someone wants to configure the gRPC connection themselves, they have some latitude to do that via the `Host` and `grpc.DialOption` helper functions: https://pkg.go.dev/google.golang.org/[email protected]#DialOption. However, since we're also applying some dial options of our own when constructing the client, allowing this flexibility may require more care when merging things in `newIndexConnection`. Specifically looking for feedback on this if people have opinions. Being able to provide these directly made it possible to unit test some of the behavior as well. - Bump grpc version from `1.62.0` -> `1.64.0`. @aulorbe and I noticed there was an issue with how we were calling `grpc.Dial()` for setting up the `grpc.ClientConn`. If you passed in an invalid `host`, the `Dial` operation would hang without error'ing for some period of time. I think ultimately we should have been using `grpc.DialContext` to configure the timeout, or using `grpc.WithTimeout()`. When looking at fixing this I bumped our grpc dependency, and noticed the `Dial` and `DialContext` methods are under deprecation notice - they want people using `grpc.NewClient`. It felt like it made sense to go ahead and make this change now: - `grpc.Dial` - https://pkg.go.dev/google.golang.org/[email protected]#Dial - `grpc.NewClient` - https://pkg.go.dev/google.golang.org/[email protected]#NewClient - Refactor `Client` to have only one `Index` method add `NewIndexConnParams` struct for passing to `Index` rather than discrete arguments. It already felt like our functions for establishing an `IndexConnection` were getting a bit bloated, and when I was looking at adding in gRPC configuration options, it felt like we should go ahead and update this before v1.0.0. - Add unit test to validate metadata in outgoing RPC requests, and unit tests to validate Client applies the appropriate values to `IndexConnection` through `Client.Index`, `along with Client.extractAuthHeader()`. - Fix formatting in doc comment code blocks in both `client.go` and `index_connection.go`. - Bump `google.golang.org/grpc` to v1.65.0 to correct snyk security vulnerability: https://security.snyk.io/vuln/SNYK-GOLANG-GOOGLEGOLANGORGGRPCMETADATA-7430177 - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [X] Breaking change (fix or feature that would cause existing functionality to not work as expected) - [ ] This change requires a documentation update - [ ] Infrastructure change (CI configs, etc) - [ ] Non-code change (docs, etc) - [ ] None of the above: (explain here) Make sure CI for unit + integration tests is passing. I've added unit tests to specifically target gRPC metadata, testing passing a `grpc.DialOption`, and creating an `IndexConnection` via `Client.Index()`.

aulorbe commented Jul 10, 2024

View reviewed changes

aulorbe force-pushed the Audrey/add-unit-tests branch from 385eb4a to ec1d912 Compare July 11, 2024 03:30

aulorbe marked this pull request as ready for review July 15, 2024 17:45

aulorbe requested a review from austin-denoble July 15, 2024 17:45

aulorbe changed the title ~~Add unit tests + differentiation for integration tests~~ Add unit tests for client.go Jul 15, 2024

aulorbe mentioned this pull request Jul 15, 2024

Add unit tests for index_connection.go #38

Merged

7 tasks

austin-denoble approved these changes Jul 17, 2024

View reviewed changes

aulorbe requested a review from austin-denoble July 17, 2024 21:17

austin-denoble approved these changes Jul 17, 2024

View reviewed changes

aulorbe and others added 18 commits July 19, 2024 10:16

Add way to run all tests vs integration vs unit

ce465f2

Save work

bb5fb2d

save work

ad3217c

Add test for toIndex

3448bf4

More tests

a7f4304

Add NewClient test

2fc11c0

Add NewClient test, 2

30af8dd

Add tests for buildClientBaseOptions

7210fb2

Save work

78876c2

Test NewClientBase

504e3e9

Refactor unit tests to be more precise

5e1d02a

Update *ClientTests to *ClientTestsIntegration

df8aab8

Consolidate BuildNewClientBase unit tests into 1

abe4628

Add \"unit\" to name so can test

3c8bcc5

Update go.sum file

ae14697

address comments

776b6a4

Make test language more clear

69d8719

aulorbe force-pushed the Audrey/add-unit-tests branch from edfc628 to 69d8719 Compare July 19, 2024 17:17

aulorbe added 3 commits July 19, 2024 10:18

Do not need docs in .gitignore anymore

b43b61d

Accidentally deleted some tests we want to keep

577fd64

Dont need Integration twice in test name

d1010fb

austin-denoble approved these changes Jul 19, 2024

View reviewed changes

aulorbe merged commit a3127fa into main Jul 19, 2024
3 checks passed

aulorbe deleted the Audrey/add-unit-tests branch July 19, 2024 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for client.go #36

Add unit tests for client.go #36

aulorbe commented Jul 9, 2024 •

edited

Loading

aulorbe Jul 10, 2024

aulorbe Jul 10, 2024 •

edited

Loading

austin-denoble Jul 17, 2024

aulorbe Jul 17, 2024

aulorbe Jul 10, 2024

aulorbe Jul 10, 2024 •

edited

Loading

aulorbe Jul 10, 2024

austin-denoble left a comment

austin-denoble Jul 17, 2024

austin-denoble Jul 17, 2024

aulorbe Jul 17, 2024

aulorbe Jul 17, 2024

austin-denoble Jul 17, 2024

aulorbe Jul 17, 2024

austin-denoble Jul 17, 2024

austin-denoble Jul 17, 2024

aulorbe Jul 17, 2024

austin-denoble Jul 17, 2024

austin-denoble Jul 17, 2024

aulorbe Jul 17, 2024

austin-denoble left a comment

		// Set environment variable for the test case
		os.Setenv("PINECONE_CONTROLLER_HOST", tc.envHost)

Add unit tests for client.go #36

Add unit tests for client.go #36

Conversation

aulorbe commented Jul 9, 2024 • edited Loading

Problem

Solution

Type of Change

Test Plan

Choose a reason for hiding this comment

aulorbe Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aulorbe Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin-denoble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austin-denoble left a comment

Choose a reason for hiding this comment

aulorbe commented Jul 9, 2024 •

edited

Loading

aulorbe Jul 10, 2024 •

edited

Loading

aulorbe Jul 10, 2024 •

edited

Loading