Skip to content

Commit

Permalink
Centralize test indexes for integration tests (#291)
Browse files Browse the repository at this point in the history
## TLDR
This PR takes our integration test timing in CI down from ~37mins to ~10mins! 🎉 

## 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 `todo`s 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](https://jestjs.io/docs/configuration#globalsetup-string)
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
  • Loading branch information
aulorbe authored Oct 14, 2024
1 parent 03de66c commit 760a033
Show file tree
Hide file tree
Showing 17 changed files with 780 additions and 639 deletions.
63 changes: 45 additions & 18 deletions .github/workflows/testing.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,30 @@ on:
workflow_call: {}

jobs:
setup:
name: Setup
runs-on: ubuntu-latest
outputs:
serverlessIndexName: ${{ steps.step3.outputs.SERVERLESS_INDEX_NAME }}
steps:
- name: Checkout code
id: step1
uses: actions/checkout@v4

- name: Install dependencies
id: step2
run: npm ci

- name: Run setup script
id: step3
env:
PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }}
run: |
SERVERLESS_INDEX_NAME=$(npx ts-node ./src/integration/setup.ts | grep "SERVERLESS_INDEX_NAME=" | cut -d'=' -f2)
echo "SERVERLESS_INDEX_NAME=$SERVERLESS_INDEX_NAME" >> $GITHUB_OUTPUT
unit-tests:
needs: setup
name: Run unit tests
runs-on: ubuntu-latest
steps:
Expand All @@ -20,8 +43,11 @@ jobs:
run: npm run test:unit -- --coverage

integration-tests:
needs: setup
name: Run integration tests
runs-on: ubuntu-latest
outputs:
serverlessIndexName: ${{ steps.runTests1.outputs.SERVERLESS_INDEX_NAME }}
strategy:
fail-fast: false
max-parallel: 2
Expand Down Expand Up @@ -55,11 +81,15 @@ jobs:
bun-version: ${{ matrix.config.bun_version }}

- name: Run integration tests (Prod)
id: runTests1
if: matrix.pinecone_env == 'prod'
env:
CI: true
PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }}
run: ${{ matrix.config.runner }} run test:integration:${{ matrix.config.jest_env }}
SERVERLESS_INDEX_NAME: ${{ needs.setup.outputs.serverlessIndexName}}
run: |
${{ matrix.config.runner }} run test:integration:${{ matrix.config.jest_env }}
echo "SERVERLESS_INDEX_NAME=${{ needs.setup.outputs.serverlessIndexName}}" >> $GITHUB_OUTPUT
- name: Run integration tests (Staging)
if: matrix.pinecone_env == 'staging'
Expand All @@ -69,28 +99,25 @@ jobs:
PINECONE_CONTROLLER_HOST: 'https://api-staging.pinecone.io'
run: ${{ matrix.config.runner }} run test:integration:${{ matrix.config.jest_env }}

integration-test-cleanup:
name: Clean up integration tests
if: always()
needs: [integration-tests]
teardown:
name: Teardown
needs: integration-tests # Ensure teardown only runs after test jobs
runs-on: ubuntu-latest
if: always() # Run teardown even if the tests fail
steps:
- name: Checkout
- name: Checkout code
uses: actions/checkout@v4
- name: Setup Node
uses: actions/setup-node@v4
with:
node-version: 18.x
registry-url: 'https://registry.npmjs.org'
- name: Install npm packages
run: |
npm install --ignore-scripts
shell: bash
- name: Run integration cleanup command

- name: Install dependencies
run: npm ci

- name: Run teardown script
env:
CI: true
PINECONE_API_KEY: ${{ secrets.PINECONE_API_KEY }}
run: npm run test:integration:cleanup
SERVERLESS_INDEX_NAME: ${{ needs.integration-tests.outputs.serverlessIndexName}}
run: |
npx ts-node ./src/integration/teardown.ts
typescript-compilation-tests:
name: TS compile
runs-on: ubuntu-latest
Expand Down
102 changes: 89 additions & 13 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
"lint": "eslint src/ --ext .ts",
"repl": "npm run build && node utils/replInit.ts",
"test:integration:node": "TEST_ENV=node jest src/integration/ -c jest.config.integration-node.js --runInBand --bail",
"test:integration-local:node": "TEST_ENV=node node src/integration/integrationRunner.js",
"test:integration:edge": "TEST_ENV=edge jest src/integration/ -c jest.config.integration-edge.js --runInBand --bail",
"test:integration-local:edge": "TEST_ENV=edge node src/integration/integrationRunner.js",
"test:integration:cleanup": "npm run build && node utils/cleanupResources.ts",
"test:unit": "jest src/"
},
Expand All @@ -42,7 +44,7 @@
"jest": "^29.5.0",
"jest-progress-bar-reporter": "^1.0.25",
"prettier": "^2.8.8",
"ts-jest": "^29.0.5",
"ts-jest": "^29.2.5",
"typedoc": "^0.24.8",
"typescript": "^4.9.4"
},
Expand Down
59 changes: 41 additions & 18 deletions src/integration/control/configureIndex.test.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,23 @@
import { BasePineconeError, PineconeBadRequestError } from '../../errors';
import {
BasePineconeError,
PineconeBadRequestError,
PineconeInternalServerError,
} from '../../errors';
import { Pinecone } from '../../index';
import { randomIndexName, waitUntilReady } from '../test-helpers';
import {
randomIndexName,
retryDeletes,
sleep,
waitUntilReady,
} from '../test-helpers';

describe('configure index', () => {
let podIndexName, serverlessIndexName;
let pinecone: Pinecone;
let podIndexName: string, serverlessIndexName: string, pinecone: Pinecone;

describe('configure index', () => {
beforeAll(async () => {
pinecone = new Pinecone();
podIndexName = randomIndexName('configureIndex');
serverlessIndexName = randomIndexName('configureIndex');
podIndexName = randomIndexName('pod-configure');
serverlessIndexName = randomIndexName('serverless-configure');

// create pod index
await pinecone.createIndex({
Expand Down Expand Up @@ -42,12 +50,10 @@ describe('configure index', () => {
});

afterAll(async () => {
// wait until indexes are done upgrading before deleting
await waitUntilReady(podIndexName);
await waitUntilReady(serverlessIndexName);

await pinecone.deleteIndex(podIndexName);
await pinecone.deleteIndex(serverlessIndexName);
// Note: using retryDeletes instead of waitUntilReady due to backend bug where index status is ready, but index
// is actually still upgrading
await retryDeletes(pinecone, podIndexName);
await retryDeletes(pinecone, serverlessIndexName);
});

describe('pod index', () => {
Expand All @@ -63,13 +69,31 @@ describe('configure index', () => {
});

test('scale podType up', async () => {
// Verify the starting state
// Verify starting state of podType is same as originally created
const description = await pinecone.describeIndex(podIndexName);
expect(description.spec.pod?.podType).toEqual('p1.x1');

await pinecone.configureIndex(podIndexName, {
spec: { pod: { podType: 'p1.x2' } },
});
// Scale up podType to x2
let state = true;
let retryCount = 0;
const maxRetries = 10;
while (state && retryCount < maxRetries) {
try {
await pinecone.configureIndex(podIndexName, {
spec: { pod: { podType: 'p1.x2' } },
});
state = false;
} catch (e) {
if (e instanceof PineconeInternalServerError) {
retryCount++;
await sleep(2000);
} else {
console.log('Unexpected error:', e);
throw e;
}
}
}
await waitUntilReady(podIndexName);
const description2 = await pinecone.describeIndex(podIndexName);
expect(description2.spec.pod?.podType).toEqual('p1.x2');
});
Expand All @@ -80,7 +104,6 @@ describe('configure index', () => {
await pinecone.configureIndex(serverlessIndexName, {
deletionProtection: 'enabled',
});

await waitUntilReady(serverlessIndexName);

// verify we cannot delete the index
Expand Down
Loading

0 comments on commit 760a033

Please sign in to comment.