Skip to content

Conversation

@hanars
Copy link
Collaborator

@hanars hanars commented Oct 3, 2025

Lookup all rare variants in a gene across all projects

)

def test_gene_variant_lookup(self):
url = reverse(gene_variant_lookup)
Copy link
Collaborator Author

@hanars hanars Oct 6, 2025

Choose a reason for hiding this comment

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

The pattern we generallyhave for search tests is we have a test that tests the request and response but mocks the actual backend helper function, and then we have tests that call the helpers directly and test their behavior. This is not an ideal way to test things as its vulnerable to missed bugs if the function signature changes at all, and is also a bit hard to read/maintain. The reason we have this elsewhere is in order to test shared vs different behavior across different backends, but for this entirely new function thats only available in clickhouse it felt better to just actually test the request end-to-end without any mocking. Once elasticsearch is fully deprecated all the other tests in this file will eventually be converted to this pattern and combined with the other request tests

@hanars hanars marked this pull request as ready for review October 6, 2025 14:51
@hanars hanars requested a review from bpblanken October 6, 2025 14:52
@hanars hanars requested review from bpblanken and removed request for bpblanken October 16, 2025 13:46


class ClickhouseSearchTests(DifferentDbTransactionSupportMixin, SearchTestHelper, TestCase):
class ClickhouseSearchTests(SearchTestHelper, AnvilAuthenticationTestCase):
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 this will conflict with the changes I made to the transaction management here on the affected status. Will we need to split DifferentDbTransactionSupportMixin out from AnvilAuthenticationTestCase?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, I will refactor this so it can be integrated with your changes without conflict

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay I have refactored the test case to instead be a mixin that is usable here

@hanars hanars changed the base branch from dev to benb/connect_postgres October 16, 2025 21:25
@hanars hanars requested a review from bpblanken October 16, 2025 21:27
self.mock_get_group_members.assert_not_called()


class AnvilAuthenticationTestCase(DifferentDbTransactionSupportMixin, AnvilAuthenticationTestMixin, TestCase):
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Base automatically changed from benb/connect_postgres to dev October 22, 2025 15:04
@hanars hanars merged commit 57271a5 into dev Oct 22, 2025
4 checks passed
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.

2 participants