Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

No assertion in test_del_param_on_nonexistent_header function and unused variables in some places in test_email.py #62703

Closed
vajrasky mannequin opened this issue Jul 19, 2013 · 9 comments
Closed

No assertion in test_del_param_on_nonexistent_header function and unused variables in some places in test_email.py #62703

vajrasky mannequin opened this issue Jul 19, 2013 · 9 comments
Labels
tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error

Comments

@vajrasky
Copy link
Mannequin

vajrasky mannequin commented Jul 19, 2013

BPO 18503
Nosy @bitdancer, @vajrasky
Files
  • add_assert_equal_on_test_del_param_on_nonexistent_header.txt
  • small_clean_up_to_test_email.txt
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
closed_at = <Date 2013-07-25.16:19:40.671>
created_at = <Date 2013-07-19.11:10:17.206>
labels = ['type-bug', 'tests']
title = 'No assertion in test_del_param_on_nonexistent_header function and unused variables in some places in test_email.py'
updated_at = <Date 2013-07-25.16:19:40.669>
user = 'https://github.com/vajrasky'

    bugs.python.org fields:

    activity = <Date 2013-07-25.16:19:40.669>
actor = 'r.david.murray'
assignee = 'none'
closed = True
closed_date = <Date 2013-07-25.16:19:40.671>
closer = 'r.david.murray'
components = ['Tests']
creation = <Date 2013-07-19.11:10:17.206>
creator = 'vajrasky'
dependencies = []
files = ['30977', '30988']
hgrepos = []
issue_num = 18503
keywords = []
message_count = 9.0
messages = ['193356', '193362', '193363', '193364', '193365', '193373', '193397', '193698', '193699']
nosy_count = 3.0
nosy_names = ['r.david.murray', 'python-dev', 'vajrasky']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue18503'
versions = ['Python 3.3', 'Python 3.4']
    @vajrasky
    Copy link
    Mannequin Author

    vajrasky mannequin commented Jul 19, 2013

    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')

    @vajrasky vajrasky mannequin added the tests Tests in the Lib/test dir label Jul 19, 2013
    @bitdancer
    Copy link
    Member

    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.

    @vajrasky
    Copy link
    Mannequin Author

    vajrasky mannequin commented Jul 19, 2013

    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')

    @vajrasky
    Copy link
    Mannequin Author

    vajrasky mannequin commented Jul 19, 2013

    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?

    @vajrasky
    Copy link
    Mannequin Author

    vajrasky mannequin commented Jul 19, 2013

    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.

    @vajrasky vajrasky mannequin changed the 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 Jul 19, 2013
    @bitdancer
    Copy link
    Member

    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.

    @vajrasky
    Copy link
    Mannequin Author

    vajrasky mannequin commented Jul 20, 2013

    Okay, next time I'll separate the problems into different tickets.

    Here is the patch to clean up the test.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 25, 2013

    New changeset 61d9c561b63d by R David Murray in branch '3.3':
    bpo-18503: small cleanups in test_email.
    http://hg.python.org/cpython/rev/61d9c561b63d

    New changeset be5f1f0bea09 by R David Murray in branch 'default':
    bpo-18503: small cleanups in test_email.
    http://hg.python.org/cpython/rev/be5f1f0bea09

    @bitdancer
    Copy link
    Member

    Thanks, Vajrasky.

    @bitdancer bitdancer added the type-bug An unexpected behavior, bug, or error label Jul 25, 2013
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    tests Tests in the Lib/test dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @bitdancer