From dabca794ed0b60be4de2f04b72d3eb3c57377842 Mon Sep 17 00:00:00 2001 From: John McCardle Date: Thu, 2 Jul 2026 20:13:18 -0400 Subject: [PATCH] Add tests pinning EntityCollection semantics and O(1) indexing (#329) Pre-change TDD tests for the std::list -> std::vector container swap: - tests/unit/entity_collection_mutation_test.py pins observable behavior: positive/negative indexing, out-of-range IndexError, slicing (returns a fresh list; slice wrappers are not cache-identity), in-order iteration, RuntimeError on size change during iteration, and grid.step() immunity to mid-step entity self-removal. - tests/regression/issue_329_entity_index_perf_test.py guards against a regression to O(n) indexed access / O(n^2) full-scan. Both pass against the pre-swap std::list implementation, locking in the public contract before the internal container change. Co-Authored-By: Claude Fable 5 --- .../issue_329_entity_index_perf_test.py | 83 ++++++++ tests/unit/entity_collection_mutation_test.py | 190 ++++++++++++++++++ 2 files changed, 273 insertions(+) create mode 100644 tests/regression/issue_329_entity_index_perf_test.py create mode 100644 tests/unit/entity_collection_mutation_test.py diff --git a/tests/regression/issue_329_entity_index_perf_test.py b/tests/regression/issue_329_entity_index_perf_test.py new file mode 100644 index 0000000..75406f7 --- /dev/null +++ b/tests/regression/issue_329_entity_index_perf_test.py @@ -0,0 +1,83 @@ +#!/usr/bin/env python3 +""" +Regression test for issue #329: grid.entities[i] must be O(1), not O(n). + +UIEntityCollection previously wrapped std::list and did std::advance() from +begin() on every indexed access, making grid.entities[i] O(n) and a full +"for i in range(len(e)): e[i]" loop O(n^2). After switching the backing +container to std::vector, indexed access is O(1). + +This test builds a large entity collection and times a full indexed pass. +With the O(n^2) implementation this pass grows quadratically; with the O(n) +implementation it stays linear. We use a generous wall-clock bound so CI +noise cannot flake it -- the point is to catch a catastrophic regression to +quadratic behavior, not to microbenchmark. +""" + +import mcrfpy +import sys +import time + +N = 5000 +# Generous bound: an O(1)-per-index vector pass finishes in tens of ms. +# The old O(n^2) list pass over 5000 entities does ~12.5M node advances. +WALL_CLOCK_BUDGET = 2.0 # seconds + + +def main(): + scene = mcrfpy.Scene("issue_329_perf") + grid = mcrfpy.Grid(grid_size=(200, 200), pos=(0, 0), size=(400, 400)) + scene.children.append(grid) + entities = grid.entities + + for i in range(N): + entities.append(mcrfpy.Entity((i % 200, (i // 200) % 200))) + + assert len(entities) == N, "expected %d entities, got %d" % (N, len(entities)) + + # Full indexed pass -- this is the O(n^2) hot path in the old code. + start = time.perf_counter() + total = 0 + for i in range(N): + e = entities[i] + if e is not None: + total += 1 + elapsed = time.perf_counter() - start + + print("indexed %d entities in %.4f s (budget %.1f s)" % + (N, elapsed, WALL_CLOCK_BUDGET)) + assert total == N, "expected to visit %d entities, visited %d" % (N, total) + + if elapsed >= WALL_CLOCK_BUDGET: + print("FAIL: indexed pass took %.4f s (>= %.1f s) -- likely O(n^2)" + % (elapsed, WALL_CLOCK_BUDGET)) + return False + + # Sanity: accessing the last element should not be dramatically slower + # than accessing the first (O(1) random access). + t0 = time.perf_counter() + for _ in range(2000): + _ = entities[0] + first_time = time.perf_counter() - t0 + + t0 = time.perf_counter() + for _ in range(2000): + _ = entities[N - 1] + last_time = time.perf_counter() - t0 + + print("2000x first-index: %.4f s, 2000x last-index: %.4f s" + % (first_time, last_time)) + # With a list, last_time would be ~N times first_time. Allow a very + # generous constant-factor slack (20x) to stay noise-proof. + if last_time > (first_time + 0.01) * 20: + print("FAIL: last-index access %.4f s vastly exceeds first-index " + "%.4f s -- indexed access is not O(1)" % (last_time, first_time)) + return False + + print("PASS") + return True + + +if __name__ == "__main__": + ok = main() + sys.exit(0 if ok else 1) diff --git a/tests/unit/entity_collection_mutation_test.py b/tests/unit/entity_collection_mutation_test.py new file mode 100644 index 0000000..88fa6de --- /dev/null +++ b/tests/unit/entity_collection_mutation_test.py @@ -0,0 +1,190 @@ +#!/usr/bin/env python3 +""" +Semantics test for issue #329: grid.entities backed by std::vector. + +Pins the observable behavior of the EntityCollection sequence after the +container swap from std::list to std::vector: + + * indexing (positive and negative) + * out-of-range IndexError + * slicing returns a plain list + * iteration yields every element in order + * mutation during iteration raises RuntimeError (size-guard / generation + check) -- iteration is index-based, so this is well-defined and can + never corrupt memory even in the remove+add "same size" case + * grid.step() turn processing is immune to entity death mid-step + (it snapshots into a vector internally) + +These behaviors match the pre-#329 std::list implementation; the swap is a +purely internal container change. +""" + +import mcrfpy +import sys + +results = [] + + +def check(name, cond): + results.append((name, bool(cond))) + print((" ok " if cond else " FAIL") + " : " + name) + + +def make_grid(name, count): + scene = mcrfpy.Scene(name) + grid = mcrfpy.Grid(grid_size=(50, 50), pos=(0, 0), size=(400, 400)) + scene.children.append(grid) + ents = grid.entities + made = [] + for i in range(count): + e = mcrfpy.Entity((i, i)) + ents.append(e) + made.append(e) + return grid, ents, made + + +def test_indexing(): + grid, ents, made = make_grid("m_idx", 5) + check("len == 5", len(ents) == 5) + check("positive index [0] is first", ents[0] is not None) + # identity is preserved through the cache: the same Entity comes back + check("index preserves identity", ents[2] is made[2]) + check("negative index [-1] == [4]", ents[-1] is made[4]) + check("negative index [-5] == [0]", ents[-5] is made[0]) + + +def test_out_of_range(): + grid, ents, made = make_grid("m_oob", 3) + got = False + try: + _ = ents[3] + except IndexError: + got = True + check("index 3 raises IndexError", got) + + got = False + try: + _ = ents[-4] + except IndexError: + got = True + check("index -4 raises IndexError", got) + + +def _same_entity(a, b): + # Slicing returns fresh Entity wrappers (it does not consult the identity + # cache), so compare the underlying entity by its distinguishing position + # rather than Python object identity. Each entity i was created at (i, i). + return a.pos.x == b.pos.x and a.pos.y == b.pos.y + + +def test_slicing(): + grid, ents, made = make_grid("m_slice", 6) + sl = ents[1:4] + check("slice returns list", isinstance(sl, list)) + check("slice length correct", len(sl) == 3) + check("slice contents in order", + _same_entity(sl[0], made[1]) and _same_entity(sl[1], made[2]) + and _same_entity(sl[2], made[3])) + + sl2 = ents[::2] + check("extended slice length", len(sl2) == 3) + check("extended slice contents", + _same_entity(sl2[0], made[0]) and _same_entity(sl2[1], made[2]) + and _same_entity(sl2[2], made[4])) + + sl3 = ents[-2:] + check("negative slice contents", + len(sl3) == 2 and _same_entity(sl3[0], made[4]) + and _same_entity(sl3[1], made[5])) + + +def test_iteration_order(): + grid, ents, made = make_grid("m_iter", 5) + seen = [e for e in ents] + check("iteration visits all", len(seen) == 5) + check("iteration in order", + all(seen[i] is made[i] for i in range(5))) + + +def test_mutation_during_iteration(): + grid, ents, made = make_grid("m_mut", 5) + # Removing during iteration changes size -> well-defined RuntimeError. + raised = False + try: + for e in ents: + ents.remove(e) + except RuntimeError: + raised = True + check("remove during iteration raises RuntimeError", raised) + + # append during iteration also raises (size changed). + grid2, ents2, made2 = make_grid("m_mut2", 3) + raised = False + try: + for e in ents2: + ents2.append(mcrfpy.Entity((30, 30))) + break_now = len(ents2) > 6 # guard against infinite loop if no raise + if break_now: + break + except RuntimeError: + raised = True + check("append during iteration raises RuntimeError", raised) + + +def test_step_immune_to_death(): + # grid.step() must snapshot internally so an entity dying mid-step + # (removing itself from the collection) cannot invalidate the loop. + scene = mcrfpy.Scene("m_step") + grid = mcrfpy.Grid(grid_size=(20, 20), pos=(0, 0), size=(200, 200)) + scene.children.append(grid) + ents = grid.entities + + removed = [] + for i in range(5): + e = mcrfpy.Entity((i, 0)) + e.turn_order = 1 + # PATH behavior with an empty path resolves to DONE on the first + # step, which fires the `step` callback deterministically. + e.set_behavior(int(mcrfpy.Behavior.PATH)) + + def on_step(trigger, data, _e=e): + # On its turn, remove itself from the grid mid-step. + try: + grid.entities.remove(_e) + removed.append(_e) + except ValueError: + pass + + e.step = on_step + ents.append(e) + + before = len(ents) + # Must not crash even though each callback mutates the live collection + # while step() is iterating its internal snapshot. + grid.step(1) + after = len(ents) + check("grid.step() survived mid-step self-removal (len %d -> %d, " + "%d removed)" % (before, after, len(removed)), True) + # If callbacks fired (expected), entities were actually removed. + check("mid-step removals took effect", after == before - len(removed)) + + +def main(): + test_indexing() + test_out_of_range() + test_slicing() + test_iteration_order() + test_mutation_during_iteration() + test_step_immune_to_death() + + failed = [n for n, ok in results if not ok] + if failed: + print("\nFAIL: %d checks failed: %s" % (len(failed), ", ".join(failed))) + return False + print("\nPASS: all %d checks passed" % len(results)) + return True + + +if __name__ == "__main__": + ok = main() + sys.exit(0 if ok else 1)