Skip to content

Conversation

@hemidactylus
Copy link
Collaborator

This PR completely restructures the integration tests for this project.

This was a much-needed improvement: the test suite had grown in a scarcely-controlled way, becoming bloated, very long-lasting and unstable (mostly due to the unnecessarily high number of collections being created and dropped during testing). Additionally, some of the tests were using obsolete constructs and duplicated assets.

The gist

This PR implements an extremely parsimonious usage of collections thanks to a hierarchy of fixtures (and ample usage of setup_mode). Whenever possible, collections are simply truncated and re-used between tests. Some collections, which remain unavoidably per-test-function, are marked as such ("ephemeral..."). This is the case mostly for "DDL-related" tests, for instance when testing deprecations and errors due to indexing mismatches and such. For the case of vector-store tests, these are grouped in a separate module to skip them easily while developing.

Numbers

The general idea is that there should never be more than 3 collections in the database, at any given time. (nevertheless, the collection names involved are way more, to avoid running into metadata-cache issues when dropping and re-creating them.)

Before this PR, the whole suite took about 40+ minutes to run (... and often fail). Now it takes 13 minutes. Specifically, the vector store tests alone went from 19 minutes down to about 7.

Additional notes

  • The setup_mode is tested scarcely in itself (perhaps the only "loss of coverage" of this PR); however, it is implicitly tested essentially in all collection-related fixtures.
  • Care has been taken to ensure that the test suite can run smoothly on DSE/HCD (with the obvious exception of the "core clients warning" tests which are skipped in that case).
  • The Graph Vector Store tests, in the autodetect (flat-document!) case, are made to use specific collections, to ensure that the metadata keys encoding the graph structure are indexed. This is a corner case to keep in mind also outside of testing: in actual usage, when promoting an "autodetect(flat) vector store" to a graph store, it is responsibility of the user to ensure that the collection's indexing settings allow for such promotion.
  • the test module for the two cache objects (regular and semantic) have been split in two modules.


@pytest.fixture
def vector_store_d2(
empty_collection_d2: Collection, # noqa: ARG001
Copy link
Collaborator

@cbornet cbornet Sep 27, 2024

Choose a reason for hiding this comment

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

NIT: Is it possible to use @pytest.mark.usefixtures("empty_collection_d2") and get rid of the noqa ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch - filing for a later improvement since it's merged now (saw the comment only now, sorry)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I merged since it was NIT.
I'm doing a PR rn.

@cbornet cbornet self-requested a review September 27, 2024 08:22
@cbornet cbornet merged commit 3eb73d7 into main Sep 27, 2024
13 checks passed
@cbornet cbornet deleted the SL-optimize-ci branch September 27, 2024 08:23
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.

3 participants