Skip to content

Rollback search fields to list and dynamically compute md5 hash in tests #311

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

Merged
merged 19 commits into from
Sep 5, 2024

Conversation

mskarlin
Copy link
Collaborator

@mskarlin mskarlin commented Aug 30, 2024

@sidnarayanan found a couple of issues while testing:

  • seems that the md5 filehash isn't resolving correctly in local tests -- I replaced the hard coded md5 hash with one that is dynamically calculated to remove ambiguity in the tests.
  • @sidnarayanan rolled back the type of a SearchIndex.fields attribute from a set back to a list to align with tantivy's expected type.
  • added GHA wildcard to run tests for release branch PRs as well

@@ -81,12 +81,11 @@ def read_from_string(self, data: str | bytes) -> BaseModel | SupportsPickle:


class SearchIndex:

REQUIRED_FIELDS: ClassVar[set[str]] = {"file_location", "body"}
REQUIRED_FIELDS: ClassVar[list[str]] = ["file_location", "body"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

set is still a Sequence, let's keep this as a set to enforce uniqueness

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see what you're saying but set isn't a Sequence, it's a Collection, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think a Collection is a subset of Sequence, unless I am missing something

Copy link
Contributor

Choose a reason for hiding this comment

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

… and keep consistent hashes; aded env vars to test.yaml
@mskarlin mskarlin merged commit 8088379 into september-2024-release Sep 5, 2024
1 check passed
@jamesbraza jamesbraza deleted the test-md5 branch September 10, 2024 16:00
@whitead whitead mentioned this pull request Sep 11, 2024
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.

3 participants