classification
Title: Test imaplib API on all methods specified in RFC 3501
Type: Stage:
Components: Versions:
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: jesstess, maciej.szulik, pitrou, r.david.murray, zvyn
Priority: normal Keywords:

Created on 2014-08-05 00:17 by zvyn, last changed 2015-12-14 16:35 by maciej.szulik.

Files
File name Uploaded Description Edit
imaplib_test_rfc3501.patch zvyn, 2014-08-05 00:17 review
imaplib_test_rfc3501v2.patch zvyn, 2014-08-05 18:46 review
new_imap_tests.diff maciej.szulik, 2015-12-07 21:32 Initial proposal for imap tests
Repositories containing patches
https://bitbucket.org/soltysh/cpython/branch/new_imap_tests
Messages (11)
msg224790 - (view) Author: Milan Oberkirch (zvyn) * Date: 2014-08-05 00:17
I finished writing tests for all methods which are specified in RFC 3501 but left out extensions for now. The attached patch will trigger many resource warnings. Do you have an idea why the sockets aren't closed?
msg224865 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-08-05 17:54
Perhaps you need to close the client connection explicitly? (i.e. shutdown())
msg224873 - (view) Author: Milan Oberkirch (zvyn) * Date: 2014-08-05 18:46
Okay that was stupid from me (it was 5am when I submitted it), sorry.
msg241016 - (view) Author: Maciej Szulik (maciej.szulik) * (Python triager) Date: 2015-04-14 20:41
Milan, thanks for your patch, I've added comments to it (look for review link next to submitted file), can you address them please? 
Other option is, if you don't mind, I'll take over this issue and finish on top of your work.
msg241043 - (view) Author: Maciej Szulik (maciej.szulik) * (Python triager) Date: 2015-04-14 22:14
One more thing I didn't mention before is, please update it to match current default, as it's failing patching, right now.
msg241169 - (view) Author: Maciej Szulik (maciej.szulik) * (Python triager) Date: 2015-04-15 21:58
Milan one more thing to change in your patch, in all your TCs you're modifying client state, I mean this part:

# lets cheat a bit here:
client.state = 'SELECTED'

It's OK to do that on the mocked server, but this is not acceptable doing on the tested library. It's better to call client.login() to change the state to 'AUTH' properly and only after that, call the actual method under test.
msg241770 - (view) Author: Milan Oberkirch (zvyn) * Date: 2015-04-22 04:29
Thanks a lot for reviewing my patch! I'm currently travalling so I'm totally 
fine with you taking over this issue (would take me a few weeks till I can 
work on it).
msg241784 - (view) Author: Maciej Szulik (maciej.szulik) * (Python triager) Date: 2015-04-22 07:49
Thanks Milan for the info, I was about to ping you about that. I have a working prototype for the tests that I've been working on during PyCon sprints with David. I'll try to submit new patch within a couple of next days.
msg256072 - (view) Author: Maciej Szulik (maciej.szulik) * (Python triager) Date: 2015-12-07 21:32
Resubmitting from http://bugs.python.org/issue25591, which I filled in unnecessarily. 

Here are the rewritten test case I've developed during PyCon sprints along with David. Once we'll have the entire test suite in place it'll be safer to introduce any changes I'd like to cover, including updating the implementation to newest RFC 3501 version.
msg256331 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-12-13 19:54
I'm afraid I don't remember the details of our conversations at PyCon.  Specifically, I don't remember what problem(s) it is you are trying to solve with this patch.

What is the reason for not just adding your new tests to the ThreadedNetworkTests test case?  That way they would be run both in regular mode and SSL mode.  You seem to have copied bits of the code from that test into yours, but it mostly moves the code away from our usual conventions.  That is, we generally put the support classes at the top level and then subclass them in test methods that need variations, rather than defining them in setup methods (where they would inaccessible unless assigned to self).  Also, the existing reap_server method is to be preferred over a chain of addCleanups.  Finally, we are moving all tests to using unittest.main() so that they can be called using unittest only instead of requiring regrtest, so the change in the __main__ block is incorrect.

Now, all that said, I can see several things I'd do to refactor the existing tests to make them clearer and easier to modify.  For example, there are better ways to get reap_threads called on all test methods than wrapping each one individually (ex: tearDownClass).  And I prefer setup methods that do addCleanup (in this case, addCleanup(self.reap_server), rather than the existing context manager approach, though I don't know how widespread my preferred pattern is in our test suite yet.

All of which has little to do with the title of this issue :).  So, perhaps we should open a new issue for refactoring the tests, and then come back to this one once we are both satisfied with the refactoring?  We can cite this issue as the motivation for the refactoring.
msg256393 - (view) Author: Maciej Szulik (maciej.szulik) * (Python triager) Date: 2015-12-14 16:35
Per the discussion we've had with David on IRC here's what happening. The last patch I've submitted is meant to clean up the tests by:
1. having single _setup method 
2. the _setup method should addClenup to clean the environment
3. have stubs with implemented mostly used methods, all others should be implemented ad-hoc in test methods
4. use unittest.main() for running tests

The current idea is to create tests along the current one, to show how the new improve the readability. For this work I'm reopening previously submitted issue http://bugs.python.org/issue25591 

The current issue will be on dependent on it and no further work should be done unless we have the tests cleared.
History
Date User Action Args
2015-12-14 16:40:08maciej.szulikunlinkissue25591 superseder
2015-12-14 16:35:47maciej.szuliksetkeywords: - patch

messages: + msg256393
2015-12-13 19:54:07r.david.murraysetmessages: + msg256331
2015-12-07 21:36:41maciej.szuliklinkissue25591 superseder
2015-12-07 21:32:50maciej.szuliksetfiles: + new_imap_tests.diff
hgrepos: + hgrepo325
messages: + msg256072
2015-04-22 07:49:40maciej.szuliksetmessages: + msg241784
2015-04-22 04:29:47zvynsetmessages: + msg241770
2015-04-15 21:58:21maciej.szuliksetmessages: + msg241169
2015-04-14 22:14:07maciej.szuliksetmessages: + msg241043
2015-04-14 20:41:14maciej.szuliksetnosy: + maciej.szulik
messages: + msg241016
2014-08-05 18:46:16zvynsetfiles: + imaplib_test_rfc3501v2.patch

messages: + msg224873
2014-08-05 17:54:49pitrousetmessages: + msg224865
2014-08-05 00:17:13zvyncreate