Issue 2267 - Unify form handling by reusing form_handler()

sitescripts/reports/tests/test_updateReport.py

Index: sitescripts/reports/tests/test_updateReport.py
===================================================================
new file mode 100644
--- /dev/null
+++ b/sitescripts/reports/tests/test_updateReport.py
@@ -0,0 +1,102 @@
+# This file is part of the Adblock Plus web scripts,
+# Copyright (C) 2017-present eyeo GmbH
+#
+# Adblock Plus is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License version 3 as
+# published by the Free Software Foundation.
+#
+# Adblock Plus is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
+
+import sys
+from urllib import urlencode
+from urllib2 import urlopen, HTTPError
+
+import pytest
+from wsgi_intercept import (urllib_intercept, add_wsgi_intercept,
+                            remove_wsgi_intercept)
+
+sys.modules['MySQLdb'] = sys

[Vasily Kuznetsov, 2019/02/05 17:12:08]

We should probably add a comment here explaining why we need this line. What do
you think?

[rhowell, 2019/02/07 03:54:33]

Done.

+from sitescripts.reports.web.updateReport import handleRequest
+
+LOCAL_HOST = 'test.local'
+REMOTE_HOST = 'reports.adblockplus.org'
+PORT = 80
+PLAINTEXT_GUID = '12345678-1234-1234-1234-123456789abc'
+
+
+def intercept_fn(environ, start_response):
+    assert environ['SERVER_NAME'] == REMOTE_HOST
+    assert PLAINTEXT_GUID in environ['PATH_INFO']
+    return 'Intercepted!'
+
+
+@pytest.fixture
+def response_for():
+    """Register two intercepts, and return responses for them."""
+    urllib_intercept.install_opener()
+    add_wsgi_intercept(LOCAL_HOST, PORT, lambda: handleRequest)
+    add_wsgi_intercept(REMOTE_HOST, 443, lambda: intercept_fn)
+
+    def response_for(data):
+        url = 'http://{}:{}'.format(LOCAL_HOST, PORT)
+        response = urlopen(url, urlencode(data))
+        return response.code, response.read()
+
+    yield response_for
+    remove_wsgi_intercept()
+
+
+@pytest.fixture
+def form_data():
+    return {
+        'email': 'john_doe@gmail.com',
+        'secret': '92b3e705f2abe74c20c1c5ea9abd9ba2',
+        'guid': PLAINTEXT_GUID,
+        'status': 'x' * 1025,
+        'usefulness': 'test USEFULNESS',
+        'notify': 'test NOTIFY',
+        'test_mode': True,
+    }
+
+
+@pytest.mark.parametrize('field,message', [
+    (('guid', ''), 'Invalid or missing report GUID'),
+    (('secret', ''), 'Wrong secret value'),
+])
+def test_http_errs(field, message, response_for, form_data, mocker):
+    mocker.patch('sitescripts.reports.web.updateReport.getReport',

[Vasily Kuznetsov, 2019/02/05 17:12:08]

This patch could be made into a fixture, since we reuse it in three tests. 
Would be less code repetition this way.

[rhowell, 2019/02/07 03:54:32]

Each of the three calls to getReport return something different (usefulness,
usefulness+email, and None). I guess I could make it a pytest fixture that takes
an argument and returns a different report based on the argument, but that
doesn't seem like it would save too much code duplication. Unless you think it
would be better?

[Vasily Kuznetsov, 2019/02/07 20:19:15]

Yeah, you're right. Not worth it.

[rhowell, 2019/02/08 01:34:51]

Acknowledged.

+                 new=lambda *args: {'usefulness': 1})
+    key, value = field
+    form_data[key] = value
+    with pytest.raises(HTTPError) as error:
+        response_for(form_data)
+
+    assert message in error.value.read()
+
+
+def test_success(response_for, form_data, mocker):
+    # These methods are patched to avoid the need for a MySQL database
+    mocker.patch('sitescripts.reports.web.updateReport.getReport',
+                 new=lambda *args: {'usefulness': 1, 'email': 'test@test.com'})
+    mocker.patch('sitescripts.reports.web.updateReport.saveReport',

[Vasily Kuznetsov, 2019/02/05 17:12:08]

If you want a noop function, you actually don't even need to provide the `new`
argument. mocker.patch() would create a MagicMock object by default, that tries
to do something reasonable in all possible scenarios. The MagicMock will also be
returned from this call so you can inspect it at the end of the test to check
that the mocked function has been called and check the invocation arguments.
This way we get a more thorough test.

[rhowell, 2019/02/07 03:54:33]

Ah, nice. Good idea.

+                 new=lambda *args: None)
+    mocker.patch('sitescripts.reports.web.updateReport.updateUserUsefulness',
+                 new=lambda *args: None)
+    mocker.patch('sitescripts.reports.web.updateReport.sendUpdateNotification',
+                 new=lambda *args: None)
+    assert response_for(form_data) == (200, '\nIntercepted!')
+
+
+def test_get_report_error(response_for, form_data, mocker):
+    mocker.patch('sitescripts.reports.web.updateReport.getReport',
+                 new=lambda *args: None)

[rhowell, 2019/02/07 03:54:33]

For some reason, I get an error if I don't provide the `new=lambda *args: None` 
here.

WSGIAppError: AttributeError("'module' object has no attribute 'connect'",) at
... /sitescripts/reports/utils.py:275

[Vasily Kuznetsov, 2019/02/07 20:19:15]

I tested it a bit and it seems that when we provide no `new` value, pytest-mock
inserts a MagicMock there and the call to it doesn't return None (it returns
another MagicMock). Then our check for None doesn't work, execution continues
and fails at some db-related call that we haven't mocked. I think the simplest
way around this is to keep your original code as it is in this version.

[rhowell, 2019/02/08 01:34:51]

Done.

+    with pytest.raises(HTTPError) as error:
+        response_for(form_data)
+
+    assert 'Report does not exist' in error.value.read()