Issue 4814 - Adds csv log to formmail2

sitescripts/formmail/test/test_formmail2.py

 # This file is part of the Adblock Plus web scripts,
-# Copyright (C) 2006-2016 Eyeo GmbH
+# Copyright (C) 2006-2017 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.
 #
@@ skipping 41 matching lines @@
 
 
 # We make this a fixture instead of a constant so we can modify it in each
 # test as needed without affecting other tests.
 @pytest.fixture
 def form_data():
     return {
         'email': 'john_doe@gmail.com',
         'mandatory': 'john_doe@gmail.com',
         'non_mandatory_message': 'Once upon a time\nthere lived a king.',
-        'non_mandatory_email': 'test@test.com'

[Vasily Kuznetsov, 2017/03/21 14:00:34]

I usually put the comma at the end of last item in multiline lists and dicts
like this because the changes that add items to it later will look cleaner in
diffs. Not a serious issue here, but in general it seems like a good rule to
follow.

[Jon Sonesen, 2017/03/22 14:28:20]

Done.

+        'non_mandatory_email': 'test@test.com',
     }
 
 
 @pytest.fixture
 def response_for(form_handler):
     """ Registers two intercepts, returns responses for them based on bool """
     urllib_intercept.install_opener()
     add_wsgi_intercept(HOST, LOG_PORT, lambda: form_handler[0])
     add_wsgi_intercept(HOST, NO_LOG_PORT, lambda: form_handler[1])
 
     def response_for(data, log=False):
-        url = 'http://{}:{}'.format(HOST, NO_LOG_PORT)
         if log:

[Vasily Kuznetsov, 2017/03/21 14:00:34]

I think this would be more readable as:

if log:
    url = ...
else:
    url = ...

[Jon Sonesen, 2017/03/22 14:28:20]

Done.

             url = 'http://{}:{}'.format(HOST, LOG_PORT)
+        else:
+            url = 'http://{}:{}'.format(HOST, NO_LOG_PORT)
         if data is None:
             response = urlopen(url)
         else:
             response = urlopen(url, urlencode(data))
         return response.code, response.read()
 
     yield response_for
     remove_wsgi_intercept()
 
 
-def test_form_handler_email_errors(form_config):
-    tmp_config = form_config

[Vasily Kuznetsov, 2017/03/21 14:00:34]

What is the purpose of this assignment? Why don't we just use `form_config`
directly? Same for the following functions.

[Jon Sonesen, 2017/03/22 14:28:20]

whoops. Done

-    del tmp_config['url'].value
-    with pytest.raises(Exception) as error:
-        formmail2.make_handler('test', tmp_config)[1]
-    assert error.value.message == 'No URL configured for form handler: test'
+@pytest.fixture
+def sm_mock(mocker):
+    return mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
 
 
-def test_form_handler_field_errors(form_config):
-    tmp_config = form_config
-    tmp_config['fields'] = {}
+@pytest.mark.parametrize('key,message', [
+    ('url', 'No URL configured for form handler: test'),
+    ('fields', 'No fields configured for form handler: test'),
+    ('template', 'No template configured for form handler: test'),
+])
+def test_config_errors(key, message, form_config):
+    del form_config[key]
     with pytest.raises(Exception) as error:
-        formmail2.make_handler('test', tmp_config)[1]
-    assert error.value.message == 'No fields configured for form handler: test'
+        formmail2.make_handler('test', form_config)[1]
+    assert error.value.message == message
 
-    del tmp_config['fields']
+
+@pytest.mark.parametrize('field,message', [
+    (('new_field', 'foo'), 'Unexpected field/fields: new_field'),
+    (('mandatory', ''), 'No mandatory entered'),
+    (('non_mandatory_email', 'asfaf'), 'Invalid email'),
+    (('email', 'asfaf'), 'You failed the email validation'),
+    (('email', ''), 'You failed the email test'),
+])
+def test_http_errs(field, message, response_for, form_data, sm_mock):
+    key, value = field
+    form_data[key] = value
+    with pytest.raises(HTTPError) as error:
+        response_for(form_data)
+    assert error.value.read() == message
+
+
+@pytest.mark.parametrize('field,expected', [
+    (('non_mandatory_message', '\xc3\xb6'), (200, '')),
+    (('non_mandatory_message', ''), (200, '')),
+])
+def test_success(field, expected, log_path, response_for, form_data, sm_mock):
+    key, value = field
+    form_data[key] = value
+    assert response_for(form_data, log=False) == expected
+    assert sm_mock.call_count == 1
+
+    params = sm_mock.call_args[0][1]['fields']
+    assert set(params.keys()) == set(form_data.keys())
+    for key, value in form_data.items():
+        assert params[key] == value.decode('utf8')
+
+    assert response_for(form_data, log=True) == expected
+    assert sm_mock.call_count == 2
+
+    assert response_for(form_data, log=True) == expected
+    assert sm_mock.call_count == 3
+
+    with open(log_path) as log_file:
+        reader = DictReader(log_file)
+        row = reader.next()
+        # rows should not be equal because the time field
+        # is added by the logging function.
+        assert row != reader.next()
+
+
+def test_config_field_errors(form_config):
+    form_config['fields'] = {}
     with pytest.raises(Exception) as error:
-        formmail2.make_handler('test', tmp_config)[1]
+        formmail2.make_handler('test', form_config)[1]
     assert error.value.message == 'No fields configured for form handler: test'
 
 
-def test_form_handler_template_errors(form_config):
-    tmp_config = form_config
-    tmp_config['template'].value = 'no'
+def test_config_template_errors(form_config):
+    form_config['template'].value = 'no'
     with pytest.raises(Exception) as error:
-        formmail2.make_handler('test', tmp_config)[1]
-    assert error.typename == 'TemplateNotFound'
-
-    del tmp_config['template'].value
-    with pytest.raises(Exception) as error:
-        formmail2.make_handler('test', tmp_config)[1]
-    assert error.value.message == ('No template configured for form handler'
-                                   ': test')
-    del tmp_config['template']
-    with pytest.raises(Exception) as error:
-        formmail2.make_handler('test', tmp_config)[1]
-    assert error.value.message == ('No template configured for form handler'
-                                   ': test')
+        formmail2.make_handler('test', form_config)[1]
+    assert error.value.message == 'Template not found at: no'
 
 
 def test_config_parse(form_config):
     assert form_config['url'].value == 'test/apply/submit'
     assert form_config['fields']['email'].value == 'mandatory, email'
 
 
-def test_sendmail_fail(log_path, response_for, form_data, mocker):
-    sm_mock = mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
+def test_sendmail_fail(log_path, response_for, form_data, sm_mock):
     sm_mock.side_effect = Exception('Sendmail Fail')
-    with pytest.raises(HTTPError) as error:
+    with pytest.raises(HTTPError):
         response_for(form_data, log=True)
-    assert error.typename == 'HTTPError'
 
     with open(log_path) as log_file:
         row = DictReader(log_file).next()
-        assert 'time' in row
-
-
-@pytest.mark.parametrize('log, res',
-                         [(True, (200, '')), (False, (200, ''))])
-def test_utf8_success(log, res, log_path, response_for, form_data, mocker):
-    """ DictWriter does not accpet utf-8, call log handler """

[Vasily Kuznetsov, 2017/03/21 14:00:34]

typo in "accpet".

[Jon Sonesen, 2017/03/22 14:28:19]

Done.

-    form_data['non_mandatory_message'] = '\xc3\xb6'
-    sm_mock = mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    assert response_for(form_data, log) == res
-    assert sm_mock.call_count == 1
-    params = sm_mock.call_args[0][1]['fields']
-    assert set(params.keys()) == set(form_data.keys())
-    for key, value in form_data.items():
-        assert params[key] == value.decode('utf8')
-
-
-def test_success(response_for, form_data, mocker):
-    sm_mock = mocker.patch('sitescripts.formmail.web.formmail2.sendMail')

[Vasily Kuznetsov, 2017/03/21 14:00:34]

This and some following lines seem to be duplicated quite a bit in the functions
around. I wonder if we could move them out somehow into a function or a fixture
or maybe just use parametrization for the whole thing.

If it looks too complicated, don't bother, we'll just do it separately in
another change later.

[Jon Sonesen, 2017/03/22 14:28:20]

The thing is that some of the test's have different behavior depending on
whether the log is there or not. I can look into it though and submit what i
come up with.

-    assert response_for(form_data) == (200, '')
-    assert sm_mock.call_count == 1
-    params = sm_mock.call_args[0][1]['fields']
-    assert set(params.keys()) == set(form_data.keys())
-    for key, value in form_data.items():
-        assert params[key] == value
-
-
-def test_log_success(log_path, response_for, form_data, mocker):
-    sm_mock = mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    assert response_for(form_data, log=True) == (200, '')
-    assert sm_mock.call_count == 1
-    params = sm_mock.call_args[0][1]['fields']
-    assert set(params.keys()) == set(form_data.keys())
-    for key, value in form_data.items():
-        assert params[key] == value
-    with open(log_path) as log_file:
-        row = DictReader(log_file).next()
-        assert 'time' in row
-
-
-def test_log_append_success(log_path, response_for, form_data, mocker):
-    sm_mock = mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    assert response_for(form_data, log=True) == (200, '')
-    form_data['non_mandatory_message'] = ''
-    assert response_for(form_data, log=True) == (200, '')
-    assert sm_mock.call_count == 2
-    params = sm_mock.call_args[0][1]['fields']
-    assert set(params.keys()) == set(form_data.keys())
-    for key, value in form_data.items():
-        assert params[key] == value
-    with open(log_path) as log_file:
-        reader = DictReader(log_file)
-        row = reader.next()
-        assert row != reader.next()
-
-
-def test_non_mandatory_no_msg(response_for, form_data, mocker):
-    mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    form_data['non_mandatory_message'] = ''
-    assert response_for(form_data) == (200, '')
-
-
-def test_invalid_email_cstm_msg(response_for, form_data, mocker, form_config):
-    mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    form_data['email'] = 'bademail'
-    with pytest.raises(HTTPError) as error:
-        response_for(form_data)
-    assert error.value.read() == 'You failed the email validation'
-
-
-def test_valid_nan_mandatory_email(response_for, form_data, mocker):

[Vasily Kuznetsov, 2017/03/21 14:00:34]

...non_mandatory...? :)

[Jon Sonesen, 2017/03/22 14:28:20]

Done.

-    mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    form_data['non_mandatory_email'] = 'asfaf'
-    with pytest.raises(HTTPError) as error:
-        response_for(form_data)
-    assert error.value.read() == 'Invalid email'
-
-    del form_data['non_mandatory_email']
-    assert response_for(form_data) == (200, '')
-
-
-def test_mandatory_fail_dflt_msg(response_for, form_data, mocker):
-    mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
-    del form_data['mandatory']
-    with pytest.raises(HTTPError) as error:
-        response_for(form_data)
-    assert error.value.read() == 'No mandatory entered'
-
-
-def test_field_err(form_config, form_data, log_path):
-    """ Submits a form that does not have the dame fields as previous submissions

[Vasily Kuznetsov, 2017/03/21 14:00:35]

typo in "dame"?

[Jon Sonesen, 2017/03/22 14:28:20]

Done.

-

[Vasily Kuznetsov, 2017/03/21 14:00:34]

Here you're continuing the same sentence in the lines following the first line
of the docstring. In such cases it doesn't make sense to follow the
title-empty_line-full_text structure of the docstring so we don't need an empty
line.

My suggestion regarding the docstrings was that where it's possible to change
them to title-empty_line-full_text structure, we should do so, but if it's not
possible, just simple multiline without a blank line would be ok. Sorry if I
didn't express this clear enough.

[Jon Sonesen, 2017/03/22 14:28:20]

Done.

-    that have the same form name, asserts that proper message is returned and
-    the row was properly written
-    """
-    formmail2.log_formdata(form_data, log_path)
-    del(form_config['fields']['email'])
-    del(form_data['email'])
-    try:
-        formmail2.log_formdata(form_data, log_path)
-    except Exception as e:
-        assert e.message == ('Field names have changed, error log '
-                             'written to {}_error').format(log_path)
-
-    with open(log_path+'_error') as error_log:
-        assert DictReader(error_log).next() == form_data
+        assert row != form_data
 
 
 def test_append_field_err(form_config, form_data, log_path):
-    """ Submits two identical forms that do not match the previous fields
+    """ Checks that error logs are correctly written and appended
 
-    found in the log file, triggering two rows to be added to the error
-    log and asserting the proper message is returned and that the rows
-    were written as expected
+    Submits three forms, the second two have different fields to the first
+    and should be added to the same log file as each other, and be identical
     """
     formmail2.log_formdata(form_data, log_path)
-    del(form_config['fields']['email'])
-    del(form_data['email'])
-    try:
+    del form_data['email']
+
+    # submit two forms with fields that dont match the config
+    # this should append the second form to the error log file
+    with pytest.raises(Exception):
         formmail2.log_formdata(form_data, log_path)
-    except Exception:
-        pass
-    try:
+    with pytest.raises(Exception):
         formmail2.log_formdata(form_data, log_path)
-    except Exception as e:
-        assert e.message == ('Field names have changed, error log'
-                             ' appended to {}_error').format(log_path)
 
-    with open(log_path+'_error') as error_log:
+    with open(log_path + '_error') as error_log:
         reader = DictReader(error_log)
-        # two identical rows should be in the error log
         assert reader.next() == form_data
         assert reader.next() == form_data