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-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
@@ skipping 75 matching lines @@
     yield response_for
     remove_wsgi_intercept()
 
 
 @pytest.fixture
 def sm_mock(mocker):
     return mocker.patch('sitescripts.formmail.web.formmail2.sendMail')
 
 
 @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'),
-            ])
+    ('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', form_config)[1]
     assert error.value.message == message
 
 
 @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'),
-            ])
+    (('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()

[Vasily Kuznetsov, 2017/03/23 13:19:16]

Are they not equal because of time field? What do you think about adding a
comment here to explain the purpose of this comparison?

[Jon Sonesen, 2017/03/23 13:31:01]

Done.

 
 
 def test_config_field_errors(form_config):
     form_config['fields'] = {}
     with pytest.raises(Exception) as error:
         formmail2.make_handler('test', form_config)[1]
     assert error.value.message == 'No fields configured for form handler: test'
 
 
 def test_config_template_errors(form_config):
     form_config['template'].value = 'no'
     with pytest.raises(Exception) as error:
         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, 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'

[Vasily Kuznetsov, 2017/03/23 13:19:16]

You probably don't need to assert this since you've already told pytest to
expect that two lines above and it will complain if a wrong error is thrown.

[Jon Sonesen, 2017/03/23 13:31:01]

Acknowledged.

 
     with open(log_path) as log_file:
         row = DictReader(log_file).next()
-        assert 'time' in row
+        assert row != form_data
 
 
 def test_append_field_err(form_config, form_data, log_path):
     """ Checks that error logs are correctly written and appended
 
     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_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 as e:

[Vasily Kuznetsov, 2017/03/23 13:19:16]

Perhaps it's better to use with pytest.raises(...) to make sure the exception is
thrown and the assert is executed.

[Jon Sonesen, 2017/03/23 13:31:01]

Done. Odd that I chose to use try's in the first place 

-        assert e.message == ('Field names have changed, error log '
-                             'written to {}_error').format(log_path)
-    try:

[Vasily Kuznetsov, 2017/03/23 13:19:16]

I suppose there's some reason for trying this twice and it probably deserves to
be written down as a comment.

[Jon Sonesen, 2017/03/23 13:31:01]

Done.

+    with pytest.raises(Exception):
         formmail2.log_formdata(form_data, log_path)
-    except Exception as e:
-        assert e.message == ('Field names have changed, error log'

[Vasily Kuznetsov, 2017/03/23 13:19:16]

Do we need to assert the whole message here again or maybe a simpler check will
do, like startswith('Field names have changed')? Or even no check.

[Jon Sonesen, 2017/03/23 13:31:01]

Agreed, I will remove the assertions

-                             ' appended to {}_error').format(log_path)
 
     with open(log_path + '_error') as error_log:
         reader = DictReader(error_log)
         assert reader.next() == form_data
         assert reader.next() == form_data