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

test: Update test/others/test_utils.py #5270

Merged
merged 9 commits into from
Jul 5, 2023
Merged

test: Update test/others/test_utils.py #5270

merged 9 commits into from
Jul 5, 2023

Conversation

sjrl
Copy link
Contributor

@sjrl sjrl commented Jul 4, 2023

Related Issues

  • fixes N/A

Proposed Changes:

  • Remove deepset Cloud specific tests
  • Add mark unit test to appropriate tests
  • Convert global variables to pytest fixtures

Notes for the reviewer

Checklist

@sjrl sjrl requested a review from a team as a code owner July 4, 2023 12:09
@sjrl sjrl requested review from bogdankostic and silvanocerza and removed request for a team and bogdankostic July 4, 2023 12:09
@sjrl
Copy link
Contributor Author

sjrl commented Jul 4, 2023

Hey @bogdankostic, @silvanocerza and I were talking about these changes offline so he offered to review this PR

@coveralls
Copy link
Collaborator

coveralls commented Jul 4, 2023

Pull Request Test Coverage Report for Build 5462544132

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 83 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.7%) to 44.607%

Files with Coverage Reduction New Missed Lines %
document_stores/search_engine.py 4 62.39%
document_stores/elasticsearch/es8.py 79 0%
Totals Coverage Status
Change from base Build 5453787062: 0.7%
Covered Lines: 10265
Relevant Lines: 23012

💛 - Coveralls

@sjrl
Copy link
Contributor Author

sjrl commented Jul 4, 2023

Hey @silvanocerza before we merge, I'd like to double check with @tstadel that is okay for us to remove the dC tests or if it would be helpful to have them present until they can be moved (e.g. maybe into the new SDK).

@sjrl sjrl requested a review from tstadel July 4, 2023 13:36
@silvanocerza
Copy link
Contributor

Ok for me even though I think it's not necessary. They weren't running and they will still be in the history if necessary. In any case I believe they must not be part of Haystack.

@tstadel
Copy link
Member

tstadel commented Jul 4, 2023

I would highly recommend to keep tests (running) until the corresponding code has been removed. There is still functionality in the old SDK that is not yet part of the new SDK. Unless there is a good reason, we should stick to our usual deprecation process (for code and its corresponding tests).

@sjrl sjrl merged commit da2c9b4 into main Jul 5, 2023
@sjrl sjrl deleted the others-test-utils branch July 5, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants