classification
Title: Silently skipped test in test_random
Type: behavior Stage: resolved
Components: Tests Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: zach.ware Nosy List: Julian.Gindi, ezio.melotti, python-dev, rhettinger, serhiy.storchaka, tim.peters, zach.ware
Priority: normal Keywords: easy, patch

Created on 2013-11-14 20:34 by zach.ware, last changed 2013-11-26 21:01 by zach.ware. This issue is now closed.

Files
File name Uploaded Description Edit
issue19588.patch Julian.Gindi, 2013-11-19 22:30 review
issue19588_v2.patch Julian.Gindi, 2013-11-21 04:58 review
Messages (13)
msg202879 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-11-14 20:34
See: http://hg.python.org/cpython/file/87099/Lib/test/test_random.py#l241

This test (and its match in MersenneTwister_TestBasicOps) is nearly always skipped by the 'if stop <= start: return' check; in a test with adding "print('skipped', i)" before the return and running test_random via regrtest with -F, i was 40 when the test returned about 21 out of 25 times.  It seems to have been this way since the test was added.

Was this intended?  It looks to me like perhaps the start and stop assignments are swapped; Serhiy suggested that perhaps stop was meant to have been added to start.  How is this test meant to work?
msg202895 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2013-11-14 21:38
Nice catch!  That's insane.  `start` and `stop` should indeed be swapped, *and* the `return` should be `continue`.  I didn't write the test, but these things are obvious to my eyeballs ;-)
msg203438 - (view) Author: Julian Gindi (Julian.Gindi) * Date: 2013-11-19 22:30
Maybe some documentation could help express the purpose of this test, but I was able to make the changes mentioned below and the test seems to work better than it used to. The test no longer returns if a value is 'skipped'. This is my first attempt at a patch so apologies if this is not perfect.
msg203570 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-11-21 04:02
The patch looks good to me, Julian.  Have you signed a contributor agreement?  If you haven't done so yet and are planning on contributing anything more than the most trivial of changes, you'll need to do so (see http://www.python.org/psf/contrib/).
msg203571 - (view) Author: Julian Gindi (Julian.Gindi) * Date: 2013-11-21 04:06
Awesome! I signed the contributor agreement today via "E-sign". I look forward to making more significant contributions soon :)
msg203572 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-11-21 04:11
Hmm, actually, I take it back...this is half of the needed patch :).  There's another test method of the same name in MersenneTwister_TestBasicOps, with the same issue.  But this half looks good!  I'll leave a comment on Rietveld (which will send you a 'review' email) pointing out the other test.
msg203573 - (view) Author: Tim Peters (tim.peters) * (Python committer) Date: 2013-11-21 04:38
Yup, the patch is semantically correct.  But I'd also swap the order of the `start =` and `stop =` lines - *everyone* expects `start` to be set first ;-)
msg203574 - (view) Author: Julian Gindi (Julian.Gindi) * Date: 2013-11-21 04:58
That makes perfect sense :) Here is an updated patch. I also made the change to the other test of the same name in MersenneTwister_TestBasicOps
msg203582 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-11-21 08:09
Original test was added in issue812202.
msg204487 - (view) Author: Julian Gindi (Julian.Gindi) * Date: 2013-11-26 15:15
Just wanted to see if there was anything else I needed to do to get this patch rolling :)
msg204496 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-11-26 16:03
Nope, your patch looks good, I just haven't gotten it committed yet :)
msg204537 - (view) Author: Roundup Robot (python-dev) Date: 2013-11-26 20:59
New changeset c8e138646be1 by Zachary Ware in branch '2.7':
Issue #19588: Fixed tests in test_random that were silently skipped most
http://hg.python.org/cpython/rev/c8e138646be1

New changeset c65882d79c5f by Zachary Ware in branch '3.3':
Issue #19588: Fixed tests in test_random that were silently skipped most
http://hg.python.org/cpython/rev/c65882d79c5f

New changeset 28ec217ce510 by Zachary Ware in branch 'default':
Issue #19588: Merge with 3.3
http://hg.python.org/cpython/rev/28ec217ce510
msg204538 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2013-11-26 21:01
Thanks for the patch, Julian!
History
Date User Action Args
2013-11-26 21:01:29zach.waresetstatus: open -> closed
messages: + msg204538

assignee: zach.ware
resolution: fixed
stage: patch review -> resolved
2013-11-26 20:59:18python-devsetnosy: + python-dev
messages: + msg204537
2013-11-26 16:03:53zach.waresetmessages: + msg204496
2013-11-26 15:15:54Julian.Gindisetmessages: + msg204487
2013-11-21 08:09:25serhiy.storchakasetmessages: + msg203582
2013-11-21 04:58:09Julian.Gindisetfiles: + issue19588_v2.patch

messages: + msg203574
2013-11-21 04:38:53tim.peterssetmessages: + msg203573
2013-11-21 04:11:06zach.waresetmessages: + msg203572
2013-11-21 04:06:11Julian.Gindisetmessages: + msg203571
2013-11-21 04:02:47zach.waresetmessages: + msg203570
stage: needs patch -> patch review
2013-11-19 22:30:10Julian.Gindisetfiles: + issue19588.patch

nosy: + Julian.Gindi
messages: + msg203438

keywords: + patch
2013-11-16 21:07:36ezio.melottisetkeywords: + easy
nosy: + ezio.melotti

stage: needs patch
2013-11-14 21:38:03tim.peterssetnosy: + tim.peters
messages: + msg202895
2013-11-14 20:34:48zach.warecreate