From c17aa249dc021513eabb8f5f36ba6d8b77cb3ebd Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Mon, 15 Jul 2019 13:52:42 +0100
Subject: [PATCH] Make query string comparison ignore order
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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                     | 28 +++++++++++++++++-
 tests/app/main/views/test_feedback.py | 41 ++++++++++++++++++---------
 tests/conftest.py                     |  6 +++-
 3 files changed, 60 insertions(+), 15 deletions(-)

diff --git a/tests/__init__.py b/tests/__init__.py
index d1ef1ddbc..2459b8500 100644
--- a/tests/__init__.py
+++ b/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)
diff --git a/tests/app/main/views/test_feedback.py b/tests/app/main/views/test_feedback.py
index bcf466689..bbebd4d2e 100644
--- a/tests/app/main/views/test_feedback.py
+++ b/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)
 
 
diff --git a/tests/conftest.py b/tests/conftest.py
index 1c66bc03b..c8e7ca1fb 100644
--- a/tests/conftest.py
+++ b/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