feat: add SQLDocumentStore tests #3517
Conversation
| @@ -141,8 +141,6 @@ jobs: | |||
| ES_JAVA_OPTS: "-Xms128m -Xmx256m" | |||
| ports: | |||
| - 9200:9200 | |||
| # env: | |||
| # ELASTICSEARCH_HOST: "elasticsearch" | |||
There was a problem hiding this comment.
leftover from previous PR
| @@ -179,8 +205,6 @@ jobs: | |||
| ES_JAVA_OPTS: "-Xms128m -Xmx256m" | |||
| ports: | |||
| - 9200:9200 | |||
| # env: | |||
| # OPENSEARCH_HOST: "opensearch" | |||
There was a problem hiding this comment.
leftover from previous PR
| @@ -142,27 +142,41 @@ def test_get_all_documents_with_incorrect_filter_value(self, ds, documents): | |||
| assert len(result) == 0 | |||
|
|
|||
| @pytest.mark.integration | |||
| def test_extended_filter(self, ds, documents): | |||
There was a problem hiding this comment.
This function was too big in any case, it was ported as it was in the first step of the refactoring but breaking it down was functional to this PR as the docstore doesn't fully support metadata and some tests are expected not to pass
| @@ -172,21 +172,6 @@ def test_get_all_documents_with_correct_filters(document_store_with_docs): | |||
| assert {d.meta["meta_field"] for d in documents} == {"test1", "test3"} | |||
|
|
|||
|
|
|||
| def test_get_all_documents_with_correct_filters_legacy_sqlite(docs, tmp_path): | |||
There was a problem hiding this comment.
This was redundant at this point
| @@ -410,47 +395,6 @@ def test_write_document_meta(document_store: BaseDocumentStore): | |||
| assert document_store.get_document_by_id("4").meta["meta_field"] == "test4" | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("document_store", ["sql"], indirect=True) | |||
| def test_sql_write_document_invalid_meta(document_store: BaseDocumentStore): | |||
There was a problem hiding this comment.
these two tests were ported to the new class
masci
changed the title
There was a problem hiding this comment.
A great step forward for SQLDocumentStore 🚀
A question: do you think we can afford, in the future, to run these tests on another SQL database, like PostgreSQL? I suppose subclassing this suite and giving another URL to the constructor should suffice, but I wonder if it's worth taking the time on the CI for this. It will also require a PostgreSQL container. Maybe worth a TODO?
Co-authored-by: Sara Zan <sara.zanzottera@deepset.ai>
Co-authored-by: bogdankostic <bogdankostic@web.de>
Related Issues
Proposed Changes:
Run the
DocumentStoreBaseTestAbstractwithSQLDocumentStoreHow did you test it?
Tests are running in the CI
Notes for the reviewer
The
SQLDocumentStoreis buggy and partially implemented in several parts, so a bunch of the base tests were failing. I have skipped the failing tests in this PR as filling the feature gap would require quite an implementation effort.Checklist