Created on 2012-08-28 21:50 by serhiy.storchaka, last changed 2012-09-01 11:31 by petri.lehtinen. This issue is now closed.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| test_mailbox_create_tmp.patch | serhiy.storchaka, 2012-08-28 21:50 | Test for 3.2 and 3.3 | review | |
| test_mailbox_create_tmp-2.7.patch | serhiy.storchaka, 2012-08-28 21:50 | Patch for 2.7 | review | |
| Messages (8) | |||
|---|---|---|---|
| msg169307 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) *  |
Date: 2012-08-28 21:50 | |
Test for mailbox contains meaningless asserts. Here is a patch that corrects testing, if I correctly understand it. |
|||
| msg169308 - (view) | Author: R. David Murray (r.david.murray) * |
Date: 2012-08-28 21:57 | |
Looks right to me on a quick scan, but I'm not familiar with that test. |
|||
| msg169349 - (view) | Author: Ezio Melotti (ezio.melotti) * |
Date: 2012-08-29 07:53 | |
+ if int(groups[0]) == int(previous_groups[0]): + self.assertGreaterEqual(int(groups[1]), int(previous_groups[1]), This checks that int(groups[1]) >= int(previous_groups[1]) if int(groups[0]) == int(previous_groups[0]) whereas the previous version (with the int() fixed) checked that int(groups[1]) >= (previous_groups[1]) or groups[0] != groups[1]. Was the previous check nonsensical apart from the wrong usage of int()? Note that even the indexes you used are different (I haven't checked what those values actually are though). |
|||
| msg169351 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2012-08-29 08:46 | |
Yes, the previous check nonsensical in two cases -- comparing strings and comparing with wrong value. groups[0] -- current seconds (str), groups[1] -- current milliseconds (str), previous_groups[0] -- previous seconds (str), previous_groups[1] -- previous milliseconds (str). As I understand sensible check should be: current seconds >= previous seconds and if current seconds == previous seconds then current milliseconds >= previous milliseconds. In other words, (int(groups[0]), int(groups[1])) >= (int(previous_groups[0]), int(previous_groups[1])). |
|||
| msg169362 - (view) | Author: Chris Jerdonek (chris.jerdonek) * |
Date: 2012-08-29 10:39 | |
> In other words, (int(groups[0]), int(groups[1])) >= (int(previous_groups[0]), int(previous_groups[1])). Why not use a single self.assertGreaterEqual() on the pairs (with an appropriate change in the message)? It currently hand-codes the lexicographic comparison, which seems an unnecessary complication (and may have increased the likelihood of error). |
|||
| msg169621 - (view) | Author: Petri Lehtinen (petri.lehtinen) * |
Date: 2012-09-01 11:10 | |
Techincally, converting to int is not necessary, because the number of digits in the unix timestamp doesn't change until year 2286 :) The patch looks good to me. I don't think comparing pairs would be any more readable than what the proposed patch does. |
|||
| msg169624 - (view) | Author: Roundup Robot (python-dev) | Date: 2012-09-01 11:30 | |
New changeset aef4a2ba3210 by Petri Lehtinen in branch '3.2': #15802: Fix test logic in TestMaildir.test_create_tmp http://hg.python.org/cpython/rev/aef4a2ba3210 New changeset 2370e331241b by Petri Lehtinen in branch '2.7': #15802: Fix test logic in TestMaildir.test_create_tmp http://hg.python.org/cpython/rev/2370e331241b New changeset e2fec0144bf8 by Petri Lehtinen in branch 'default': #15802: Fix test logic in TestMaildir.test_create_tmp http://hg.python.org/cpython/rev/e2fec0144bf8 |
|||
| msg169625 - (view) | Author: Petri Lehtinen (petri.lehtinen) * |
Date: 2012-09-01 11:31 | |
Fixed, thanks. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2012-09-01 11:31:53 | petri.lehtinen | set | status: open -> closed resolution: fixed messages: + msg169625 stage: commit review -> resolved |
| 2012-09-01 11:30:45 | python-dev | set | nosy:
+ python-dev messages: + msg169624 |
| 2012-09-01 11:10:36 | petri.lehtinen | set | assignee: petri.lehtinen messages: + msg169621 stage: commit review |
| 2012-08-29 10:39:06 | chris.jerdonek | set | nosy:
+ chris.jerdonek messages: + msg169362 |
| 2012-08-29 08:46:26 | serhiy.storchaka | set | messages:
+ msg169351 title: Illegal test for mailbox -> Nonsensical test for mailbox |
| 2012-08-29 07:53:33 | ezio.melotti | set | nosy:
+ ezio.melotti messages: + msg169349 |
| 2012-08-29 00:47:57 | jcea | set | nosy:
+ jcea |
| 2012-08-28 22:01:58 | akuchling | set | nosy:
- akuchling |
| 2012-08-28 21:57:11 | r.david.murray | set | nosy:
+ petri.lehtinen messages: + msg169308 |
| 2012-08-28 21:50:51 | serhiy.storchaka | set | files: + test_mailbox_create_tmp-2.7.patch |
| 2012-08-28 21:50:08 | serhiy.storchaka | create | |