R. David Murray <rdmur...@bitdance.com> added the comment: 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. ---------- _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue6484> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
