pinecone-ts-client icon indicating copy to clipboard operation
pinecone-ts-client copied to clipboard

Centralize test indexes for integration tests

Open aulorbe opened this issue 1 year ago • 4 comments

Problem

Our integration tests are currently very flakey (meaning they fail in an non-deterministic manner depending largely on fluctuations in latency on backend operations).

Solution

Centralize the creation and deletion of indexes for integration tests; cleanup the tests where I can.

The tests that now use the centralized indexes: - Control: - describeIndex.test.ts - listIndexes.test.ts - Data: - fetch.test.ts - list.test.ts - query.test.ts

The tests that still have to create/delete their own indexes because their operations are difficult to undo: - Control: - configureIndex.test.ts - createIndex.test.ts - Data: - delete.test.ts - upsertAndUpdate.test.ts

New jobs & files

  • I added a job in testing.yml called setup that calls a file called setup.ts (src/integration/setup.ts)
  • I added a job in testing.yml called teardown that calls a file called teardown.ts (src/integration/teardown.ts)
  • setup.ts sets up (creates and seeds) a shared serverless index (only serverless for now b/c we don't have pod-based data plane tests). The index name is randomized, so simultaneous jobs can occur using the same Pinecone API key 😄 .
    • I have put a todo in this file to refactor it; right now, it's quite rudimentary in the way it loops and checks conditions; you'll notice other todos in other files that point to where we need more tests, etc. I'd like to keep these in there for now, if that's okay.
  • teardown.ts deletes this index
  • I found some tests that were not actually testing what they intended to, so I have marked those as skipped for now (e.g. createIndex.test.ts > test('insufficient quota')`)
  • I added a file called integrationRunner.js, which is a file for running integration tests locally with a new command in package.json that you can execute by running npm run test:integration-local:node. This will do the setup, tests, and teardown all in 1 go for you locally.

Bug fixes

  • Last week, @austin-denoble and I noticed a bug in waitUntilReady where the status sometimes evaluated to Ready without the index actually being ready. So, this PR includes a fix to that function that also checks for state.ready being true and adds a value (that it also checks for) to the IndexModelStatusStateEnum that accounts for an index's state being Upgrading, which accounted for a lot of flakiness in the configureIndex tests.

Notes

  • The goal of this PR is to decrease flakiness
  • I removed one of our Bun versions to try to get the time down a bit; we might want to experiment with upping the max-parallel field too in testing.yml (although I tried removing it altogether and we had failures)
  • I removed the job in testing.yml that calls utils/cleanupResources.ts, since I replaced it with my Teardown job. This file (cleanupResources) is still called via cron, though, by testing-cleanup.yml
  • I decided not to use jest's globalSetup and globalTeardown capabilities because the "global" scope is per option in our test matrix. Creating a custom setup and teardown job instead allowed me to create a single test index we can use across all options in our matrix, which is more efficient.

Type of Change

  • [x] 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
  • [x] Infrastructure change (CI configs, etc)
  • [ ] Non-code change (docs, etc)
  • [ ] None of the above: (explain here)

Test Plan

CI passes consistently


  • To see the specific tasks where the Asana app for GitHub is being used, see below:
    • https://app.asana.com/0/0/1208439494339500
    • https://app.asana.com/0/0/1207545518662985

aulorbe avatar Oct 03 '24 19:10 aulorbe

(Recent CI failures are due to super flakey upsert-and-update test. Have removed for now. No way to win with that test despite adding a big sleep. Will revisit later.)

aulorbe avatar Oct 04 '24 20:10 aulorbe

(Recent CI failures are due to super flakey upsert-and-update test. Have removed for now. No way to win with that test despite adding a big sleep. Will revisit later.)

Okay. @austin-denoble and I decided that we will assert the client calls the correct endpoint w/the correct params, but not assert things like updates, etc. are actually reflected later. This eliminates a ton of waiting time + works towards separation of concerns, since if updates (e.g.) don't work, that is ultimately a bug with the backend, not the client.

aulorbe avatar Oct 07 '24 18:10 aulorbe

We have had 3 successful* runs in a row (I just reran the latest run x3). I am declaring the flakiness (that we control) surpassed!

*There is a bug with list where sometimes a pagination token is not included in the response. Not counting this as a 'flake' on our end.

aulorbe avatar Oct 08 '24 19:10 aulorbe

I decided not to use jest's globalSetup and globalTeardown capabilities because they interfered with how our integration tests run in parallel in CI -- the "global"-ness of these jest fixtures would be per option in our matrix, so even though they would be "global" to all test files, they would not be global across all CI runs (node, bun, edge, etc.), so collisions happened.

When I initially was thinking about updating things I was thinking "global" in terms of all files - so a single "test" run would be per option in the matrix.

Instead of having a hard-coded "serverlessIndexName" that gets inherently shared, I'd prefer if each run of integration tests had an index name that was randomized, and then shared across all test files in that run.

I'm not sure what issues you were seeing when using the global setup / teardown in Jest. The other client integration test suites tend to create randomized names for indexes and resources per run, so things are shared globally across integration tests / files, but not all possible integration tests that might be run in CI when factoring in a matrix.

Note: we spoke about this offline. We are sticking w/the current approach of having setup and teardown jobs, since it's more efficient, but we are randomizing the index names to allow simultaneous runs by diff users on the same Pinecone account 🎉

aulorbe avatar Oct 09 '24 21:10 aulorbe