No unit test for mailcap module

[gnofi]

BPO	6484
Nosy	@loewis, @ncoghlan, @vstinner, @ezio-melotti, @bitdancer, @akheron
Files	mailcap.txt: Sample mailcap file used in test_mailcap.py test_mailcap.py: Trunk version of test for mailcap module (Corrected) test_mailcap.py: py3k version of test for mailcap module (Corrected)

^{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 2011-08-23.13:53:01.064>
created_at = <Date 2009-07-14.18:39:06.439>
labels = ['type-feature', 'tests']
title = 'No unit test for mailcap module'
updated_at = <Date 2012-08-16.18:11:36.288>
user = 'https://bugs.python.org/gnofi'

bugs.python.org fields:

activity = <Date 2012-08-16.18:11:36.288>
actor = 'r.david.murray'
assignee = 'none'
closed = True
closed_date = <Date 2011-08-23.13:53:01.064>
closer = 'ncoghlan'
components = ['Tests']
creation = <Date 2009-07-14.18:39:06.439>
creator = 'gnofi'
dependencies = []
files = ['19125', '19126', '19127']
hgrepos = []
issue_num = 6484
keywords = []
message_count = 17.0
messages = ['90516', '90517', '90519', '97289', '97290', '103933', '115735', '117937', '117938', '142667', '142680', '142767', '142811', '142823', '142827', '168396', '168397']
nosy_count = 9.0
nosy_names = ['loewis', 'ncoghlan', 'vstinner', 'ezio.melotti', 'r.david.murray', 'gnofi', 'python-dev', 'petri.lehtinen', 'anthonyb']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue6484'
versions = ['Python 3.3']

[gnofi]

There is currently no test_mailcap or any other standalone unit test for
the mailcap module. The only existing test is a self-test at the end of
the module.

I would like to be assigned to work on this patch.

(Why am I assigning myself to write tests for a small, older module? I'm
a complete noob to the Python-Dev community and I'm getting my feet wet
with this. Let me know if you have any advice or if I'm doing something
wrong.)

[bitdancer]

Welcome!

Please read the material at http://www.python.org/dev if you haven't
already, especially the dev FAQ. At the moment the case can't be
assigned to you in the tracker interface, but your claiming it in the
text is sufficient.

Please write the tests and create a diff-patch as outlined in the FAQ.
Then upload it here for review. If you need advice, #python-dev on
Freenode is a good place to get it.

[loewis]

Please also identify yourself with your full name.

[gnofi]

Attached is a new file for testing the mailcap module in Python 2.7.

Writing the test was a little tricky because the existence and contents of the .mailcap file(s) will vary depending on the system or user. Therefore, the test mostly uses its own version of .mailcap, which I will also submit to this issue.

This is the first patch I've ever submitted to Python Core Development. Any feedback is appreciated.

[gnofi]

This is a sample .mailcap I created for the test. It should also go in the Lib/test directory.

It begins with a period due to a mailcap file naming convention (see RFC 1524). Is it OK that the file will be semi-hidden?

[gnofi]

Submitting Python 3 version of test. Note that it currently fails due to bpo-8496.

It should use the same .mailcap file I submitted earlier.

[bitdancer]

Thanks for contributing this; sorry it took so long to get a review. Overall the tests look good (I didn't work through the logic of each test that looks up data; I'm trusting you on that part :)

Here are some comments:

1. In test_listmailcapfiles, you can use test.support.EnvironmentVarGuard and inside the test set the value of MAILCAPS. That way you can test both cases (set and not set).

2. I notice that the listmailcapfiles docstring is inaccurate (it actually lists the *possible* locations of mailcap files, and applies only to unix). You could file a doc bug for that, but it is not an API method so it isn't a huge deal.

3. In test_lookup I think it might be better to hardcode the expected value rather than computing it. It would make it clearer what the test was expecting, and remove any chance that you are just repeating the algorithm that the code itself is using to compute the value.

4. Your use of EnvironmentVarGuard in GetcapsTest is not technically correct, though it does happen to work. You should really be doing self.env = test.support.EnvironmentVarGuard().__enter__() to mimic the 'with' protocol. It is a detail of the implementation that __enter__ returns self.

5. Why conditionalize your test on the existence of a system mailcap file? You want a controlled environment for testing, so it is better, IMO, to just use your test mailcap file. This will simplify your tests.
   Or you could add a second test method that also does the simple checks if and only if one of the possible system mailcap files does exist, if your goal is to test that part of the code path. (In that case you should skip the test with an appropriate message if no system mailcap files are found).

6. I don't see any reason why the test file needs to be named ".mailcap", you can specify its filename in any test where you need it. It would indeed be better not to have a test file with a leading '.'. Current convention would be to create a directory named 'mailcaptestdata' and put your test data files in there, but since you have only one it would in fact be OK to just put it directly in the test directory if you can come up with a clear name for it. Alternatively you could group those tests that need it into a single test case and use the new setUpClass to create it from an embedded string and tearDownClass to delete it afterward.

Thanks again for working on this.

[gnofi]

Replacing .mailcap with mailcap.txt. Same content, but with more conventional file name.

[gnofi]

r.david.murray: Thanks a lot for your feedback! I've implemented those suggestions and they helped located a bug. (See case 9923.)

[anthonyb]

Added ncoghlan to the nosy list - we're reviewing/fixing unit test coverage as part of the sprints at PyconAU. Thanks Gnofi!

[python-dev]

New changeset 69cb66ab61cc by Nick Coghlan in branch 'default':
Add unit tests for the mailcap module. Patch by Gregory Nofi (closes bpo-6484)
http://hg.python.org/cpython/rev/69cb66ab61cc

[python-dev]

New changeset a1bb07d67a24 by Ezio Melotti in branch 'default':
bpo-6484: refactor a bit the tests.
http://hg.python.org/cpython/rev/a1bb07d67a24

[ncoghlan]

The buildbots are reporting a test failure on Windows:

======================================================================
FAIL: test_listmailcapfiles (test.test_mailcap.HelperFunctionTest)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows\build\lib\test\test_mailcap.py", line 74, in test_listmailcapfiles
    self.assertEqual(env_mailcaps, mcfiles)
AssertionError: Lists differ: ['/testdir1/.mailcap', '/testd... != ['/testdir1/.mailcap;/testdir2...

First differing element 0:
/testdir1/.mailcap
/testdir1/.mailcap;/testdir2/mailcap

First list contains 1 additional elements.
First extra element 1:
/testdir2/mailcap

• ['/testdir1/.mailcap', '/testdir2/mailcap']
  ? ^^^^

+ ['/testdir1/.mailcap;/testdir2/mailcap']
? ^

There's a discrepancy between the test (uses os.pathsep) and the mailcap module (hardcoded to use ':' on all platforms).

I suspect the test is actually right and the module is wrong, but I don't know enough about mailcap to be sure. Anyone else?

[gnofi]

There's a bug for that test failure: bpo-9923. I submitted a patch with it. Committing that should fix the failure.

[ncoghlan]

Ah, cheers - closing this one again.

/me wanders off to meta-tracker to ask for a list of "dependency of" and "superseder of" issues in the issue header...

[akheron]

Any reason why this hasn't been backported to 3.2 or 2.7?

[bitdancer]

There are divided opinions about the advisability of backporting tests that are not part of a bug fix. In this case, there is also the fact that it includes a test that fails without a bug fix that was not backported.