This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Add code coverage for argument in random.shuffle
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: cheryl.sabella, mark.dickinson, rhettinger, vstinner
Priority: normal Keywords:

Created on 2017-05-08 21:20 by cheryl.sabella, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 1504 merged cheryl.sabella, 2017-05-08 21:23
Messages (9)
msg293253 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-05-08 21:20
Added test cases for `random` argument in random.shuffle for code coverage.

Two interesting results (documented within test cases):
1.  In the docs, `random` is defined as a function returning a float in the range [0, 1), but negative floats could be returned and shuffle worked using slice notation.
2.  A dictionary with sequential numeric keys could be shuffled.
msg293254 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-05-08 21:26
Add Mark Dickinson and Raymond Hettinger to nosy list per Brett's suggestion on core-mentorship.
msg293259 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-05-08 23:02
* Why not use unittest.mock to test the *random* argument for shuffle?

* Try to avoid direct calls to _randbelow().  That method is a non-public implementation detail subject to change.

* Also please don't make test_random.py depend on secrets which itself depends on random.py.
msg293286 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-05-09 07:33
I see you've added a test for the behaviour of:

    shuffle(seq, random=lambda: -1.0)

I'd suggest leaving that test out: that this works right now is really just an accident of the (CPython) implementation, and it may well not work on other Python implementations. Adding a test in effect says that there was a deliberate decision to support this (which as far as I know there isn't, and shouldn't be).
msg293288 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-05-09 07:58
I'm also a bit uncomfortable with adding tests for other specific exceptions when a misbehaving `random` is passed: again, the details of whether an exception is raised or not for "self.assertRaises(IndexError, shuffle, seq, random=lambda: -1.1)", and which precise class of exception is raised, seem like something that could be left as implementation-defined. I'm not sure I see much value in pinning down the precise behaviour in this case (which is effectively what a testcase does).
msg293359 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-05-09 23:03
OK, I've removed the test for the negative values returned by random.  In core-mentorship, David Murray had suggested adding a comment to document it as unexpected behavior.  I had asked there because I wasn't quite sure what to do about it.

Should I also leave out the test for being able to shuffle a dict with numeric keys?

Thanks.
msg293360 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-05-09 23:08
I've also changed the test to use Mock for the random function.  I'm new to mock, so I'm not sure if I did it right.  Using a return_value seemed to be the best way to go.  I didn't know if it was necessary to test if an incorrect function was sent in.
msg293498 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-05-11 15:19
New changeset f111fd2e65ef7aefd4ebeadbb48e84d609bf3733 by Raymond Hettinger (csabella) in branch 'master':
bpo-30308: Code coverage for argument in random.shuffle (#1504)
https://github.com/python/cpython/commit/f111fd2e65ef7aefd4ebeadbb48e84d609bf3733
msg293499 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2017-05-11 15:20
Thank you for the patch.
History
Date User Action Args
2022-04-11 14:58:46adminsetgithub: 74494
2017-05-11 15:20:37rhettingersetstatus: open -> closed
resolution: fixed
messages: + msg293499

stage: resolved
2017-05-11 15:19:39rhettingersetmessages: + msg293498
2017-05-09 23:08:01cheryl.sabellasetmessages: + msg293360
2017-05-09 23:03:04cheryl.sabellasetmessages: + msg293359
2017-05-09 07:58:18mark.dickinsonsetmessages: + msg293288
2017-05-09 07:57:22vstinnersetnosy: + vstinner
2017-05-09 07:33:24mark.dickinsonsetmessages: + msg293286
2017-05-08 23:02:32rhettingersetmessages: + msg293259
2017-05-08 22:58:21rhettingersetassignee: rhettinger
2017-05-08 21:26:16cheryl.sabellasetnosy: + rhettinger, mark.dickinson
messages: + msg293254
2017-05-08 21:23:30cheryl.sabellasetpull_requests: + pull_request1604
2017-05-08 21:20:00cheryl.sabellacreate