refactor: Pinecone tests #3555
refactor: Pinecone tests #3555
Conversation
There was a problem hiding this comment.
Looks very good to me! 👍 The only change I would request is to remove one unnecessary import of Label and Answer in test/document_stores/test_pinecone.py. It doesn't seem to be needed there if I am not mistaken. Otherwise it's all ready to merge.
Out of curiosity I would like to better understand the reason behind changing [""] to .get("") to prevent KeyErrors I assume in test_base.py 🙂
| @@ -318,7 +318,7 @@ def test_duplicate_documents_skip(self, ds, documents): | |||
|
|
|||
| ds.write_documents(updated_docs, duplicate_documents="skip") | |||
| for d in ds.get_all_documents(): | |||
| assert d.meta["name"] != "Updated" | |||
| assert d.meta.get("name") != "Updated" | |||
There was a problem hiding this comment.
I understand that this change ensure that no KeyError is raised if "name" is not part of d.meta and instead Null is returned. My question is if that was an issue before? The documents all have meta data with a name set, no?
There is nothing wrong about this change. I just would like to understand it a bit better. 🙂
There was a problem hiding this comment.
Pinecone tests have their own set of documents provided by the documents fixture, and some of them don't have any metadata attached. I think all the document stores should be able to pass tests on the same set of documents, but I didn't want to change too much functionality in this PR so I left pinecone tests with their custom set of docs, which requires this change.
| @pytest.mark.integration | ||
| def test_simplified_filters(self, ds, documents): | ||
| pass | ||
|
|
||
| # NOTE: Pinecone does not support dates, so it can't do lte or gte on date fields. When a new release introduces this feature, |
There was a problem hiding this comment.
I like that we only have this comment once now. 👍
masci
force-pushed
the
massi/pinecone
branch
from
0b01bc7 to
e85ac1f
Compare
masci
force-pushed
the
massi/pinecone
branch
from
e85ac1f to
b1606cb
Compare
Related Issues
Proposed Changes:
DocumentStoreBaseTestAbstractHow did you test it?
pytest -m"document_store and integration" test/document_stores/test_pinecone.pyNotes for the reviewer
Tests run on Linux, Win and MacOS because the storage is mocked. Mock can be disabled passing
--mock-pinecone=falseso the test suite is still to be considered of kind "integration".Checklist