From ecc506306be556d51c64da05740b2a2539cd5499 Mon Sep 17 00:00:00 2001 From: Chris Hill Scott Date: Tue, 4 Jul 2017 13:38:08 +0100 Subject: [PATCH 1/5] Move helper function `test_utils.py` is actually a file for testing some utils, not some utils for testing. Test helper functions and fixtures are better placed in conftest, and also means that we can use them in conftest. --- tests/app/main/views/test_activity.py | 3 +-- tests/app/main/views/test_conversation.py | 2 +- tests/app/main/views/test_dashboard.py | 2 +- tests/app/main/views/test_jobs.py | 3 +-- tests/app/main/views/test_notifications.py | 3 +-- tests/app/main/views/test_send.py | 2 +- tests/app/main/views/test_service_settings.py | 2 +- tests/app/main/views/test_templates.py | 8 ++++++-- tests/app/test_utils.py | 6 ------ tests/conftest.py | 6 ++++++ 10 files changed, 19 insertions(+), 18 deletions(-) diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index 8872b5296..0ac91737a 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -8,8 +8,7 @@ from bs4 import BeautifulSoup from app.main.views.jobs import get_time_left, get_status_filters from tests import notification_json -from tests.conftest import SERVICE_ONE_ID, mock_get_notifications -from tests.app.test_utils import normalize_spaces +from tests.conftest import SERVICE_ONE_ID, mock_get_notifications, normalize_spaces from freezegun import freeze_time diff --git a/tests/app/main/views/test_conversation.py b/tests/app/main/views/test_conversation.py index e70aea412..0408690d5 100644 --- a/tests/app/main/views/test_conversation.py +++ b/tests/app/main/views/test_conversation.py @@ -8,7 +8,7 @@ from notifications_python_client.errors import HTTPError from tests.conftest import ( SERVICE_ONE_ID, ) -from tests.app.test_utils import normalize_spaces +from tests.conftest import normalize_spaces from freezegun import freeze_time from unittest import mock from app.main.views.conversation import get_user_number diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index d5819bc8e..668d2acd4 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -21,8 +21,8 @@ from tests.conftest import ( SERVICE_ONE_ID, mock_get_inbound_sms_summary, mock_get_inbound_sms_summary_with_no_messages, + normalize_spaces, ) -from tests.app.test_utils import normalize_spaces stub_template_stats = [ { diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index ad9abb6ae..3a4a65b82 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -8,8 +8,7 @@ from bs4 import BeautifulSoup from app.main.views.jobs import get_time_left, get_status_filters from tests import notification_json -from tests.app.test_utils import normalize_spaces -from tests.conftest import SERVICE_ONE_ID +from tests.conftest import SERVICE_ONE_ID, normalize_spaces from freezegun import freeze_time diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 6b7b8bb69..996394fda 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -8,8 +8,7 @@ from app.utils import ( DELIVERED_STATUSES, ) -from tests.app.test_utils import normalize_spaces -from tests.conftest import mock_get_notification, SERVICE_ONE_ID +from tests.conftest import mock_get_notification, SERVICE_ONE_ID, normalize_spaces @pytest.mark.parametrize('notification_status, expected_status', [ diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index acbb0ac73..f54cb0aaf 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -15,7 +15,6 @@ from notifications_utils.template import LetterPreviewTemplate, LetterImageTempl from notifications_utils.recipients import RecipientCSV from tests import validate_route_permission, validate_route_permission_with_client -from tests.app.test_utils import normalize_spaces from tests.conftest import ( mock_get_service_template, mock_get_service_template_with_placeholders, @@ -23,6 +22,7 @@ from tests.conftest import ( mock_get_service, mock_get_international_service, mock_get_service_email_template, + normalize_spaces, SERVICE_ONE_ID, ) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 54f5f5b22..6c0f70ac6 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -10,7 +10,7 @@ import app from app.main.views.service_settings import dummy_bearer_token from app.utils import email_safe from tests import validate_route_permission, service_json -from tests.app.test_utils import normalize_spaces +from tests.conftest import normalize_spaces from tests.conftest import active_user_with_permissions, platform_admin_user diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 38446df14..e193c2a9c 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -7,9 +7,13 @@ from flask import url_for from freezegun import freeze_time from notifications_python_client.errors import HTTPError from tests.conftest import service_one as create_sample_service -from tests.conftest import mock_get_service_email_template, mock_get_service_letter_template, SERVICE_ONE_ID +from tests.conftest import ( + mock_get_service_email_template, + mock_get_service_letter_template, + normalize_spaces, + SERVICE_ONE_ID, +) from tests import validate_route_permission, template_json, single_notification_json -from tests.app.test_utils import normalize_spaces from app.main.views.templates import get_last_use_message, get_human_readable_delta diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 9bfe8828b..e1fa0e4f3 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -154,9 +154,3 @@ def test_generate_notifications_csv_calls_twice_if_next_link(mocker): # mock_calls[0][2] is the kwargs from first call assert mock_get_notifications.mock_calls[0][2]['page'] == 1 assert mock_get_notifications.mock_calls[1][2]['page'] == 2 - - -def normalize_spaces(input): - if isinstance(input, str): - return ' '.join(input.split()) - return normalize_spaces(' '.join(item.text for item in input)) diff --git a/tests/conftest.py b/tests/conftest.py index 2afa1880e..69fc41a23 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1778,3 +1778,9 @@ def client_request(logged_in_client): return BeautifulSoup(resp.data.decode('utf-8'), 'html.parser') return ClientRequest + + +def normalize_spaces(input): + if isinstance(input, str): + return ' '.join(input.split()) + return normalize_spaces(' '.join(item.text for item in input)) From 5a76244d6d4365c041ae59f708d65fb62592ac6f Mon Sep 17 00:00:00 2001 From: Chris Hill Scott Date: Tue, 4 Jul 2017 14:05:11 +0100 Subject: [PATCH 2/5] Fix page title on individual notification page Should match `

` on the page. --- app/templates/views/notifications/notification.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 4cba2923b..f2d4c4ea7 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -5,7 +5,7 @@ {% from "components/page-footer.html" import page_footer %} {% block service_page_title %} - Report + {{ message_count_label(1, template.template_type, suffix='') | capitalize }} {% endblock %} {% block maincolumn_content %} From dd52f6082800c0e1f9f90592974cf9d72b41b4e6 Mon Sep 17 00:00:00 2001 From: Chris Hill Scott Date: Tue, 4 Jul 2017 14:07:43 +0100 Subject: [PATCH 3/5] Fix page titles on preview page Should match the `

` --- app/templates/views/check.html | 2 +- app/templates/views/notifications/check.html | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/check.html b/app/templates/views/check.html index e09bbd556..ebb207edd 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -14,7 +14,7 @@ {% endmacro %} {% block service_page_title %} - {{ "Error" if errors else "Preview" }} + {{ "Error" if errors else "Preview of {}".format(template.name) }} {% endblock %} {% block maincolumn_content %} diff --git a/app/templates/views/notifications/check.html b/app/templates/views/notifications/check.html index fb336b631..e9f65c00d 100644 --- a/app/templates/views/notifications/check.html +++ b/app/templates/views/notifications/check.html @@ -3,7 +3,7 @@ {% from "components/message-count-label.html" import message_count_label %} {% block service_page_title %} - {{ "Error" if error else "Preview" }} + {{ "Error" if error else "Preview of {}".format(template.name) }} {% endblock %} {% block maincolumn_content %} From 7ff6075e0a2e3d451981cb3344e05b67be5c0d6b Mon Sep 17 00:00:00 2001 From: Chris Hill Scott Date: Tue, 4 Jul 2017 14:13:06 +0100 Subject: [PATCH 4/5] Fix page title on 404 Should match the `

` on the page --- app/templates/error/404.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/error/404.html b/app/templates/error/404.html index cf691522f..ea6a5b701 100644 --- a/app/templates/error/404.html +++ b/app/templates/error/404.html @@ -1,5 +1,5 @@ {% extends "withoutnav_template.html" %} -{% block per_page_title %}Page not found{% endblock %} +{% block per_page_title %}Page could not be found{% endblock %} {% block maincolumn_content %}
From 17199f0c2695856448b786d1d08fb1613bc5cbcb Mon Sep 17 00:00:00 2001 From: Chris Hill Scott Date: Tue, 4 Jul 2017 14:04:46 +0100 Subject: [PATCH 5/5] Test for page titles in client request fixture MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’re frequently ending up with mismatched page titles on our pages because: - they’re hard to spot in the browsers tab bar - new pages are often created by copy pasting old ones It would be hard to write some kind of macro that spat out the h1 and the page title because they go into different blocks in the parent template. Instead, we can catch these mistakes by testing for them. Going forward we should be using the `client_request` fixture for testing. So it makes sense to put the page title test in here so that we get it for free in every test – same way we do for testing response codes. fixup! Test for page titles in client request fixture --- tests/conftest.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 69fc41a23..1f2c6218c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1757,13 +1757,26 @@ def client_request(logged_in_client): yield session @staticmethod - def get(endpoint, _expected_status=200, _follow_redirects=False, **endpoint_kwargs): + def get( + endpoint, + _expected_status=200, + _follow_redirects=False, + _test_page_title=True, + **endpoint_kwargs + ): resp = logged_in_client.get( url_for(endpoint, **(endpoint_kwargs or {})), follow_redirects=_follow_redirects, ) assert resp.status_code == _expected_status - return BeautifulSoup(resp.data.decode('utf-8'), 'html.parser') + page = BeautifulSoup(resp.data.decode('utf-8'), 'html.parser') + if _test_page_title: + page_title, h1 = ( + normalize_spaces(page.find(selector).text) for selector in ('title', 'h1') + ) + if not normalize_spaces(page_title).startswith(h1): + raise AssertionError('Page title ‘{}’ does not start with H1 ‘{}’'.format(page_title, h1)) + return page @staticmethod def post(endpoint, _data=None, _expected_status=None, _follow_redirects=False, **endpoint_kwargs):