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

compatibility Test coverage #1057

Open
wants to merge 18 commits into
base: mainline
Choose a base branch
from
Open

compatibility Test coverage #1057

wants to merge 18 commits into from

Conversation

adityabharadwaj198
Copy link
Member

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

  • feature

  • What is the current behavior? (You can also link to an open issue here)

  • Currently there are limited compatibility tests

  • What is the new behavior (if this is a feature change)?

  • more compatibility tests added as part of this PR

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

  • No

  • Have unit tests been run against this PR? (Has there also been any additional testing?)

  • Related Python client changes (link commit/PR here)

  • NA

  • Related documentation changes (link commit/PR here)

  • NA

  • Other information:

  • NA

  • Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added / updated (for bug fixes / features)

@adityabharadwaj198 adityabharadwaj198 marked this pull request as draft December 1, 2024 17:37
@adityabharadwaj198
Merge remote-tracking branch 'origin/aditya/comp-test-coverage' into …
fb07372 
…aditya/comp-test-coverage

# Conflicts:
#	tests/compatibility_tests/compatibility_test_runner.py
@adityabharadwaj198 adityabharadwaj198 marked this pull request as ready for review December 3, 2024 05:26
@adityabharadwaj198 adityabharadwaj198 changed the title Test coverage compatibility Test coverage Dec 9, 2024
farshidz
farshidz requested changes Dec 10, 2024
View reviewed changes
Copy link
Collaborator

@farshidz farshidz left a comment

Choose a reason for hiding this comment

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

There are a few problematic patterns. I'll do another pass once these have been addressed:

  • Unnecessary try/except (then raise) catching generic Exception class. These are superfluous
  • Different test cases in for loops, meaning if one test case fails, the others don't run
  • Evolution of an API post-2.0 ignored, specifically for structured index field types but possibly also others

tests/compatibility_tests/base_test_case/base_compatibility_test.py
cls.logger.debug(f"Results file deleted: {filepath}")
if filepath.exists():
filepath.unlink()
cls.logger.debug(f"Results file deleted: {filepath}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if debug log level is excessive here. Do you think we will ever run this in debug mode? Is there a risk if we logged these at info level (i.e. print them all the time)?

Copy link
Member Author

Choose a reason for hiding this comment

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

we can run it in debug mode if debugging locally. If these are logged at info level then it would make the logs too verbose and it'd be difficult to find valuable information

tests/compatibility_tests/add_or_replace_documents/test_add_documents.py Outdated
"indexName": structured_index_name,
"type": "structured",
"model": "sentence-transformers/all-MiniLM-L6-v2",
"normalizeEmbeddings": False,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Non-normalized indexes need to be covered, but are unusual. If covering one, we should cover normalized

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

tests/compatibility_tests/add_or_replace_documents/test_add_documents.py
}]
@classmethod
def tearDownClass(cls) -> None:
cls.indexes_to_delete = [index['indexName'] for index in cls.indexes_to_test_on]
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could also do this in setupclass, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am using setupClass in prepare mode to create Marqo client and do other basic setup. I don't want to delete any indexes in prepare mode, I only want to delete an index after it has been thoroughly tested which happens after Test mode run finishes, so I've added it in tearDownClass() which will directly be called after a test run (executed by Test mode) finishes

tests/compatibility_tests/add_or_replace_documents/test_add_documents.py
"model": "sentence-transformers/all-MiniLM-L6-v2",
"normalizeEmbeddings": False,
"allFields": [
{"name": "Title", "type": "text"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

You get the best coverage if you have all data types. That does mean you will need extra tests as some types were added in later versions. This comment applies to both index types.

tests/compatibility_tests/add_or_replace_documents/test_add_documents.py
try:
self.logger.debug(f'Feeding documents to {self.indexes_to_test_on}')
for index in self.indexes_to_test_on:
if index.get("type") is not None and index.get('type') == 'structured':
Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem with the way this is written is if the test fails for one index type, it doesn't run for the other. You need either subtests (if you can make that work here) or separate tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Subtests alone won't work here, because they just can be a wrapper above assertionErrors. Any other types of exceptions / errors and the subtest will fail and make the whole test fail. So I'm now using a try catch block + a subtest. Try catch block will be useful for any generic errors where as assertion errors can be logged by subtests

tests/compatibility_tests/add_or_replace_documents/test_add_documents.py Outdated
try:
all_results[index_name][doc_id] = self.client.index(index_name).get_document(doc_id)
except Exception as e:
self.logger.error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we fail here?

Copy link
Member Author

Choose a reason for hiding this comment

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

No point in failing a prepare mode test. Even if we fail here - when we run test mode we will not have the context of which prepare mode tests failed.

tests/compatibility_tests/add_or_replace_documents/test_add_documents.py Outdated Show resolved Hide resolved
tests/compatibility_tests/create_index/test_create_index.py Show resolved Hide resolved
tests/compatibility_tests/create_structured_index/test_create_structured_index.py
class TestCreateStructuredIndex(BaseCompatibilityTestCase):
indexes_settings_to_test_on = [
{
"type": "structured",
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are field types that 2.0 didn't support. You need to cover those

tests/compatibility_tests/create_structured_index/test_create_structured_index.py Outdated
self.logger.debug(f"Printing actual_settings {actual_settings}")
self.logger.debug(f"Printing expected_settings {expected_settings.get(index_name)}")
except Exception as e:
raise Exception(f"Exception when getting index settings for index {index_name}") from e
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit odd. We're not expecting an error here but if one does happen the test fails. I don't think you need try/except at all

Copy link
Member Author

Choose a reason for hiding this comment

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

try except is required so that even if one test fails the other can run. The issue here was that I was raising another exception anyway. I have changed that and now I let the other tests run before reporting exceptions

@adityabharadwaj198 adityabharadwaj198 temporarily deployed to marqo-build-environment December 17, 2024 14:02 — with GitHub Actions Inactive
@adityabharadwaj198 adityabharadwaj198 temporarily deployed to marqo-build-environment December 24, 2024 03:47 — with GitHub Actions Inactive
@adityabharadwaj198 adityabharadwaj198 temporarily deployed to marqo-build-environment December 24, 2024 04:06 — with GitHub Actions Inactive
@adityabharadwaj198 adityabharadwaj198 deployed to marqo-build-environment December 25, 2024 04:49 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@farshidz farshidz farshidz requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@adityabharadwaj198 @farshidz