pinecone-io / go-pinecone

Pinecone.io Golang Client
Apache License 2.0
49 stars 9 forks source link

Centralize Go test suite #48

Closed aulorbe closed 4 months ago

aulorbe commented 4 months ago

Problem

Our current test set up is not as useful as it could be. Some issues are:

Solution

Make it all better!

The current architecture now looks like this:

Two top-level test infra files:

  1. integration_test_suite.go: This file defines a single test struct called IntegrationTests that holds the fields for everything we need wrt integration testing across all go files in our project a. Importantly, this file also contains the testify mandatory SetupSuite and TeardownSuite methods attached to this struct, so that indexes are always torn down after testing completes
  2. run_integration_test_suites.go: This file actually runs the test suites defined in integration_test_suite.go via testify's suite.Run command. It runs 2 suites: 1 for pods (podTestSuite) and 1 for serverless (serverlessTestSuite)

Individual test files: Each file still has a complementary ..._test.go file that contains its integration and unit tests. The main difference this PR introduces is that each of these files no longer contains a redundant SetupSuite function, etc. Instead, they simply call run_integration_test_suites.go's RunSuites() method, and everything is automagically created/destroyed.

Genesis

This refactor arose from Audrey trying to write integration tests for update, but being unable to do so, since she could not easily compare IDs, vector values or metadata before vs after update operations.

Misc.:

There is still a lot of things we can do to make our tests better and more efficient, I'm sure. This is just one baby step on the longer journey towards test suite-maturity.

FAQs

Why do we need two infra-type files (integration_test_suite.go and run_integration_test_suites.go)?

I don't like it either, but apparently this is what is needed for testify to work 😢 . You can't have the suite.Run call in the same file as the SetupSuite and TeardownSuite methods.

Does the fact that all (integration) tests now share the same struct (IntegrationTests) mean that when you run the integration tests in a specific file (e.g. client_tests.go), all integration tests actually run?

Yes. This obviously isn't ideal for dev work, but you can figure out how to run individual tests via the command line by reading up on go test.

Why have different Suites for pods vs serverless indexes, when they share most fields?

This is totally fair and tbh I simply didn't refactor this part because this PR is getting gigantic and it seemed like it would add unnecessary complexity to it. But we should go over the pros/cons of having everything in a single Suite later! For now, splitting them out produced the invaluable outcome of allowing me to test different things per index type.

Type of Change

Test Plan

CI passes.