classification
Title: No assertion in test_del_param_on_nonexistent_header function and unused variables in some places in test_email.py
Type: behavior Stage: resolved
Components: Tests Versions: Python 3.3, Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: python-dev, r.david.murray, vajrasky
Priority: normal Keywords:

Created on 2013-07-19 11:10 by vajrasky, last changed 2013-07-25 16:19 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
add_assert_equal_on_test_del_param_on_nonexistent_header.txt vajrasky, 2013-07-19 11:10 review
small_clean_up_to_test_email.txt vajrasky, 2013-07-20 03:01 review
Messages (9)
msg193356 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-07-19 11:10
In Lib/test/test_email/test_email.py, line 389, there is a unit test function:

    def test_del_param_on_nonexistent_header(self):
        msg = Message()
        msg.del_param('filename', 'content-disposition')

There is no assertion here, unlike other unit test functions, such as:

    def test_del_param_on_other_header(self):
        msg = Message()
        msg.add_header('Content-Disposition', 'attachment', filename='bud.gif')
        msg.del_param('filename', 'content-disposition')
        self.assertEqual(msg['content-disposition'], 'attachment')
msg193362 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-19 14:49
That's because there's nothing to assert.  The test is making sure no exception is raised.  (Why no exception is raised is a good question, but that design predates my stewardship of the module :)

A comment to that effect would probably be a good idea.
msg193363 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-07-19 16:01
I see.

Anyway, I am just curious. Why not doing try and catch?

    def test_del_param_on_nonexistent_header(self):
        msg = Message()
        try:
            msg.del_param('filename', 'content-disposition')
        except:
            self.fail('del_param on empty msg raised exception!')

Or is it better this way?

    def test_del_param_on_nonexistent_header(self):
        msg = Message()
        # Deleting param on empty msg should not raise exception.
        msg.del_param('filename', 'content-disposition')
msg193364 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-07-19 16:06
Anyway, I found another issue on line 393 on the same file:

    def test_del_nonexistent_param(self):
        msg = Message()
        msg.add_header('Content-Type', 'text/plain', charset='utf-8')
        existing_header = msg['Content-Type']
        msg.del_param('foobar', header='Content-Type')
        self.assertEqual(msg['Content-Type'], 'text/plain; charset="utf-8"')

The variable existing_header is never used. Either we can remove it or change it to:

    def test_del_nonexistent_param(self):
        msg = Message()
        msg.add_header('Content-Type', 'text/plain', charset='utf-8')
        existing_header = msg['Content-Type']
        msg.del_param('foobar', header='Content-Type')
        self.assertEqual(msg['Content-Type'], existing_header)

At first, I wanted to create a ticket for this. But then, I thought why do not we combine these menial problems together?
msg193365 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-07-19 16:10
Another unused variable in line 2268 on function test_bad_multipart:

    def test_bad_multipart(self):
        eq = self.assertEqual
        msg1 = Message()
        msg1['Subject'] = 'subpart 1'
        msg2 = Message()
        msg2['Subject'] = 'subpart 2'
        r = MIMEMessage(msg1)
        self.assertRaises(errors.MultipartConversionError, r.attach, msg2)

The variable eq is never used and can be removed.

I have changed the title of the ticket to reflect the problem better.
msg193373 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-19 20:34
We prefer one ticket per issue.  The line is fuzzy...I think it would be OK to fix these all at once since they are all small cleanups in test_email, but as you can tell from how complex the title got, they are not really conceptually related.

The try/except/fail is a valid style.  I prefer to just let the exception raise, and put in a comment to that effect, but it is definitely a personal style thing.
msg193397 - (view) Author: Vajrasky Kok (vajrasky) * Date: 2013-07-20 03:01
Okay, next time I'll separate the problems into different tickets.

Here is the patch to clean up the test.
msg193698 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-07-25 16:18
New changeset 61d9c561b63d by R David Murray in branch '3.3':
#18503: small cleanups in test_email.
http://hg.python.org/cpython/rev/61d9c561b63d

New changeset be5f1f0bea09 by R David Murray in branch 'default':
#18503: small cleanups in test_email.
http://hg.python.org/cpython/rev/be5f1f0bea09
msg193699 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-07-25 16:19
Thanks, Vajrasky.
History
Date User Action Args
2013-07-25 16:19:40r.david.murraysetstatus: open -> closed
versions: + Python 3.3
type: behavior
messages: + msg193699

resolution: fixed
stage: resolved
2013-07-25 16:18:10python-devsetnosy: + python-dev
messages: + msg193698
2013-07-20 03:01:53vajraskysetfiles: + small_clean_up_to_test_email.txt

messages: + msg193397
2013-07-19 20:34:32r.david.murraysetmessages: + msg193373
2013-07-19 16:10:26vajraskysetmessages: + msg193365
title: No assertion in test_del_param_on_nonexistent_header function -> No assertion in test_del_param_on_nonexistent_header function and unused variables in some places in test_email.py
2013-07-19 16:06:37vajraskysetmessages: + msg193364
2013-07-19 16:01:59vajraskysetmessages: + msg193363
2013-07-19 14:49:26r.david.murraysetmessages: + msg193362
2013-07-19 11:10:17vajraskycreate