populationgenomics / seqr

web-based analysis tool for rare disease genomics
GNU Affero General Public License v3.0
3 stars 1 forks source link

Upstream 2024 04 22 #199

Closed EddieLF closed 2 months ago

EddieLF commented 5 months ago

Pulls in new upstream changes from after 2024-01-15 (#198), up until 2024-04-22.

Merge conflicts came up in two main areas:

requirements.txt were rebuilt after pulling in changes with pip-compile requirements.in.


Some noteworthy changes from the changelog

EddieLF commented 2 months ago

@illusional I think I've got it down to one failing python unit test, in the dataset_api_tests.py where it's mocking an email. It mocks the email with an f-string containing the {SEQR_URL} global defined at the top of the file. For some reason the URL string is missing from the email content when the assert gets called. The process_message is also different.

I'm confused why this test is failing, where there are a bunch of other tests that make use of _assert_expected_notificationwhich aren't failing. Any ideas?

illusional commented 2 months ago

Nice @EddieLF! I imagine it comes from here: https://github.com/populationgenomics/seqr/blob/8731e2f16113bf7da189a60bd9e15b568235f3bb/seqr/utils/search/add_data_utils.py#L58 (which comes from) https://github.com/populationgenomics/seqr/blob/8731e2f16113bf7da189a60bd9e15b568235f3bb/settings.py#L312

Up to you if you want to modify the https://github.com/populationgenomics/seqr/blob/8731e2f16113bf7da189a60bd9e15b568235f3bb/seqr/views/apis/dataset_api_tests.py#L14 in the test to just be a /, or add an environment variable to the tests (I'd probably lean towards the former, because the testing environment is simpler, and tests the code, not the configuration).

Or mock the variable in the test to be what's listed above, is also totally fine too.

illusional commented 2 months ago

@EddieLF, just following up because I could test it locally, that function is only called within that one test (and it's failing on the first invocation), and it was added in this merge.

EddieLF commented 2 months ago

@illusional thanks for taking a look and suggesting a fix. I've added a @mock.patch for the BASE_URL in the add_data_utils to mock the existence of the environment variable. This has resolved the test failure, however overall the Python unit tests are still marked as failing with exit code 2. I took a look through the logs and nothing jumped out, all pytest modules report ok and cmd+f fail turns up nothing. There are some warnings about "DateTimeField %s received a naive datetime (%s)", but these don't indicate failure. Any idea what I might be missing? Cheers

illusional commented 2 months ago

It's failing because the codecoverage fell under 90% (in the unittest). If I were you, I'd just drop the minimum coverage (in the unit-test.yaml) to like 85%.