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.

Sign up for GitHub

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

Jump to bottom

Fix windows ci tests #2144

Merged
merged 6 commits into from
Feb 9, 2022
Merged

Fix windows ci tests #2144

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ee88d30
move commandline args to global conftest
tstadel Feb 8, 2022
30a2aec
correct test exclude paths
tstadel Feb 8, 2022
c7a71e0
Update Documentation & Code Style
github-actions[bot] Feb 8, 2022
960a38a
exclude test_generator_pipeline_with_translator from windows ci
tstadel Feb 9, 2022
a993e2f
exclude further oom tests
tstadel Feb 9, 2022
e72843d
enable log_cli
tstadel Feb 9, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/windows_ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,5 @@ jobs:
# As on windows we are going to disable quite a few tests these, hence these files will throw error refer https://github.com/pytest-dev/pytest/issues/812
# Removing test_ray, test_utils, test_preprocessor, test_knowledge_graph and test_connector
- name: Run tests
if: ${{ !contains(fromJSON('["test_ray.py", "test_knowledge_graph.py", "test_connector.py", "test_summarizer_translation.py"]'), matrix.test-path) }}
if: ${{ !contains(fromJSON('["./test/test_ray.py", "./test/test_knowledge_graph.py", "./test/test_connector.py", "./test/test_summarizer_translation.py", "./test/test_summarizer.py"]'), matrix.test-path) }}
run: pytest --document_store_type=memory,faiss,elasticsearch -m "not tika and not graphdb" -k "not test_parsr_converter" -s ${{ matrix.test-path }}
20 changes: 20 additions & 0 deletions conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
def pytest_addoption(parser):
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why this part of conftest.py needed to be moved to the root folder? rest_api and ui tests don't really use document store's parametrization and I'm sure pytest can deal with conftest.pys in subfolders...

Copy link
Member Author

@tstadel tstadel Feb 9, 2022 •
edited

Choose a reason for hiding this comment

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

Yes, unfortunately pytest doesn't. conftest.py only works with the package on the same level. So the --document_store_type option works perfectly with all tests within haystack's test folder. But in rest_api's and ui's test folder the addoption from conftest.py is not available and thus the cmd option is not available resulting in an error. In windows ci we always call pytest for any test file no matter where it lies with the document_store_type option.

By putting it into the package root, addoption is available in subpackages too.
Alternatives to that would be:

  • A) place a conftest.py with the same pytest_addoption into rest_api's and ui's test folder
  • B) do not call pytest with the document_store_type option for rest_api and ui tests in windows ci

A) is pretty stupid and introduces duplicate code that currently has no functionality inside the subpackages tests confusing anybody who takes a look at it
B) is rather clumsy as we have to check the relative file paths and besides that I think this option is intended to be global anyway

The con of the current approach of course is that there's a conftest.py floating around in the root folder.
I also found that issue where the same problem is addressed, but I couldn't find any other solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think for now it's the best solution, thanks for the explanation. When we're gonna refactor the tests we will try to find a cleaner way to filter document store tests that does not require that piece of code 🙂

parser.addoption("--document_store_type", action="store", default="elasticsearch, faiss, memory, milvus, weaviate")


def pytest_generate_tests(metafunc):
# Get selected docstores from CLI arg
document_store_type = metafunc.config.option.document_store_type
selected_doc_stores = [item.strip() for item in document_store_type.split(",")]

# parametrize document_store fixture if it's in the test function argument list
# but does not have an explicit parametrize annotation e.g
# @pytest.mark.parametrize("document_store", ["memory"], indirect=False)
found_mark_parametrize_document_store = False
for marker in metafunc.definition.iter_markers("parametrize"):
if "document_store" in marker.args[0]:
found_mark_parametrize_document_store = True
break
# for all others that don't have explicit parametrization, we add the ones from the CLI arg
if "document_store" in metafunc.fixturenames and not found_mark_parametrize_document_store:
metafunc.parametrize("document_store", selected_doc_stores, indirect=True)
22 changes: 0 additions & 22 deletions test/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,28 +64,6 @@
MOCK_DC = True


def pytest_addoption(parser):
parser.addoption("--document_store_type", action="store", default="elasticsearch, faiss, memory, milvus, weaviate")


def pytest_generate_tests(metafunc):
# Get selected docstores from CLI arg
document_store_type = metafunc.config.option.document_store_type
selected_doc_stores = [item.strip() for item in document_store_type.split(",")]

# parametrize document_store fixture if it's in the test function argument list
# but does not have an explicit parametrize annotation e.g
# @pytest.mark.parametrize("document_store", ["memory"], indirect=False)
found_mark_parametrize_document_store = False
for marker in metafunc.definition.iter_markers("parametrize"):
if "document_store" in marker.args[0]:
found_mark_parametrize_document_store = True
break
# for all others that don't have explicit parametrization, we add the ones from the CLI arg
if "document_store" in metafunc.fixturenames and not found_mark_parametrize_document_store:
metafunc.parametrize("document_store", selected_doc_stores, indirect=True)


def _sql_session_rollback(self, attr):
"""
Inject SQLDocumentStore at runtime to do a session rollback each time it is called. This allows to catch
Expand Down