Skip to content

Commit

Permalink
Update integration test setup (#109)
Browse files Browse the repository at this point in the history
## Problem

I want Typescript to work properly within my integration tests without
having to manage a separate tsconfig file.

## Solution

Getting this working was a bit of an adventure. As usual, when I thought
I was 80% done I was only actually about 20% done. First I did these
things:

- Move integration tests underneath the `src/` directory, which is the
`rootDir` in our `tsconfig.json` file.
- Update test commands in package.json so that `npm run test:unit` does
not include integration tests.
- Add a new test, `src/integration/data/deleteMany.test.ts`, using a
randomly generated namespace to store test-specific state in the index
from other test runs. In testing these data plane operations, it seems
fine to recycle the same index and save the time it would take to spin
up a fresh one for every test.

That was enough to get tests passing locally with `npm run
test:integration`. Then I tried running them in CI and everything was
mysteriously failing. After some investigation, I realized there was no
`fetch` present in the node environment where things run on Github
Actions. I'm still somewhat baffled how this was working on my local
machine, in the node repl and in the sample apps, but not in this node
CI env.

To address this:
- I explicitly import `cross-fetch` and pass that fetch implementation
to the openapi client. Node doesn't natively have a `fetch` command.
`cross-fetch` ([npm](https://www.npmjs.com/package/cross-fetch)) is a
polyfill proxy that attempts to activate the correct fetch polyfill for
the current environment.
- Updated tests for the projectIdSingleton, which is the only thing I've
written in this client that used fetch directly. All other unit tests
are mocking things at the openapi object layer so had no direct
dependency on fetch.

Also:
- Cleanup and remove vestiges of the old integration test suite, which
hasn't been running for quite a while.

## Type of Change

- [x] New feature (non-breaking change which adds functionality)
- [x] Infrastructure change (CI configs, etc)

## Test Plan

CI should be green.
  • Loading branch information
jhamon authored Sep 13, 2023
1 parent 5db5b18 commit 3da50dc
Show file tree
Hide file tree
Showing 14 changed files with 206 additions and 421 deletions.
5 changes: 2 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,8 @@
"format": "prettier --write .",
"lint": "eslint src/ --ext .ts",
"repl": "npm run build && node utils/replInit.ts",
"test:legacy": "jest --runInBand --detectOpenHandles tests/legacy-integration/",
"test:integration": "jest tests/integration/ --setupFilesAfterEnv ./utils/globalIntegrationTestSetup.ts",
"test:unit": "jest src/ --setupFilesAfterEnv ./utils/globalUnitTestSetup.ts"
"test:integration": "jest src/integration --setupFilesAfterEnv ./utils/globalIntegrationTestSetup.ts",
"test:unit": "jest src/ --setupFilesAfterEnv ./utils/globalUnitTestSetup.ts --testPathIgnorePatterns src/integration"
},
"engines": {
"node": ">=14.0.0"
Expand Down
10 changes: 9 additions & 1 deletion src/__tests__/pinecone.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import { Pinecone } from '../pinecone';
import type { PineconeConfiguration } from '../data';
import crossFetch from 'cross-fetch';

jest.mock('cross-fetch', () => {
return {
__esModule: true,
default: jest.fn(),
};
});

jest.mock('../data/fetch');
jest.mock('../data/upsert');
Expand Down Expand Up @@ -96,7 +104,7 @@ describe('Pinecone', () => {
const client = new Pinecone();

expect(client.getConfig().projectId).toEqual('test-project');
expect(global.fetch).not.toHaveBeenCalled();
expect(crossFetch).not.toHaveBeenCalled();
});

test('config object should take precedence when both config object and environment variables are provided', () => {
Expand Down
36 changes: 20 additions & 16 deletions src/data/__tests__/projectIdSingleton.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
import { ProjectIdSingleton } from '../projectIdSingleton';
import crossFetch from 'cross-fetch';

jest.mock('cross-fetch', () => {
return {
__esModule: true,
default: jest.fn(),
};
});

describe('ProjectIdSingleton', () => {
afterEach(() => {
Expand All @@ -7,12 +15,10 @@ describe('ProjectIdSingleton', () => {

test('issues whoami requests for unknown projectId', async () => {
// @ts-ignore
global.fetch = jest.fn(() =>
Promise.resolve({
status: 200,
json: () => Promise.resolve({ project_name: 'abcdef' }),
})
);
crossFetch.mockResolvedValue({
status: 200,
json: () => Promise.resolve({ project_name: 'abcdef' }),
});

const testApiKey = 'api-key-1';
const testConfig = {
Expand All @@ -23,7 +29,7 @@ describe('ProjectIdSingleton', () => {
const id = await ProjectIdSingleton.getProjectId(testConfig);

expect(id).toEqual('abcdef');
expect(global.fetch).toHaveBeenCalledWith(
expect(crossFetch).toHaveBeenCalledWith(
'https://controller.gcp-free.pinecone.io/actions/whoami',
{
headers: {
Expand All @@ -38,12 +44,10 @@ describe('ProjectIdSingleton', () => {

test('only makes API call once per api key', async () => {
// @ts-ignore
global.fetch = jest.fn(() =>
Promise.resolve({
status: 200,
json: () => Promise.resolve({ project_name: 'xyzxyz' }),
})
);
crossFetch.mockResolvedValue({
status: 200,
json: () => Promise.resolve({ project_name: 'xyzxyz' }),
});

const testApiKey = 'api-key-2';
const testConfig2 = {
Expand All @@ -53,15 +57,15 @@ describe('ProjectIdSingleton', () => {

const id = await ProjectIdSingleton.getProjectId(testConfig2);
expect(id).toEqual('xyzxyz');
expect(global.fetch).toHaveBeenCalledTimes(1);
expect(crossFetch).toHaveBeenCalledTimes(1);

const id2 = await ProjectIdSingleton.getProjectId(testConfig2);
expect(id2).toEqual('xyzxyz');
expect(global.fetch).toHaveBeenCalledTimes(1);
expect(crossFetch).toHaveBeenCalledTimes(1);

await ProjectIdSingleton.getProjectId(testConfig2);
await ProjectIdSingleton.getProjectId(testConfig2);
await ProjectIdSingleton.getProjectId(testConfig2);
expect(global.fetch).toHaveBeenCalledTimes(1);
expect(crossFetch).toHaveBeenCalledTimes(1);
});
});
1 change: 1 addition & 0 deletions src/data/projectIdSingleton.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
} from '../errors';
import type { PineconeConfiguration } from './types';
import { buildUserAgent } from '../utils';
import fetch from 'cross-fetch';

// We only ever want to call whoami a maximum of once per API key, even if there
// are multiple instantiations of the Index class. So we use a singleton here
Expand Down
2 changes: 2 additions & 0 deletions src/data/vectorOperationsProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
} from '../pinecone-generated-ts-fetch';
import { queryParamsStringify, buildUserAgent } from '../utils';
import { ProjectIdSingleton } from './projectIdSingleton';
import fetch from 'cross-fetch';

const basePath = (config: PineconeConfiguration, indexName: string) =>
`https://${indexName}-${config.projectId}.svc.${config.environment}.pinecone.io`;
Expand Down Expand Up @@ -51,6 +52,7 @@ export class VectorOperationsProvider {
headers: {
'User-Agent': buildUserAgent(false),
},
fetchApi: fetch,
};

const indexConfiguration = new Configuration(indexConfigurationParameters);
Expand Down
2 changes: 1 addition & 1 deletion src/errors/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export class PineconeEnvironmentVarsNotSupportedError extends BasePineconeError
export class PineconeUnknownRequestFailure extends BasePineconeError {
constructor(url: string, underlyingError: Error) {
const message = `Something went wrong while attempting to call ${url}. Please check your configuration and try again later. Underlying error was ${JSON.stringify(
underlyingError
underlyingError.message
)}`;
super(message);
this.name = 'PineconeUnknownRequestFailure';
Expand Down
94 changes: 94 additions & 0 deletions src/integration/data/deleteMany.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import { Pinecone, Index } from '../../index';
import {
randomString,
createIndexIfDoesNotExist,
generateRecords,
} from '../test-helpers';

describe('deleteMany', () => {
const INDEX_NAME = 'ts-integration';
let pinecone: Pinecone, index: Index, ns: Index, namespace: string;

beforeAll(async () => {
pinecone = new Pinecone();

await createIndexIfDoesNotExist(pinecone, INDEX_NAME);

namespace = randomString(16);
index = pinecone.index(INDEX_NAME);
ns = index.namespace(namespace);
});

afterAll(async () => {
await pinecone.listIndexes();
await ns.deleteAll();
});

test('verify deleteMany with ids', async () => {
const recordsToUpsert = generateRecords(5, 3);
expect(recordsToUpsert).toHaveLength(3);
expect(recordsToUpsert[0].id).toEqual('0');
expect(recordsToUpsert[1].id).toEqual('1');
expect(recordsToUpsert[2].id).toEqual('2');

await ns.upsert(recordsToUpsert);

// Check records got upserted
let stats = await ns.describeIndexStats();
if (stats.namespaces) {
expect(stats.namespaces[namespace].recordCount).toEqual(3);
} else {
fail('Expected namespaces to be defined');
}

// Look more closely at one of the records to make sure values set
const fetchResult = await ns.fetch(['0']);
const records = fetchResult.records;
if (records) {
expect(records['0'].id).toEqual('0');
expect(records['0'].values.length).toEqual(5);
} else {
fail(
'Did not find expected records. Fetch result was ' +
JSON.stringify(fetchResult)
);
}

// Try deleting 2 of 3 vectors
await ns.deleteMany(['0', '2']);
stats = await ns.describeIndexStats();
if (stats.namespaces) {
expect(stats.namespaces[namespace].recordCount).toEqual(1);
} else {
fail(
'Expected namespaces to be defined (second call). Stats were ' +
JSON.stringify(stats)
);
}

// Check that record id='1' still exists
const fetchResult2 = await ns.fetch(['1']);
const records2 = fetchResult2.records;
if (records2) {
expect(records2['1']).not.toBeUndefined();
} else {
fail(
'Expected record 2 to be defined. Fetch result was ' +
JSON.stringify(fetchResult2)
);
}

// deleting non-existent indexes should not throw
await ns.deleteMany(['0', '1', '2', '3']);

// Verify all are now removed
stats = await ns.describeIndexStats();
if (stats.namespaces) {
expect(stats.namespaces[namespace]).toBeUndefined();
} else {
// no-op. This should actually happen unless there
// are leftover namespaces from previous runs that
// failed or stopped without proper cleanup.
}
});
});
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Pinecone } from '../../dist';
import { Pinecone } from '../index';

describe('Client initialization', () => {
test('can accept a config object', () => {
Expand Down
73 changes: 73 additions & 0 deletions src/integration/test-helpers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
import { Pinecone } from '../index';
import type { PineconeRecord, RecordSparseValues } from '../index';

export const randomString = (length) => {
const characters =
'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789';
let result = '';

for (let i = 0; i < length; i++) {
const randomIndex = Math.floor(Math.random() * characters.length);
result += characters.charAt(randomIndex);
}

return result;
};

export const createIndexIfDoesNotExist = async (
pinecone: Pinecone,
indexName: string
) => {
const indexList = await pinecone.listIndexes();
let found = false;
indexList.forEach((i) => {
if (i.name === indexName) {
found = true;
}
});
if (!found) {
await pinecone.createIndex({
name: indexName,
dimension: 5,
waitUntilReady: true,
});
}
};

export const generateRecords = (
dimension: number,
quantity: number,
withSparseValues?: boolean
): PineconeRecord[] => {
const vectors: PineconeRecord[] = [];
for (let i = 0; i < quantity; i++) {
const values: number[] = [];
for (let j = 0; j < dimension; j++) {
values.push(Math.random());
}
let vector: PineconeRecord = {
id: i.toString(),
values,
};
if (withSparseValues) {
vector = {
...vector,
sparseValues: generateSparseValues(dimension),
};
}

vectors.push(vector);
}
return vectors;
};

export const generateSparseValues = (dimension: number): RecordSparseValues => {
const values: number[] = [];
const indices: number[] = [];
for (let j = 0; j < dimension; j++) {
values.push(Math.random());
indices.push(j);
}
const sparseValues: RecordSparseValues = { indices, values };
return sparseValues;
};
2 changes: 2 additions & 0 deletions src/pinecone.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { Index, PineconeConfigurationSchema } from './data';
import { buildValidator } from './validator';
import { queryParamsStringify, buildUserAgent } from './utils';
import type { PineconeConfiguration, RecordMetadata } from './data';
import fetch from 'cross-fetch';

/**
* @example
Expand Down Expand Up @@ -62,6 +63,7 @@ export class Pinecone {
headers: {
'User-Agent': buildUserAgent(false),
},
fetchApi: fetch,
};
const api = new IndexOperationsApi(new ApiConfiguration(apiConfig));

Expand Down
Loading

0 comments on commit 3da50dc

Please sign in to comment.