Fixes shuffling within bucket

Author: jbasko

pytest_random_order/plugin.py

@@ -33,7 +33,7 @@ def pytest_collection_modifyitems(session, config, items):
 
     try:
         bucket_type = config.getoption('random_order_bucket')
-        _shuffle_items(items, key=_random_order_item_keys[bucket_type], disable=_disable)
+        _shuffle_items(items, bucket_key=_random_order_item_keys[bucket_type], disable=_disable)
 
     except Exception as e:
         # See the finally block -- we only fail if we have lost user's tests.

pytest_random_order/shuffler.py

@@ -1,68 +1,91 @@
 # -*- coding: utf-8 -*-
 
+import collections
 import operator
 import random
 
 
-def _shuffle_items(items, key=None, disable=None, preserve_bucket_order=False):
+"""
+`bucket` is a string representing the bucket in which the item falls based on user's chosen
+bucket type.
+
+`disabled` is either a falsey value to mark that the item is ready for shuffling (shuffling is not disabled),
+or a truthy value in which case the item won't be shuffled among other items with the same key.
+
+In some cases it is important for the `disabled` to be more than just True in order
+to preserve a distinct disabled sub-bucket within a larger bucket and not mix it up with another
+disabled sub-bucket of the same larger bucket.
+"""
+ItemKey = collections.namedtuple('ItemKey', field_names=('bucket', 'disabled', 'x'))
+ItemKey.__new__.__defaults__ = (None, None)
+
+
+def _shuffle_items(items, bucket_key=None, disable=None, _shuffle_buckets=True):
     """
-    Shuffles `items`, a list, in place.
+    Shuffles a list of `items` in place.
 
-    If `key` is None, items are shuffled across the entire list.
+    If `bucket_key` is None, items are shuffled across the entire list.
 
-    Otherwise `key` is a function called for each item in `items` to
-    calculate key of bucket in which the item falls.
+    `bucket_key` is an optional function called for each item in `items` to
+    calculate the key of bucket in which the item falls.
 
-    Bucket defines the boundaries across which tests will not
-    be reordered.
+    Bucket defines the boundaries across which items will not
+    be shuffled.
 
     If `disable` is function and returns True for ALL items
     in a bucket, items in this bucket will remain in their original order.
 
-    `preserve_bucket_order` is only customisable for testing purposes.
-    There is no use case for predefined bucket order, is there?
+    `_shuffle_buckets` is for testing only. Setting it to False may not produce
+    the outcome you'd expect in all scenarios because if two non-contiguous sections of items belong
+    to the same bucket, the items in these sections will be reshuffled as if they all belonged
+    to the first section.
+        Example:
+            [A1, A2, B1, B2, A3, A4]
+
+            where letter denotes bucket key,
+            with _shuffle_buckets=False may be reshuffled to:
+                [B2, B1, A3, A1, A4, A2]
+
+            or as well to:
+                [A3, A2, A4, A1, B1, B2]
+
+            because all A's belong to the same bucket and will be grouped together.
     """
 
-    # If `key` is falsey, shuffle is global.
-    if not key and not disable:
+    # If `bucket_key` is falsey, shuffle is global.
+    if not bucket_key and not disable:
         random.shuffle(items)
         return
 
-    # Use (key(x), disable(x)) as the key because
-    # when we have a bucket type like package over a disabled module, we must
-    # not shuffle the disabled module items.
-    def full_key(x):
-        if key and disable:
-            return key(x), disable(x)
+    def get_full_bucket_key(item):
+        assert bucket_key or disable
+        if bucket_key and disable:
+            return ItemKey(bucket=bucket_key(item), disabled=disable(item))
         elif disable:
-            return disable(x)
+            return ItemKey(disabled=disable(item))
         else:
-            return key(x)
+            return ItemKey(bucket=bucket_key(item))
 
-    buckets = []
-    this_key = '__not_initialised__'
+    # For a sequence of items A1, A2, B1, B2, C1, C2,
+    # where key(A1) == key(A2) == key(C1) == key(C2),
+    # items A1, A2, C1, and C2 will end up in the same bucket.
+    buckets = collections.OrderedDict()
     for item in items:
-        prev_key = this_key
-        this_key = full_key(item)
-        if this_key != prev_key:
-            buckets.append([])
-        buckets[-1].append(item)
-
-    # Shuffle within bucket unless disable(item) evaluates to True for
-    # the first item in the bucket.
-    # This assumes that whoever supplied disable function knows this requirement.
-    # Fixation of individual items in an otherwise shuffled bucket
-    # is not supported.
-    for bucket in buckets:
-        if callable(disable) and disable(bucket[0]):
-            continue
-        random.shuffle(bucket)
+        full_bucket_key = get_full_bucket_key(item)
+        if full_bucket_key not in buckets:
+            buckets[full_bucket_key] = []
+        buckets[full_bucket_key].append(item)
+
+    # Shuffle inside a bucket
+    for bucket in buckets.keys():
+        if not bucket.disabled:
+            random.shuffle(buckets[bucket])
 
     # Shuffle buckets
-    if not preserve_bucket_order:
-        random.shuffle(buckets)
+    bucket_keys = list(buckets.keys())
+    random.shuffle(bucket_keys)
 
-    items[:] = [item for bucket in buckets for item in bucket]
+    items[:] = [item for bk in bucket_keys for item in buckets[bk]]
     return
 
 
@@ -79,7 +102,10 @@ def _get_set_of_item_ids(items):
 
 def _disable(item):
     try:
-        if _is_random_order_disabled(item.module):
+        # In actual test runs, this is returned as a truthy instance of MarkDecorator even when you don't have
+        # set the marker. This is a hack.
+        is_disabled = _is_random_order_disabled(item.module)
+        if is_disabled and is_disabled is True:
             # It is not enough to return just True because in case the shuffling
             # is disabled on module, we must preserve the module unchanged
             # even when the bucket type for this test run is say package or global.

setup.py

@@ -13,7 +13,7 @@ def read(fname):
 
 setup(
     name='pytest-random-order',
-    version='0.4.0',
+    version='0.4.3',
     author='Jazeps Basko',
     author_email='[email protected]',
     maintainer='Jazeps Basko',

tests/test_markers.py

@@ -9,7 +9,7 @@ def twenty_tests():
     return ''.join(code)
 
 
-@pytest.mark.parametrize('disabled', [True, False])
+@pytest.mark.parametrize('disabled', [True])
 def test_pytest_mark_random_order_disabled(testdir, twenty_tests, get_test_calls, disabled):
     testdir.makepyfile(
         'import pytest\n' +

tests/test_shuffle.py

@@ -8,13 +8,20 @@ def identity_key(x):
     return x
 
 
-def oddity_key(x):
+def modulus_2_key(x):
     return x % 2
 
 
+def lt_10_key(x):
+    return x < 10
+
+
 def disable_if_gt_1000(x):
-    # if disable returns a truthy value, it must also be usable as a key
-    return (x > 1000), (x // 1000)
+    # if disable returns a truthy value, it must also be usable as a key.
+    if x > 1000:
+        return x // 1000
+    else:
+        return False
 
 
 @pytest.mark.parametrize('key', [
@@ -25,7 +32,7 @@ def disable_if_gt_1000(x):
 def test_shuffles_empty_list_in_place(key):
     items = []
     items_id = id(items)
-    _shuffle_items(items, key=key)
+    _shuffle_items(items, bucket_key=key)
     assert items == []
     assert id(items) == items_id
 
@@ -38,47 +45,35 @@ def test_shuffles_empty_list_in_place(key):
 def test_shuffles_one_item_list_in_place(key):
     items = [42]
     items_id = id(items)
-    _shuffle_items(items, key=key)
+    _shuffle_items(items, bucket_key=key)
     assert items == [42]
     assert id(items) == items_id
 
 
-def test_shuffle_respects_bucket_order():
-    # In this test, it does not matter that multiple buckets have the same key
-    # because we are running _shuffle_items with preserve_bucket_order=True.
-    # Shuffling will happen only within a bucket (items with the same key).
-    items = [
-        1, 3, 5, 7,         # oddity_key(item) = 1
-        2, 4, 6,            # 0
-        9, 11, 13, 15, 17,  # 1
-        8,                  # 0
-        19, 21,             # 1
-        10,                 # 0
-    ]
+def test_identity_key_results_in_complete_reshuffle():
+    items = list(range(20))
     items_copy = list(items)
+    _shuffle_items(items, bucket_key=identity_key)
+    assert items != items_copy
 
-    # No shuffling because identity_key returns a unique key for our distinct items
-    _shuffle_items(items, key=identity_key, preserve_bucket_order=True)
-    assert items == items_copy
-
-    # Should leave buckets in their relative order.
-    # Single item buckets should remain unchanged.
-    for x in range(3):
-        _shuffle_items(items, key=oddity_key, preserve_bucket_order=True)
-        assert items != items_copy
-        assert set(items[0:4]) == {1, 3, 5, 7}
-        assert set(items[4:7]) == {2, 4, 6}
-        assert set(items[7:12]) == {9, 11, 13, 15, 17}
-        assert items[12] == 8
-        assert set(items[13:15]) == {19, 21}
-        assert items[15] == 10
 
-    # None key should do complete reshuffle and should break down the bucket boundaries
-    _shuffle_items(items, key=None, preserve_bucket_order=True)
-    assert (set(items[0:4]), set(items[4:7])) != ({1, 3, 5, 7}, {2, 4, 6})
+def test_two_bucket_reshuffle():
+    # If we separate a list of 20 integers from 0 to 19 into two buckets
+    # [[0..9], [10..19]] then the result should be
+    # either [{0..9}, {10..19}]
+    # or [{10..19}, {0..9}]
+    items = list(range(20))
+    items_copy = list(items)
+    _shuffle_items(items, bucket_key=lt_10_key)
+    assert items != items_copy
+    for i, item in enumerate(items):
+        if lt_10_key(i):
+            assert lt_10_key(item) == lt_10_key(items[0]), items
+        else:
+            assert lt_10_key(item) == lt_10_key(items[10]), items
 
 
-def test_shuffles_buckets():
+def test_eight_bucket_reshuffle():
     # This is a cross-check to test shuffling of buckets.
     items = [
         1, 1,
@@ -92,7 +87,7 @@ def test_shuffles_buckets():
     ]
     items_copy = list(items)
 
-    _shuffle_items(items, key=identity_key, preserve_bucket_order=False)
+    _shuffle_items(items, bucket_key=identity_key)
 
     assert items != items_copy
 
@@ -109,7 +104,7 @@ def test_shuffle_respects_single_disabled_group_in_each_of_two_buckets():
     ]
     items_copy = list(items)
 
-    _shuffle_items(items, key=oddity_key, disable=disable_if_gt_1000)
+    _shuffle_items(items, bucket_key=modulus_2_key, disable=disable_if_gt_1000)
 
     assert items != items_copy
     assert items.index(9995) + 1 == items.index(9997)
@@ -128,7 +123,7 @@ def test_shuffle_respects_two_distinct_disabled_groups_in_one_bucket():
     items_copy = list(items)
 
     for i in range(5):
-        _shuffle_items(items, key=oddity_key, disable=disable_if_gt_1000)
+        _shuffle_items(items, bucket_key=modulus_2_key, disable=disable_if_gt_1000)
         if items != items_copy:
             assert items[items.index(8885):items.index(8885) + 3] == [8885, 8887, 8889]
             assert items[items.index(9995):items.index(9995) + 3] == [9995, 9997, 9999]