bpo-30308: Code coverage for argument in random.shuffle#1504
bpo-30308: Code coverage for argument in random.shuffle#1504rhettinger merged 3 commits intopython:masterfrom
Conversation
Lib/test/test_random.py
Outdated
| self.assertRaises(TypeError, shuffle, (1, 2, 3)) | ||
| self.assertRaises(TypeError, shuffle, {1, 2, 3}) | ||
| self.assertRaises(TypeError, shuffle, 'shuffle') | ||
| self.assertRaises(KeyError, shuffle, dict(a=1, b=2)) |
There was a problem hiding this comment.
Let's only keep the first of these four tests. It is reasonable to check to see if an immutable argument raises a TypeError. The rest are either repeats of the same concept or an over-specification (what the current code happens to do rather than what is guaranteed).
Lib/test/test_random.py
Outdated
| def test_shuffle_dict_with_numeric_keys(self): | ||
| shuffle = self.gen.shuffle | ||
| seq = dict(zip(range(9), 'abcdefghi')) | ||
| self.assertRaises(KeyError, shuffle, seq) |
There was a problem hiding this comment.
I think this isn't a valid test and should be dropped. It is an effect incidental to the implementation (due to duck-typing).
Lib/test/test_random.py
Outdated
| mock_random = unittest.mock.Mock(return_value=0.5) | ||
| seq = bytearray(b'abcdefghijk') | ||
| shuffle(seq, mock_random) | ||
| self.assertEqual(seq, b'ajbhcgdiekf') |
There was a problem hiding this comment.
Let's drop line 169. We're not guaranteeing a particular shuffle order. Instead, we're just checking the shuffle worked at all when given a shuffle argument.
rhettinger
left a comment
rhettinger
left a comment
There was a problem hiding this comment.
Very little needs to be added to the shuffle() tests. We need to check to make sure it doesn't fail when given a random argument and that it raises a TypeError for an immutable input. Everything else are mostly incidental implementation details (nothing that we want to guarantee or even care about). We try to write tests that would matter if they failed at some point in the future if shuffle() gets rewritten.
|
Thank you for taking the time to explain the reasons for what to include and not include. The things I was wondering about make much more sense now. I really have to keep in mind that simple is better. :-) |
rhettinger
left a comment
rhettinger
left a comment
There was a problem hiding this comment.
Looks great. Thank you.
4 participants
Add code coverage tests for optional parameter
randomin random.shuffle.