Merge pull request #4686 from broadinstitute/improve-test-covergae

Improve test covergae

.github/workflows/unit-tests.yml

@@ -70,9 +70,10 @@ jobs:
           pip install -r requirements-dev.txt
       - name: Run coverage tests
         run: |
-          coverage run --source="./matchmaker","./seqr","./reference_data","./panelapp" --omit="*/migrations/*","*/apps.py" manage.py test -p '*_tests.py' -v 2 reference_data seqr matchmaker panelapp
+          coverage run --source="./matchmaker","./seqr","./reference_data","./panelapp" --omit="*/migrations/*","*/apps.py","*/admin.py" manage.py test -p '*_tests.py' -v 2 reference_data seqr matchmaker panelapp
           coverage report --fail-under=99
           coverage report | (! grep [1-8][0-9]%)
+          coverage report | (! grep 9[0-5]%)
 
   nodejs:
     runs-on: ubuntu-latest

seqr/management/commands/detect_inactive_privileged_users.py

@@ -8,15 +8,15 @@
 logger = logging.getLogger(__name__)
 
 WARNING_TEMPLATE = """
-Hi there {} - 
+Hi there {user} - 
 
-You have not logged in to seqr in {} days. Unless you log in within the next {} days, your account will be deactivated.
+You have not logged in to seqr in {days_login} days. Unless you log in within the next {deactivate_days_login} days, your account will be deactivated.
 """
 
 DEACTIVATE_TEMPLATE = """
-Hi there {} - 
+Hi there {user} - 
 
-You have not logged in to seqr in {} days, and therefore your account has been deactivated.
+You have not logged in to seqr in {days_login} days, and therefore your account has been deactivated.
 Please feel free to reach out to the seqr team if you would like your account reinstated.
 """
 
@@ -36,24 +36,24 @@ def handle(self, *args, **options):
 
         for user in warn_users:
             logger.info('Warning {} of impending account inactivation'.format(user.email))
-            days_login = (now - user.last_login.replace(tzinfo=None)).days
-            email_content = WARNING_TEMPLATE.format(user.get_full_name(), days_login, 90 - days_login)
-            try:
-                user.email_user('Warning: seqr account deactivation', email_content)
-            except AnymailError as e:
-                logger.error('Unable to send email: {}'.format(e))
+            self._safe_email_user(user, WARNING_TEMPLATE, 'deactivation')
 
         for user in deactivate_users:
             logger.info('Inactivating account for {}'.format(user.email))
             user.is_active = False
             user.save()
-
-            days_login = (now - user.last_login.replace(tzinfo=None)).days
-            email_content = DEACTIVATE_TEMPLATE.format(user.get_full_name(), days_login)
-            try:
-                user.email_user('Warning: seqr account deactivated', email_content)
-            except AnymailError as e:
-                logger.error('Unable to send email: {}'.format(e))
+            self._safe_email_user(user, DEACTIVATE_TEMPLATE, 'deactivated')
 
         logger.info('Inactive user check complete')
 
+    @staticmethod
+    def _safe_email_user(user, email_template, event):
+        days_login = (datetime.now() - user.last_login.replace(tzinfo=None)).days
+        email_content = email_template.format(
+            user=user.get_full_name(), days_login=days_login, deactivate_days_login=90 - days_login,
+        )
+        try:
+            user.email_user(f'Warning: seqr account {event}', email_content)
+        except AnymailError as e:
+            logger.error('Unable to send email: {}'.format(e))
+

seqr/management/tests/load_rna_seq_tests.py

@@ -65,6 +65,8 @@ def test_tpm(self, mock_utils_logger):
                 'NA19678_D1\t1kg project nåme with uniçøde\t\tENSG00000233750\t 6.04\twhole_blood\n',
                 'NA19677\t1kg project nåme with uniçøde\t\tENSG00000233750\t5.31\tmuscle\n',
                 'NA19678\tTest Reprocessed Project\t\tENSG00000240361\t0.2\twhole_blood\n',
+                'NA20870\tTest Reprocessed Project\t\tENSG00000240361\t0.2\twhole_blood\n',
+                'NA20870\tTest Reprocessed Project\t\tENSG00000240361\t0.7\twhole_blood\n',
             ],
             unmatched_samples='NA19677 (1kg project nåme with uniçøde), NA19678 (Test Reprocessed Project), NA19678_D1 (1kg project nåme with uniçøde)',
             additional_errors=['Samples missing required "tissue": NA19675_D2'],
@@ -97,6 +99,7 @@ def test_tpm(self, mock_utils_logger):
 
         self.mock_logger.info.assert_has_calls([
             mock.call('create 1 RnaSeqTpm for NA19678'),
+            mock.call('Error in T_NA20870: mismatched entries for ENSG00000240361'),
             mock.call('DONE'),
         ])
         mock_utils_logger.warning.assert_has_calls([
@@ -113,6 +116,7 @@ def test_tpm(self, mock_utils_logger):
         self.assertEqual(models.values('sample').distinct().count(), 2)
         self.mock_logger.info.assert_has_calls([
             mock.call('create 1 RnaSeqTpm for NA19678'),
+            mock.call('Error in T_NA20870: mismatched entries for ENSG00000240361'),
             mock.call('DONE'),
         ])
 

seqr/management/tests/transfer_families_to_different_project_tests.py

@@ -1,26 +1,24 @@
 import responses
 from django.core.management import call_command
-from django.test import TestCase
 import mock
 import json
 
 from seqr.models import Family, VariantTagType, VariantTag, Sample
-from seqr.views.utils.test_utils import AirflowTestCase
+from seqr.views.utils.test_utils import AirflowTestCase, AuthenticationTestCase
 
 
-class TransferFamiliesTest(TestCase):
-    fixtures = ['users', '1kg_project']
+class TransferFamiliesTest(object):
 
-    def _test_command(self, mock_logger, additional_family, logs):
+    def _test_command(self, additional_family, logs):
         call_command(
             'transfer_families_to_different_project', '--from-project=R0001_1kg', '--to-project=R0003_test', additional_family, '2',
         )
 
-        mock_logger.assert_has_calls([
+        self.assert_json_logs(user=None, expected=[
             *logs,
-            mock.call('Updating "Excluded" tags'),
-            mock.call('Updating families'),
-            mock.call('Done.'),
+            ('Updating "Excluded" tags', None),
+            ('Updating families', None),
+            ('Done.', None),
         ])
 
         family = Family.objects.get(family_id='2')
@@ -39,11 +37,14 @@ def _test_command(self, mock_logger, additional_family, logs):
 
         return family
 
-    @mock.patch('seqr.utils.search.elasticsearch.es_utils.ELASTICSEARCH_SERVICE_HOSTNAME', 'testhost')
-    @mock.patch('seqr.management.commands.transfer_families_to_different_project.logger.info')
-    def test_es_command(self, mock_logger):
+
+class TransferFamiliesLocalTest(TransferFamiliesTest, AuthenticationTestCase):
+    fixtures = ['users', '1kg_project']
+
+
+    def test_es_command(self):
         self._test_command(
-            mock_logger, additional_family='12', logs=[mock.call('Found 1 out of 2 families. No match for: 12.')]
+            additional_family='12', logs=[('Found 1 out of 2 families. No match for: 12.', None)]
         )
 
 
@@ -55,11 +56,11 @@ class TransferFamiliesAirflowTest(TransferFamiliesTest, AirflowTestCase):
     def setUp(self):
         super().setUp()
         self.set_up_one_dag(dataset_type='SNV_INDEL')
-        self.set_up_one_dag(dataset_type='SV')
+        self.set_up_one_dag(dataset_type='SV', status=400)
 
     def set_up_one_dag(self, **kwargs):
         dataset_type = kwargs.pop('dataset_type', 'MITO')
-        super().set_up_one_dag(dataset_type=dataset_type)
+        super().set_up_one_dag(dataset_type=dataset_type, **kwargs)
 
     def _get_dag_variables(self, dataset_type):
         return {
@@ -69,39 +70,44 @@ def _get_dag_variables(self, dataset_type):
             'dataset_type': dataset_type
         }
 
-    def _add_update_check_dag_responses(self, **kwargs):
+    def _add_update_check_dag_responses(self, status=200, **kwargs):
         # get variables
         responses.add(responses.GET, f'{self.MOCK_AIRFLOW_URL}/api/v1/variables/{self.DAG_NAME}', json={
             'key': self.DAG_NAME,
             'value': '{}'
-        })
+        }, status=status)
         # get variables again if the response of the previous request didn't include the updated variables
         responses.add(responses.GET, f'{self.MOCK_AIRFLOW_URL}/api/v1/variables/{self.DAG_NAME}', json={
             'key': self.DAG_NAME,
             'value': json.dumps(self._get_dag_variables(**kwargs))
-        })
+        }, status=status)
 
     def assert_airflow_delete_families_calls(self):
-        self._assert_call_counts(15)
+        self._assert_call_counts(13)
         call_count_per_dag = 5
-        for i, dataset_type in enumerate(['MITO', 'SNV_INDEL', 'SV']):
-            offset = i * call_count_per_dag
+        offset = 0
+        for dataset_type in ['MITO', 'SNV_INDEL']:
             self._assert_airflow_calls(self._get_dag_variables(dataset_type), call_count_per_dag, offset)
+            offset += call_count_per_dag
+
+        self._assert_update_variables_airflow_calls(self._get_dag_variables('SV'), offset)
 
     def _assert_update_check_airflow_calls(self, call_count, offset, update_check_path):
         variables_update_check_path = f'{self.MOCK_AIRFLOW_URL}/api/v1/variables/{self.DAG_NAME}'
         super()._assert_update_check_airflow_calls(call_count, offset, variables_update_check_path)
 
     @responses.activate
     @mock.patch('seqr.utils.search.elasticsearch.es_utils.ELASTICSEARCH_SERVICE_HOSTNAME', '')
-    @mock.patch('seqr.management.commands.transfer_families_to_different_project.logger.info')
-    def test_hail_backend_command(self, mock_logger):
-        searchable_family = self._test_command(mock_logger, additional_family='4', logs=[
-            mock.call('Found 2 out of 2 families.'),
-            mock.call('Disabled search for 7 samples in the following 1 families: 2'),
-            mock.call('Successfully triggered DELETE_FAMILIES DAG for 1 MITO families'),
-            mock.call('Successfully triggered DELETE_FAMILIES DAG for 1 SNV_INDEL families'),
-            mock.call('Successfully triggered DELETE_FAMILIES DAG for 1 SV families'),
+    def test_hail_backend_command(self):
+        searchable_family = self._test_command(additional_family='4', logs=[
+            ('Found 2 out of 2 families.', None),
+            ('Disabled search for 7 samples in the following 1 families: 2', None),
+            ('Successfully triggered DELETE_FAMILIES DAG for 1 MITO families', None),
+            ('Successfully triggered DELETE_FAMILIES DAG for 1 SNV_INDEL families', None),
+            ('400 Client Error: Bad Request for url: http://testairflowserver/api/v1/variables/DELETE_FAMILIES', {
+                'severity': 'ERROR',
+                '@type': 'type.googleapis.com/google.devtools.clouderrorreporting.v1beta1.ReportedErrorEvent',
+            })
         ])
 
         samples = Sample.objects.filter(individual__family=searchable_family)

seqr/utils/social_auth_pipeline.py

@@ -37,9 +37,3 @@ def log_signed_in(backend, response, is_new=False, *args, **kwargs):
     logger.info('Logged in {} ({})'.format(response['email'], backend.name), extra={'user_email': response['email']})
     if is_new:
         logger.info('Created user {} ({})'.format(response['email'], backend.name), extra={'user_email': response['email']})
-
-
-def log_azure_signed_in(backend, details, is_new=False, *args, **kwargs):
-    logger.info('Logged in {} ({})'.format(details['email'], backend.name), extra={'user_email': details['email']})
-    if is_new:
-        logger.info('Created user {} ({})'.format(details['email'], backend.name), extra={'user_email': details['email']})

seqr/utils/vcf_utils.py

@@ -91,7 +91,7 @@ def validate_vcf_and_get_samples(data_path, user, genome_version, path_name=None
         raise ErrorsWarningsException(['No samples found in the provided VCF.'], [])
     _validate_vcf_meta(meta, genome_version)
 
-    return sorted(samples)
+    return samples
 
 
 def _validate_vcf_exists(data_path, user, path_name, allowed_exts):

seqr/views/apis/data_manager_api_tests.py

@@ -470,6 +470,29 @@
         },
     ],
 }
+INVALID_AIRTABLE_SAMPLE_RECORDS = {
+    'records': [
+        {
+            'id': 'rec2B6OGmQpAkQW3s',
+            'fields': {
+                'SeqrProject': [
+                    'https://seqr.broadinstitute.org/project/R0002_empty/project_page',
+                    'https://seqr.broadinstitute.org/project/R0004_non_analyst_project/project_page',
+                ],
+                'PDOStatus': ['Historic', 'Methods (Loading)', 'Available in seqr'],
+                'CollaboratorSampleID': 'NA21234',
+            }
+        },
+        {
+            'id': 'recW24C2CJW5lT65K',
+            'fields': {
+                'CollaboratorSampleID': 'HG00731',
+                'SeqrProject': ['https://seqr.broadinstitute.org/project/R0001_1kg/details'],
+                'PDOStatus': ['Available in seqr'],
+            }
+        },
+    ],
+}
 
 PIPELINE_RUNNER_URL = 'http://pipeline-runner:6000/loading_pipeline_enqueue'
 
@@ -1523,12 +1546,17 @@ def test_get_loaded_projects(self):
         self.assertEqual(response.status_code, 200)
         self.assertDictEqual(response.json(), {'projects': self.WES_PROJECT_OPTIONS})
 
+        self._assert_expected_airtable_errors(url)
+
     def _assert_expected_pm_access(self, get_response):
         response = get_response()
         self.assertEqual(response.status_code, 200)
         self.login_data_manager_user()
         return response
 
+    def _assert_expected_airtable_errors(self, url):
+        return True
+
     @responses.activate
     @mock.patch('seqr.views.utils.airtable_utils.BASE_URL', 'https://seqr.broadinstitute.org/')
     @mock.patch('seqr.views.utils.export_utils.os.makedirs')
@@ -2038,3 +2066,14 @@ def _test_validate_dataset_type(self, url):
         self.assertListEqual(response.json()['errors'], [f'Data file or path {self.CALLSET_DIR}/mito_callset.mt is not found.'])
         self._set_file_not_found()
 
+    def _assert_expected_airtable_errors(self, url):
+        responses.replace(
+            responses.GET, 'https://api.airtable.com/v0/app3Y97xtbbaOopVR/Samples',
+            json=INVALID_AIRTABLE_SAMPLE_RECORDS, status=200,
+        )
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 400)
+        self.assertDictEqual(response.json(), {
+            'error': 'The following samples are associated with misconfigured PDOs in Airtable: HG00731, NA21234',
+        })
+

seqr/views/utils/export_utils.py

@@ -41,14 +41,6 @@ def export_table(filename_prefix, header, rows, file_format='tsv', titlecase_hea
         response.writelines(['\t'.join(header)+'\n'])
         response.writelines(('\t'.join(map(str, row))+'\n' for row in rows))
         return response
-    elif file_format == "json":
-        response = HttpResponse(content_type='application/json')
-        response['Content-Disposition'] = 'attachment; filename="{}.json"'.format(filename_prefix).encode('ascii', 'ignore')
-        for row in rows:
-            json_keys = [s.replace(" ", "_").lower() for s in header]
-            json_values = list(map(str, row))
-            response.write(json.dumps(OrderedDict(zip(json_keys, json_values)))+'\n')
-        return response
     elif file_format == "xls":
         wb = xl.Workbook(write_only=True)
         ws = wb.create_sheet()

seqr/views/utils/json_utils.py

@@ -8,7 +8,7 @@ class DjangoJSONEncoderWithSets(DjangoJSONEncoder):
 
     def default(self, o):
         if isinstance(o, set):
-            return list(o)
+            return sorted(o)
 
         return super(DjangoJSONEncoderWithSets, self).default(o)
 

settings.py

@@ -482,7 +482,7 @@
 elif SOCIAL_AUTH_AZUREAD_V2_TENANT_OAUTH2_KEY:
     SOCIAL_AUTH_PIPELINE = SOCIAL_AUTH_PIPELINE_BASE + \
                            SOCIAL_AUTH_PIPELINE_CLOUD_BASE + \
-                           ('seqr.utils.social_auth_pipeline.log_azure_signed_in',)
+                           ('seqr.utils.social_auth_pipeline.log_signed_in',)
 else:
     SOCIAL_AUTH_PIPELINE = SOCIAL_AUTH_PIPELINE_BASE + SOCIAL_AUTH_PIPELINE_USER_EXIST + \
                            SOCIAL_AUTH_PIPELINE_ASSOCIATE_USER + SOCIAL_AUTH_PIPELINE_LOG