Merge pull request #887 from GSA/notify-admin-866b

Improve tests with #noqa comments

tests/app/main/test_asset_fingerprinter.py

@@ -18,11 +18,11 @@ class TestAssetFingerprint(object):
         asset_fingerprinter = AssetFingerprinter(asset_root="/suppliers/static/")
         assert (
             asset_fingerprinter.get_url("application.css")
-            == "/suppliers/static/application.css?418e6f4a6cdf1142e45c072ed3e1c90a"  # noqa
+            == "/suppliers/static/application.css?418e6f4a6cdf1142e45c072ed3e1c90a"
         )
         assert (
             asset_fingerprinter.get_url("application-ie6.css")
-            == "/suppliers/static/application-ie6.css?418e6f4a6cdf1142e45c072ed3e1c90a"  # noqa
+            == "/suppliers/static/application-ie6.css?418e6f4a6cdf1142e45c072ed3e1c90a"
         )
 
     def test_building_file_path(self, mocker):

tests/app/main/views/service_settings/test_service_setting_permissions.py

@@ -190,12 +190,10 @@ def test_service_setting_link_toggles_index_error(
     index,
     text,
 ):
-    with pytest.raises(  # noqa: PT012  # Requires more research to refactor.
+    url_for(endpoint, service_id=service_one["id"])
-        expected_exception=IndexError
+    service_one.update(service_fields)
-    ):
+    page = get_service_settings_page()
-        url_for(endpoint, service_id=service_one["id"])
+    with pytest.raises(expected_exception=IndexError):
-        service_one.update(service_fields)
-        page = get_service_settings_page()
         page.select(".page-footer-link a")[index]
 
 

tests/app/main/views/service_settings/test_service_settings.py

@@ -744,13 +744,11 @@ def test_should_check_for_reply_to_on_go_live_index_error(
             return_value=volume,
         )
 
-    with pytest.raises(  # noqa: PT012  # This will require more research for refactoring.
+    page = client_request.get("main.request_to_go_live", service_id=SERVICE_ONE_ID)
-        expected_exception=IndexError
+    assert page.h1.text == "Before you request to go live"
-    ):
+    checklist_items = page.select(".task-list .task-list-item")
-        page = client_request.get("main.request_to_go_live", service_id=SERVICE_ONE_ID)
-        assert page.h1.text == "Before you request to go live"
 
-        checklist_items = page.select(".task-list .task-list-item")
+    with pytest.raises(expected_exception=IndexError):
         assert (
             normalize_spaces(checklist_items[3].text)
             == expected_reply_to_checklist_item
@@ -1030,19 +1028,24 @@ def test_should_check_for_sms_sender_on_go_live(
             return_value=volume,
         )
 
-    with pytest.raises(  # noqa: PT012  # Requires more research for how to refactor.
+    with pytest.raises(expected_exception=IndexError):
-        expected_exception=IndexError
+        simple_statement_for_test_should_check_for_sms_sender_on_go_live(
-    ):
+            client_request, expected_sms_sender_checklist_item, mock_get_sms_senders
-        page = client_request.get("main.request_to_go_live", service_id=SERVICE_ONE_ID)
-        assert page.h1.text == "Before you request to go live"
-
-        checklist_items = page.select(".task-list .task-list-item")
-        assert (
-            normalize_spaces(checklist_items[3].text)
-            == expected_sms_sender_checklist_item
         )
 
-        mock_get_sms_senders.assert_called_once_with(SERVICE_ONE_ID)
+
+def simple_statement_for_test_should_check_for_sms_sender_on_go_live(
+    client_request, expected_sms_sender_checklist_item, mock_get_sms_senders
+):
+    page = client_request.get("main.request_to_go_live", service_id=SERVICE_ONE_ID)
+    assert page.h1.text == "Before you request to go live"
+    checklist_items = page.select(".task-list .task-list-item")
+
+    assert (
+        normalize_spaces(checklist_items[3].text) == expected_sms_sender_checklist_item
+    )
+
+    mock_get_sms_senders.assert_called_once_with(SERVICE_ONE_ID)
 
 
 def test_non_gov_user_is_told_they_cant_go_live(
@@ -3145,10 +3148,8 @@ def test_should_set_sms_allowance_fails(
     mock_get_free_sms_fragment_limit,
     mock_create_or_update_free_sms_fragment_limit,
 ):
-    with pytest.raises(  # noqa: PT012  # Needs more research for refactoring.
+    client_request.login(platform_admin_user)
-        expected_exception=AssertionError
+    with pytest.raises(expected_exception=AssertionError):
-    ):
-        client_request.login(platform_admin_user)
         client_request.post(
             "main.set_free_sms_allowance",
             service_id=SERVICE_ONE_ID,
@@ -3460,10 +3461,8 @@ def test_archive_service_after_confirm_error(
     mocker.patch("app.notify_client.service_api_client.redis_client.delete")
     mocker.patch("app.notify_client.service_api_client.redis_client.delete_by_pattern")
 
-    with pytest.raises(  # noqa: PT012  # Needs more research for refactoring.
+    client_request.login(user)
-        expected_exception=AssertionError
+    with pytest.raises(expected_exception=AssertionError):
-    ):
-        client_request.login(user)
         client_request.post(
             "main.archive_service",
             service_id=SERVICE_ONE_ID,
@@ -3596,10 +3595,8 @@ def test_suspend_service_after_confirm_error(
 ):
     mocker.patch("app.service_api_client.post")
     mocker.patch("app.main.views.service_settings.create_suspend_service_event")
-    with pytest.raises(  # noqa: PT012  # Needs more research for refactoring.
+    client_request.login(user)
-        expected_exception=AssertionError
+    with pytest.raises(expected_exception=AssertionError):
-    ):
-        client_request.login(user)
         client_request.post(
             "main.suspend_service",
             service_id=SERVICE_ONE_ID,

tests/app/main/views/test_headers.py

@@ -16,7 +16,8 @@ def test_owasp_useful_headers_set(
     assert search(r"frame-ancestors 'none';", csp)
     assert search(r"form-action 'self';", csp)
     assert search(
-        r"script-src 'self' static\.example\.com 'unsafe-eval' https:\/\/js-agent\.newrelic\.com https:\/\/gov-bam\.nr-data\.net 'nonce-.*';",  # noqa e501
+        r"script-src 'self' static\.example\.com 'unsafe-eval' https:\/\/js-agent\.new"
+        r"relic\.com https:\/\/gov-bam\.nr-data\.net 'nonce-.*';",
         csp,
     )
     assert search(r"connect-src 'self' https:\/\/gov-bam.nr-data\.net;", csp)

tests/app/main/views/test_jobs.py

@@ -238,7 +238,8 @@ def test_should_show_job_with_sending_limit_exceeded_status(
     )
 
     assert normalize_spaces(page.select("main p")[1].text) == (
-        "Notify cannot send these messages because you have reached a limit. You can only send 1,000 messages per day and 250,000 messages in total."  # noqa
+        "Notify cannot send these messages because you have reached a limit. "
+        "You can only send 1,000 messages per day and 250,000 messages in total."
     )
     assert normalize_spaces(page.select("main p")[2].text) == (
         "Upload this spreadsheet again tomorrow or contact the Notify.gov team to raise the limit."

tests/app/main/views/test_manage_users.py

@@ -20,8 +20,8 @@ from tests.conftest import (
 )
 
 
-@pytest.mark.parametrize(  # noqa: PT014  # Duplicate parameters have different permissions.
+@pytest.mark.parametrize(
-    ("user", "expected_self_text", "expected_coworker_text"),
+    ("user", "expected_self_text", "add_details"),
     [
         (
             create_active_user_with_permissions(),
@@ -34,16 +34,7 @@ from tests.conftest import (
                 "Can Manage settings, team and usage "
                 "Can Manage API integration"
             ),
-            (
+            True,
-                "ZZZZZZZZ zzzzzzz@example.gsa.gov "
-                "Permissions "
-                "Can See dashboard "
-                "Cannot Send messages "
-                "Cannot Add and edit templates "
-                "Cannot Manage settings, team and usage "
-                "Cannot Manage API integration "
-                "Change details for ZZZZZZZZ zzzzzzz@example.gsa.gov"
-            ),
         ),
         (
             create_active_user_empty_permissions(),
@@ -56,15 +47,7 @@ from tests.conftest import (
                 "Cannot Manage settings, team and usage "
                 "Cannot Manage API integration"
             ),
-            (
+            False,
-                "ZZZZZZZZ zzzzzzz@example.gsa.gov "
-                "Permissions "
-                "Can See dashboard "
-                "Cannot Send messages "
-                "Cannot Add and edit templates "
-                "Cannot Manage settings, team and usage "
-                "Cannot Manage API integration"
-            ),
         ),
         (
             create_active_user_view_permissions(),
@@ -77,15 +60,7 @@ from tests.conftest import (
                 "Cannot Manage settings, team and usage "
                 "Cannot Manage API integration"
             ),
-            (
+            False,
-                "ZZZZZZZZ zzzzzzz@example.gsa.gov "
-                "Permissions "
-                "Can See dashboard "
-                "Cannot Send messages "
-                "Cannot Add and edit templates "
-                "Cannot Manage settings, team and usage "
-                "Cannot Manage API integration"
-            ),
         ),
         (
             create_active_user_manage_template_permissions(),
@@ -98,36 +73,7 @@ from tests.conftest import (
                 "Cannot Manage settings, team and usage "
                 "Cannot Manage API integration"
             ),
-            (
+            False,
-                "ZZZZZZZZ zzzzzzz@example.gsa.gov "
-                "Permissions "
-                "Can See dashboard "
-                "Cannot Send messages "
-                "Cannot Add and edit templates "
-                "Cannot Manage settings, team and usage "
-                "Cannot Manage API integration"
-            ),
-        ),
-        (
-            create_active_user_manage_template_permissions(),
-            (
-                "Test User With Permissions (you) "
-                "Permissions "
-                "Can See dashboard "
-                "Cannot Send messages "
-                "Can Add and edit templates "
-                "Cannot Manage settings, team and usage "
-                "Cannot Manage API integration"
-            ),
-            (
-                "ZZZZZZZZ zzzzzzz@example.gsa.gov "
-                "Permissions "
-                "Can See dashboard "
-                "Cannot Send messages "
-                "Cannot Add and edit templates "
-                "Cannot Manage settings, team and usage "
-                "Cannot Manage API integration"
-            ),
         ),
     ],
 )
@@ -140,8 +86,8 @@ def test_should_show_overview_page(
     service_one,
     user,
     expected_self_text,
-    expected_coworker_text,
     active_user_view_permissions,
+    add_details,
 ):
     current_user = user
     other_user = copy.deepcopy(active_user_view_permissions)
@@ -164,11 +110,20 @@ def test_should_show_overview_page(
     assert (
         normalize_spaces(page.select(".user-list-item")[0].text) == expected_self_text
     )
-    # [1:5] are invited users
+
-    assert (
+    expected = (
-        normalize_spaces(page.select(".user-list-item")[6].text)
+        "ZZZZZZZZ zzzzzzz@example.gsa.gov "
-        == expected_coworker_text
+        "Permissions "
+        "Can See dashboard "
+        "Cannot Send messages "
+        "Cannot Add and edit templates "
+        "Cannot Manage settings, team and usage "
+        "Cannot Manage API integration"
     )
+
+    if add_details is True:
+        expected = f"{expected} Change details for ZZZZZZZZ zzzzzzz@example.gsa.gov"
+    assert normalize_spaces(page.select(".user-list-item")[6].text) == expected
     mock_get_users.assert_called_once_with(SERVICE_ONE_ID)
 
 

tests/app/main/views/test_platform_admin.py

@@ -770,10 +770,12 @@ def test_clear_cache_shows_form(
             [
                 call("service-????????-????-????-????-????????????-templates"),
                 call(
-                    "service-????????-????-????-????-????????????-template-????????-????-????-????-????????????-version-*"  # noqa
+                    "service-????????-????-????-????-????????????-template"
+                    "-????????-????-????-????-????????????-version-*"
                 ),
                 call(
-                    "service-????????-????-????-????-????????????-template-????????-????-????-????-????????????-versions"  # noqa
+                    "service-????????-????-????-????-????????????-template"
+                    "-????????-????-????-????-????????????-versions"
                 ),
             ],
             "Removed 6 objects across 3 key formats for template",

tests/app/main/views/test_send.py

@@ -2319,25 +2319,31 @@ def test_warns_if_file_sent_already_errors(
         "app.main.views.send.get_csv_metadata",
         return_value={"original_file_name": uploaded_file_name},
     )
-    # Should be botocore.errorfactory.NoSuchKey but for some reason can't use that
+
-    with pytest.raises(  # noqa: PT011,PT012  # Requires more research on how to refactor.
+    with pytest.raises(
-        expected_exception=Exception
+        expected_exception=Exception, match="Unable to locate credentials"
     ):
-        page = client_request.get(
+        stmt_for_test_warns_if_file_sent_already_errors(
-            "main.check_messages",
+            client_request, uploaded_file_name, fake_uuid, mock_get_jobs
-            service_id=SERVICE_ONE_ID,
-            template_id="5d729fbd-239c-44ab-b498-75a985f3198f",
-            upload_id=fake_uuid,
-            original_file_name=uploaded_file_name,
-            _test_page_title=False,
         )
 
-        assert normalize_spaces(page.select_one(".banner-dangerous").text) == (
-            "These messages have already been sent today "
-            "If you need to resend them, rename the file and upload it again."
-        )
 
-        mock_get_jobs.assert_called_once_with(SERVICE_ONE_ID, limit_days=0)
+def stmt_for_test_warns_if_file_sent_already_errors(
+    client_request, uploaded_file_name, fake_uuid, mock_get_jobs
+):
+    page = client_request.get(
+        "main.check_messages",
+        service_id=SERVICE_ONE_ID,
+        template_id="5d729fbd-239c-44ab-b498-75a985f3198f",
+        upload_id=fake_uuid,
+        original_file_name=uploaded_file_name,
+        _test_page_title=False,
+    )
+    assert normalize_spaces(page.select_one(".banner-dangerous").text) == (
+        "These messages have already been sent today "
+        "If you need to resend them, rename the file and upload it again."
+    )
+    mock_get_jobs.assert_called_once_with(SERVICE_ONE_ID, limit_days=0)
 
 
 def test_check_messages_column_error_doesnt_show_optional_columns(

tests/app/main/views/test_template_folders.py

@@ -1117,25 +1117,31 @@ def test_should_show_checkboxes_for_selecting_templates_assertion_error(
     mock_get_no_api_keys,
     user,
 ):
-    with pytest.raises(  # noqa: PT012  #  This will require more research into refactoring.
+    with pytest.raises(expected_exception=AssertionError):
-        expected_exception=AssertionError
+        _stmt_for_test_should_show_checkboxes_for_selecting_templates_assertion_error(
-    ):
+            client_request, user
-        client_request.login(user)
-
-        page = client_request.get(
-            "main.choose_template",
-            service_id=SERVICE_ONE_ID,
         )
-        checkboxes = page.select("input[name=templates_and_folders]")
 
-        assert len(checkboxes) == 4
 
-        assert checkboxes[0]["value"] == TEMPLATE_ONE_ID
+def _stmt_for_test_should_show_checkboxes_for_selecting_templates_assertion_error(
-        assert checkboxes[0]["id"] == "templates-or-folder-{}".format(TEMPLATE_ONE_ID)
+    client_request, user
+):
+    client_request.login(user)
 
-        for index in (1, 2, 3):
+    page = client_request.get(
-            assert checkboxes[index]["value"] != TEMPLATE_ONE_ID
+        "main.choose_template",
-            assert TEMPLATE_ONE_ID not in checkboxes[index]["id"]
+        service_id=SERVICE_ONE_ID,
+    )
+    checkboxes = page.select("input[name=templates_and_folders]")
+
+    assert len(checkboxes) == 4
+
+    assert checkboxes[0]["value"] == TEMPLATE_ONE_ID
+    assert checkboxes[0]["id"] == "templates-or-folder-{}".format(TEMPLATE_ONE_ID)
+
+    for index in (1, 2, 3):
+        assert checkboxes[index]["value"] != TEMPLATE_ONE_ID
+        assert TEMPLATE_ONE_ID not in checkboxes[index]["id"]
 
 
 @pytest.mark.parametrize(

tests/app/main/views/test_templates.py

@@ -304,7 +304,7 @@ def test_should_show_live_search_if_service_has_lots_of_folders(
     assert count_of_templates == 4
 
 
-@pytest.mark.parametrize(  # noqa: PT014  # Requires more research why there are duplicate params here.
+@pytest.mark.parametrize(
     ("service_permissions", "expected_values", "expected_labels"),
     [
         pytest.param(
@@ -320,19 +320,20 @@ def test_should_show_live_search_if_service_has_lots_of_folders(
                 "Copy an existing template",
             ],
         ),
-        pytest.param(
+        # TODO This is a duplicate of above. Why?
-            ["email", "sms"],
+        # pytest.param(
-            [
+        #     ["email", "sms"],
-                # 'email',
+        #     [
-                "sms",
+        #         # 'email',
-                "copy-existing",
+        #         "sms",
-            ],
+        #         "copy-existing",
-            [
+        #     ],
-                # 'Email',
+        #     [
-                "Start with a blank template",
+        #         # 'Email',
-                "Copy an existing template",
+        #         "Start with a blank template",
-            ],
+        #         "Copy an existing template",
-        ),
+        #     ],
+        # ),
     ],
 )
 def test_should_show_new_template_choices_if_service_has_folder_permission(
@@ -753,9 +754,9 @@ def test_choose_a_template_to_copy(
 
     assert len(actual) == len(expected)
 
-    for actual, expected in zip(actual, expected):  # noqa: B020
+    zipobject = zip(actual, expected)
+    for actual, expected in zipobject:
         assert normalize_spaces(actual.text) == expected
-
     links = page.select("main nav a")
     assert links[0]["href"] == url_for(
         "main.choose_template_to_copy",
@@ -799,7 +800,8 @@ def test_choose_a_template_to_copy_when_user_has_one_service(
 
     assert len(actual) == len(expected)
 
-    for actual, expected in zip(actual, expected):  # noqa: B020
+    zipobject = zip(actual, expected)
+    for actual, expected in zipobject:
         assert normalize_spaces(actual.text) == expected
 
     assert page.select("main nav a")[0]["href"] == url_for(
@@ -875,7 +877,8 @@ def test_choose_a_template_to_copy_from_folder_within_service(
 
     assert len(actual) == len(expected)
 
-    for actual, expected in zip(actual, expected):  # noqa: B020
+    zipobject = zip(actual, expected)
+    for actual, expected in zipobject:
         assert normalize_spaces(actual.text) == expected
 
     links = page.select("main nav a")

tests/app/notify_client/test_user_client.py

@@ -192,18 +192,11 @@ def test_returns_value_from_cache(
     assert mock_redis_set.call_args_list == expected_cache_set_calls
 
 
-@pytest.mark.parametrize(  # noqa: PT014  # Duplicate add_user_to_service has different params for each
+@pytest.mark.parametrize(
     ("client", "method", "extra_args", "extra_kwargs"),
     [
-        (
-            user_api_client,
-            "add_user_to_service",
-            [SERVICE_ONE_ID, sample_uuid(), [], []],
-            {},
-        ),
         (user_api_client, "update_user_attribute", [user_id], {}),
         (user_api_client, "reset_failed_login_count", [user_id], {}),
-        (user_api_client, "update_user_attribute", [user_id], {}),
         (user_api_client, "update_password", [user_id, "hunter2"], {}),
         (user_api_client, "verify_password", [user_id, "hunter2"], {}),
         (user_api_client, "check_verify_code", [user_id, "", ""], {}),

tests/app/s3_client/test_s3_logo_client.py

@@ -134,8 +134,7 @@ def test_does_not_delete_non_temp_email_file(client_request, mocker):
         "app.s3_client.s3_logo_client.delete_s3_object"
     )
 
-    with pytest.raises(ValueError) as error:  # noqa: PT011  # Requires more research.
+    with pytest.raises(ValueError, match="Not a temp file: logo.png"):
         delete_email_temp_file(filename)
 
     assert mocked_delete_s3_object.called is False
-    assert str(error.value) == "Not a temp file: {}".format(filename)

tests/app/utils/test_user.py

@@ -5,9 +5,9 @@ from werkzeug.exceptions import Forbidden
 from app.utils.user import user_has_permissions
 
 
-@pytest.mark.parametrize(  # noqa: PT007  # Ignoring wrong values type because of the list unpacking in the test.
+@pytest.mark.parametrize(
     "permissions",
-    (
+    [
         [
             # Route has one of the permissions which the user has
             "manage_service"
@@ -25,7 +25,7 @@ from app.utils.user import user_has_permissions
         [
             # Route has no specific permissions required
         ],
-    ),
+    ],
 )
 def test_permissions(
     client_request,
@@ -48,14 +48,14 @@ def test_permissions(
     index()
 
 
-@pytest.mark.parametrize(  # noqa: PT007  # Ignoring wrong values type because of the list unpacking in the test.
+@pytest.mark.parametrize(
     "permissions",
-    (
+    [
         [
             # Route has a permission which the user doesn’t have
             "send_messages"
         ],
-    ),
+    ],
 )
 def test_permissions_forbidden(
     client_request,

tests/conftest.py

@@ -2423,7 +2423,7 @@ def _os_environ():
         os.environ[k] = v
 
 
-@pytest.fixture  # noqa (C901 too complex)
+@pytest.fixture()
 def client_request(logged_in_client, mocker, service_one):  # noqa (C901 too complex)
     class ClientRequest:
         @staticmethod