classification
Title: Nonsensical test for mailbox
Type: behavior Stage: resolved
Components: Tests Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: petri.lehtinen Nosy List: barry, chris.jerdonek, ezio.melotti, jcea, petri.lehtinen, python-dev, r.david.murray, serhiy.storchaka
Priority: normal Keywords: patch

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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2012-09-01 11:31
Fixed, thanks.
History
Date User Action Args
2012-09-01 11:31:53petri.lehtinensetstatus: open -> closed
resolution: fixed
messages: + msg169625

stage: commit review -> resolved
2012-09-01 11:30:45python-devsetnosy: + python-dev
messages: + msg169624
2012-09-01 11:10:36petri.lehtinensetassignee: petri.lehtinen
messages: + msg169621
stage: commit review
2012-08-29 10:39:06chris.jerdoneksetnosy: + chris.jerdonek
messages: + msg169362
2012-08-29 08:46:26serhiy.storchakasetmessages: + msg169351
title: Illegal test for mailbox -> Nonsensical test for mailbox
2012-08-29 07:53:33ezio.melottisetnosy: + ezio.melotti
messages: + msg169349
2012-08-29 00:47:57jceasetnosy: + jcea
2012-08-28 22:01:58akuchlingsetnosy: - akuchling
2012-08-28 21:57:11r.david.murraysetnosy: + petri.lehtinen
messages: + msg169308
2012-08-28 21:50:51serhiy.storchakasetfiles: + test_mailbox_create_tmp-2.7.patch
2012-08-28 21:50:08serhiy.storchakacreate