Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integration tests for collections #299

Merged
merged 4 commits into from Jan 24, 2024
Merged

Integration tests for collections #299

merged 4 commits into from Jan 24, 2024

Conversation

jhamon
Copy link
Collaborator

@jhamon jhamon commented Jan 23, 2024

Problem

We received a bug report that creation of indexes using PodSpec fails if source_collection is specified.

Solution

  • The fix for the bug was a one-line change.
  • Added several integration tests to exercise index --> collection --> index path and error cases.
  • Restructured integration tests so that tests using pod-based indexes reside in tests/integration/control/pod and can be run separately from severless indexes tested in tests/integration/control/serverless. This allows for greater parallelism in CI.
  • Adjusted CI configs to run these tests in parallel to integration tests using serverless indexes. The collections tests are quite slow due to the waiting required for pod indexes and collections to become ready for use.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • Infrastructure change (CI configs, etc)

@jhamon jhamon changed the title WIP Collection testing Jan 23, 2024
@jhamon jhamon changed the title Collection testing Integration tests for collections Jan 24, 2024
)
assert 'Index and collection must have the same dimension' in str(e.value)

# def test_create_index_from_notready_collection(self, client, ready_index, random_vector, dimension, metric, environment):
Copy link
Collaborator Author

@jhamon jhamon Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this scenario. Need to follow up later. I expected it to fail but it seems to succeed?

Comment on lines +63 to +67
source_collection: Optional[str] = None
"""
The name of the collection to use as the source for the pod index. This configuration is only used when creating a pod index from an existing collection.
"""

Copy link
Collaborator Author

@jhamon jhamon Jan 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This missing property was the cause of create_index with source_collection failing.

@jhamon jhamon marked this pull request as ready for review January 24, 2024 11:28
Copy link
Contributor

@austin-denoble austin-denoble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work continuing to expand our testing setup. Some nits and questions, but this looks good to go. 🚢

run: poetry run pytest tests/integration/control/serverless -s -v
env:
PINECONE_DEBUG_CURL: 'true'
PINECONE_CONTROLLER_HOST: 'https://api.pinecone.io'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We could probably leave this out for the standard non-staging run, similar to down in control-rest-serverless. Small complaint, more for consistencies sake.

pass

if time_waited >= 120:
raise Exception(f"Index {index_name} is not ready to delete after 120 seconds")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Wording here could be: "Index {index_name} was not able to be deleted after 120 seconds"


@pytest.fixture()
def create_index_params(index_name, environment, dimension, metric):
spec = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I thought you had to use the PodSpec class for spec rather than passing a plain object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The python client is flexible enough to accept both.

Comment on lines +6 to +7
time.sleep(10) # Wait a little more, just in case.
client.configure_index(ready_index, replicas=1, pod_type='p1.x1')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need some asserts here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe someday. To start with, I just wanted to see if the request can be made without errors and this was enough to catch the bug we saw ahead of the release last week. Asserting what the server actually does as a result can come later, or possibly be covered in tests owned by platform.

@jhamon jhamon merged commit 995c0a1 into main Jan 24, 2024
66 checks passed
@jhamon jhamon deleted the jhamon/collection-testing branch January 24, 2024 19:33
@jhamon jhamon restored the jhamon/collection-testing branch February 5, 2024 20:53
@jhamon jhamon deleted the jhamon/collection-testing branch February 5, 2024 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants