Fix MessageCache slicing bugs, improve tests

2 files changed, 41 insertions, 26 deletions

diff --git a/bot/utils/message_cache.py b/bot/utils/message_cache.py
index b2f8f66bf..67da8ecf3 100644
--- a/bot/utils/message_cache.py
+++ b/bot/utils/message_cache.py
@@ -142,20 +142,29 @@ class MessageCache:
 
             # Having empty cells is an implementation detail. To the user the cache contains as many elements as they
             # inserted, therefore any empty cells should be ignored. There can only be Nones at the tail.
-            if (
-                (self._start < self._end and not self._start < stop <= self._end)
-                or (self._start > self._end and self._end < stop <= self._start)
-            ):
-                stop = self._end
+            if step > 0:
+                if (
+                    (self._start < self._end and not self._start < stop <= self._end)
+                    or (self._start > self._end and self._end < stop <= self._start)
+                ):
+                    stop = self._end
+            else:
+                lower_boundary = (self._start - 1) % self.maxlen
+                if (
+                    (self._start < self._end and not self._start - 1 <= stop < self._end)
+                    or (self._start > self._end and self._end < stop < lower_boundary)
+                ):
+                    stop = lower_boundary
 
             if (start < stop and step > 0) or (start > stop and step < 0):
                 return self._messages[start:stop:step]
             # step != 1 may require a start offset in the second slicing.
             if step > 0:
                 offset = ceil((self.maxlen - start) / step) * step + start - self.maxlen
+                return self._messages[start::step] + self._messages[offset:stop:step]
             else:
-                offset = self.maxlen - ((start + 1) % step)
-            return self._messages[start::step] + self._messages[offset:stop:step]
+                offset = ceil((start + 1) / -step) * -step - start - 1
+                return self._messages[start::step] + self._messages[self.maxlen - 1 - offset:stop:step]
 
         else:
             raise TypeError(f"cache indices must be integers or slices, not {type(item)}")
diff --git a/tests/bot/utils/test_message_cache.py b/tests/bot/utils/test_message_cache.py
index ff313c6d6..5e871cd19 100644
--- a/tests/bot/utils/test_message_cache.py
+++ b/tests/bot/utils/test_message_cache.py
@@ -163,40 +163,46 @@ class TestMessageCache(unittest.TestCase):
 
     def test_slicing_with_unfilled_cache(self):
         """Test if slicing returns the correct messages if the cache is not yet fully filled."""
-        cache = MessageCache(maxlen=5)
-        messages = [MockMessage() for _ in range(4)]
-
-        for msg in messages:
-            cache.append(msg)
+        sizes = (5, 10, 55, 101)
 
-        test_cases = (
+        slices = (
             slice(None), slice(2, None), slice(None, 2), slice(None, None, 2), slice(None, None, 3), slice(-1, 2),
             slice(-1, 3000), slice(-3, -1), slice(-10, 3), slice(-10, 4, 2), slice(None, None, -1), slice(None, 3, -2),
             slice(None, None, -3)
         )
 
-        for current_loop in test_cases:
-            with self.subTest(current_loop=current_loop):
-                self.assertListEqual(cache[current_loop], messages[current_loop])
+        for size in sizes:
+            cache = MessageCache(maxlen=size)
+            messages = [MockMessage() for _ in range(size // 3 * 2)]
+
+            for msg in messages:
+                cache.append(msg)
+
+            for slice_ in slices:
+                with self.subTest(current_loop=(size, slice_)):
+                    self.assertListEqual(cache[slice_], messages[slice_])
 
     def test_slicing_with_overfilled_cache(self):
         """Test if slicing returns the correct messages if the cache was appended with more messages it can contain."""
-        cache = MessageCache(maxlen=5)
-        messages = [MockMessage() for _ in range(8)]
-
-        for msg in messages:
-            cache.append(msg)
-        messages = messages[3:]
+        sizes = (5, 10, 55, 101)
 
-        test_cases = (
+        slices = (
             slice(None), slice(2, None), slice(None, 2), slice(None, None, 2), slice(None, None, 3), slice(-1, 2),
             slice(-1, 3000), slice(-3, -1), slice(-10, 3), slice(-10, 4, 2), slice(None, None, -1), slice(None, 3, -2),
             slice(None, None, -3)
         )
 
-        for current_loop in test_cases:
-            with self.subTest(current_loop=current_loop):
-                self.assertListEqual(cache[current_loop], messages[current_loop])
+        for size in sizes:
+            cache = MessageCache(maxlen=size)
+            messages = [MockMessage() for _ in range(size * 3 // 2)]
+
+            for msg in messages:
+                cache.append(msg)
+            messages = messages[size // 2:]
+
+            for slice_ in slices:
+                with self.subTest(current_loop=(size, slice_)):
+                    self.assertListEqual(cache[slice_], messages[slice_])
 
     def test_length(self):
         """Test if len returns the correct number of items in the cache."""