Centralize test indexes for integration tests
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.ymlcalledsetupthat calls a file calledsetup.ts(src/integration/setup.ts) - I added a job in
testing.ymlcalledteardownthat calls a file calledteardown.ts(src/integration/teardown.ts) setup.tssets 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
todoin this file to refactor it; right now, it's quite rudimentary in the way it loops and checks conditions; you'll notice othertodos 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.
- I have put a
teardown.tsdeletes 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 inpackage.jsonthat you can execute by runningnpm 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
waitUntilReadywhere thestatussometimes evaluated toReadywithout the index actually being ready. So, this PR includes a fix to that function that also checks forstate.readybeingtrueand adds a value (that it also checks for) to theIndexModelStatusStateEnumthat accounts for an index's state beingUpgrading, 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-parallelfield too intesting.yml(although I tried removing it altogether and we had failures) - I removed the job in
testing.ymlthat callsutils/cleanupResources.ts, since I replaced it with myTeardownjob. This file (cleanupResources) is still called via cron, though, bytesting-cleanup.yml - I decided not to use jest's
globalSetupandglobalTeardowncapabilities 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
(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.)
(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.
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.
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 🎉