Issue 2267 - Unify form handling by reusing form_handler()

sitescripts/reports/tests/test_updateReport.py

 # 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)
 
+# We are mocking the functions that use MySQLdb, so it is not needed. This
+# is to prevent the tests from crashing when they try to import it.
 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'
+UR_PATH = 'sitescripts.reports.web.updateReport.'
 
 
 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',
+        'email': 'jane_doe@example.com',
         'secret': '92b3e705f2abe74c20c1c5ea9abd9ba2',
         'guid': PLAINTEXT_GUID,
         'status': 'x' * 1025,
-        'usefulness': 'test USEFULNESS',
+        'usefulness': 0,
         'notify': 'test NOTIFY',
-        'test_mode': True,
+        'message': 'test MESSAGE',
+        'subject': 'test SUBJECT',
+        'name': 'test NAME',
     }
 
 
 @pytest.mark.parametrize('field,message', [
-    (('guid', ''), 'Invalid or missing report GUID'),
-    (('secret', ''), 'Wrong secret value'),
+    (('guid', 'badGUID'), 'Invalid or missing report GUID'),
+    (('secret', 'badSECRET'), '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})
+    mocker.patch(UR_PATH + 'getReport', 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)
+    mocker.patch(UR_PATH + 'getReport', new=lambda *args: {'usefulness': 1,
+                 'email': 'jane_doe@example.com'})
+    sr_mock = mocker.patch(UR_PATH + 'saveReport')
+    uuu_mock = mocker.patch(UR_PATH + 'updateUserUsefulness')
+    sun_mock = mocker.patch(UR_PATH + 'sendUpdateNotification')
+
     assert response_for(form_data) == (200, '\nIntercepted!')
+
+    assert sr_mock.call_count == 1
+    for key in ['usefulness', 'email']:
+        assert key in sr_mock.call_args[0][1]
+        assert sr_mock.call_args[0][1][key] == str(form_data[key])
+
+    assert '0' in uuu_mock.call_args[0] and 1 in uuu_mock.call_args[0]
+
+    for key in ['email', 'status']:
+        assert key in sun_mock.call_args[0][0]
+    assert sun_mock.call_args[0][0]['email'] == form_data['email']
+
+    # These should not be equal, because updateReport.py strips characters
+    # over 1024, and form_data['status'] has 1025.
+    assert str(sr_mock.call_args[0][1]['status']) != form_data['status']
+    assert str(sun_mock.call_args[0][0]['status']) != form_data['status']
 
 
 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.

+    mocker.patch(UR_PATH + 'getReport', new=lambda *args: None)
     with pytest.raises(HTTPError) as error:
         response_for(form_data)
 
     assert 'Report does not exist' in error.value.read()