Make query string comparison ignore order

Query string ordering is non-deterministic. This can cause tests to fail
in a non helpful way when we’re comparing two URLs. The values in the
query string can match, but the string won’t because the order is
different. This commit adds some code to split up the URL and check that
each part of it matches, rather than checking the thing as a whole.

tests/__init__.py

@@ -1,6 +1,7 @@
 import uuid
 from datetime import datetime, timedelta, timezone
 from unittest.mock import patch
+from urllib.parse import parse_qs, urlparse
 
 import pytest
 from flask import url_for
@@ -28,7 +29,7 @@ class TestClient(FlaskClient):
             login_user(model_user)
 
     def logout(self, user):
-        self.get(url_for("main.logout"))
+        self.get(url_for("main.sign_out"))
 
 
 def sample_uuid():
@@ -579,3 +580,28 @@ def validate_route_permission_with_client(mocker,
     if resp.status_code != response_code:
         pytest.fail("Invalid permissions set for endpoint {}".format(route))
     return resp
+
+
+def assert_url_expected(actual, expected):
+    actual_parts = urlparse(actual)
+    expected_parts = urlparse(expected)
+    for attribute in actual_parts._fields:
+        if attribute == 'query':
+            # query string ordering can be non-deterministic
+            # so we need to parse it first, which gives us a
+            # dictionary of keys and values, not a
+            # serialized string
+            assert parse_qs(
+                expected_parts.query
+            ) == parse_qs(
+                actual_parts.query
+            )
+        else:
+            assert getattr(
+                actual_parts, attribute
+            ) == getattr(
+                expected_parts, attribute
+            ), (
+                'Expected redirect: {}\n'
+                'Actual redirect: {}'
+            ).format(expected, actual)

tests/app/main/views/test_feedback.py

@@ -210,29 +210,44 @@ def test_passes_user_details_through_flow(
     {'feedback': 'blah', 'name': 'Fred'},
     {'feedback': 'blah'},
 ])
-@pytest.mark.parametrize('ticket_type, expected_response, things_expected_in_url, expected_error', [
-    (PROBLEM_TICKET_TYPE, 200, [], element.Tag),
-    (QUESTION_TICKET_TYPE, 302, ['thanks', 'email_address_provided=False', 'out_of_hours_emergency=False'], type(None)),
+@pytest.mark.parametrize('ticket_type, expected_response, expected_redirect, expected_error', [
+    (
+        PROBLEM_TICKET_TYPE,
+        200,
+        lambda: None,
+        element.Tag,
+    ),
+    (
+        QUESTION_TICKET_TYPE,
+        302,
+        partial(
+            url_for,
+            '.thanks',
+            email_address_provided=False,
+            out_of_hours_emergency=False,
+            _external=True,
+        ),
+        type(None),
+    ),
 ])
 def test_email_address_required_for_problems(
-    client,
+    client_request,
     mocker,
     data,
     ticket_type,
     expected_response,
-    things_expected_in_url,
+    expected_redirect,
     expected_error
 ):
     mocker.patch('app.main.views.feedback.zendesk_client')
-    response = client.post(
-        url_for('main.feedback', ticket_type=ticket_type),
-        data=data,
+    client_request.logout()
+    page = client_request.post(
+        'main.feedback',
+        ticket_type=ticket_type,
+        _data=data,
+        _expected_status=expected_response,
+        _expected_redirect=expected_redirect(),
     )
-    assert response.status_code == expected_response
-    # This is to work around non-deterministic query ordering in Flask url_for
-    for thing in things_expected_in_url:
-        assert thing in response.location
-    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
     assert isinstance(page.find('span', {'class': 'error-message'}), expected_error)
 
 

tests/conftest.py

@@ -16,6 +16,7 @@ from app import create_app
 from . import (
     TestClient,
     api_key_json,
+    assert_url_expected,
     generate_uuid,
     invite_json,
     job_json,
@@ -2875,6 +2876,9 @@ def client_request(
         def login(user, service=service_one):
             logged_in_client.login(user, mocker, service)
 
+        def logout():
+            logged_in_client.logout(None)
+
         @staticmethod
         def get(
             endpoint,
@@ -2939,7 +2943,7 @@ def client_request(
             )
             assert resp.status_code == _expected_status
             if _expected_redirect:
-                assert resp.location == _expected_redirect
+                assert_url_expected(resp.location, _expected_redirect)
             return BeautifulSoup(resp.data.decode('utf-8'), 'html.parser')
 
     return ClientRequest