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

Fix windows ci tests #2144

Merged
merged 6 commits into from Feb 9, 2022
Merged

Fix windows ci tests #2144

merged 6 commits into from Feb 9, 2022

Conversation

tstadel
Copy link
Member

@tstadel tstadel commented Feb 8, 2022

Proposed changes:

  • move command line options to global conftest.py
  • correct paths of excluded test files

Status (please check what you already did):

  • First draft (up for discussions & feedback)
  • Final code

@tstadel tstadel requested a review from ZanSara February 8, 2022 19:12
@@ -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

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 🙂

@tstadel tstadel merged commit 1bdd1f4 into master Feb 9, 2022
@tstadel tstadel deleted the fix_windows_ci_tests branch February 9, 2022 20:29
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.

None yet

2 participants