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

Improve document stores unit test parametrization #1202

Merged
merged 5 commits into from
Jun 22, 2021
Merged

Improve document stores unit test parametrization #1202

merged 5 commits into from
Jun 22, 2021

Conversation

vblagoje
Copy link
Member

@vblagoje vblagoje commented Jun 16, 2021

Proposed changes:

Parametrizes document store tests allowing developers to select a subset of document stores in the generic document store related tests.

This is just a preview and request for feedback. Only test/test_retriever.py tests have been updated for this preview. Updating other tests is trivial given this approach is approved. All relevant generic document store tests have been updated.

To run tests for all document stores, an explicit parametrize annotation marker with a list of all document stores needs to be removed from a unit test function (see diff). However, we want to leave a particular explicit parametrize annotation in some test cases as the test might test a particular functionality for a specific document store.

To try out an updated test approach, we'll focus on test_dpr_embedding test in test_retriever.py

To run tests for all document stores, no changes in pytest command invocation are needed. That remains a default option.
To run a particular test or a subset of tests with a desired document store type a developer needs to add a command-line parameter. For example, to run test for faiss only one should invoke this command:

pytest -v test_retriever.py::test_dpr_embedding --document_store_type="faiss" 

One can also run a test on a subset of document stores:

pytest -v test_retriever.py::test_dpr_embedding --document_store_type="faiss, memory" 

Resolves #1108
Looking forward to your feedback.

Status (please check what you already did):

  • First draft (up for discussions & feedback)
  • Final code
  • Added tests
  • Updated documentation

@vblagoje
Copy link
Member Author

Ok, given these CI failures, perhaps it's not as easy to expand the document store parametrization to the entire test suite. Investigating...

@vblagoje
Copy link
Member Author

Ok this works now, really nice! It actually caught some weaviate failures 😆

@vblagoje
Copy link
Member Author

This PR should be ready to go now @oryx1729 @tholor . Document store tests for elasticsearch, faiss, memory, and milvus have been updated. IMHO this approach could be expanded to other fixtures that have multiple implementation. It reduces test code complexity and improves developer experience as well as long-term test code management.

@tholor tholor requested a review from tanaysoni June 18, 2021 10:42
@vblagoje
Copy link
Member Author

Hey @tanaysoni how does this one look to you?

@oryx1729
Copy link
Contributor

Thank you for working on this, @vblagoje! Left a small comment but otherwise looks good to go 🚀

@oryx1729 oryx1729 merged commit 02fc4c7 into deepset-ai:master Jun 22, 2021
@vblagoje vblagoje deleted the parametrize_doc_stores branch February 28, 2023 12:08
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.

Improve document stores unit test parametrization
2 participants