From 12f6eac80c69511a3028fda9e85d14c95cb65c63 Mon Sep 17 00:00:00 2001 From: Mike Stitt Date: Wed, 29 Jul 2026 22:55:22 -0400 Subject: [PATCH] Run isolated 'robotpy test' tests in pytest marker order IsolatedTestsPlugin pulled every isolated robot-fixture test ahead of the rest, which discarded the order pytest-order established during collection. Tests using @pytest.mark.order silently ran in the wrong sequence. The run loop now walks session.items in order and drains in-flight subprocesses at ordering boundaries. Unordered tests are untouched and still run isolated and in parallel. - Group tests by order-marker identity, so a class or module marker serialises the group against everything outside it without serialising its interior - Pass -p no:order to the isolated subprocess, which otherwise warns about markers referencing tests it cannot see - Add pytest-order to dependencies and tests/requirements.txt --- subprojects/robotpy-wpilib/pyproject.toml | 1 + .../robotpy-wpilib/tests/requirements.txt | 1 + .../tests/test_pytest_plugins.py | 533 ++++++++++++++++++ .../testing/pytest_isolated_tests_plugin.py | 84 ++- 4 files changed, 600 insertions(+), 19 deletions(-) diff --git a/subprojects/robotpy-wpilib/pyproject.toml b/subprojects/robotpy-wpilib/pyproject.toml index 2677094e5..d232124c6 100644 --- a/subprojects/robotpy-wpilib/pyproject.toml +++ b/subprojects/robotpy-wpilib/pyproject.toml @@ -31,6 +31,7 @@ dependencies = [ # For running robot tests "pytest>=3.9", + "pytest-order", "pytest-reraise", ] diff --git a/subprojects/robotpy-wpilib/tests/requirements.txt b/subprojects/robotpy-wpilib/tests/requirements.txt index e079f8a60..114768e8b 100644 --- a/subprojects/robotpy-wpilib/tests/requirements.txt +++ b/subprojects/robotpy-wpilib/tests/requirements.txt @@ -1 +1,2 @@ pytest +pytest-order diff --git a/subprojects/robotpy-wpilib/tests/test_pytest_plugins.py b/subprojects/robotpy-wpilib/tests/test_pytest_plugins.py index 40326e880..d0f50512d 100644 --- a/subprojects/robotpy-wpilib/tests/test_pytest_plugins.py +++ b/subprojects/robotpy-wpilib/tests/test_pytest_plugins.py @@ -432,3 +432,536 @@ def test_state_transitions(robot, control): ) result.assert_outcomes(passed=1) + + +# Seconds a chain's sentinel writer sleeps before writing, when the test that reads that +# sentinel runs in an isolated subprocess. +# +# Why this is needed at all: a plain in-process test writes its sentinel within milliseconds, +# while a robot test needs a few hundred ms to spawn its subprocess and reach its first +# assertion. Without ordering support the robot test is merely *started* early, so by the time +# it actually looks, a fast writer has already written -- the case passes and proves nothing. +# Delaying the writer removes that race so the case fails without the fix, which is the only +# thing that makes it a guard. +# +# Choosing the value: sweeping the delay against the unfixed plugin put the cutoff between +# 150ms and 200ms on an M-series Mac (0/5 runs caught the bug at 150ms, 5/5 at 200ms). 1s +# leaves roughly 5x margin. +# +# The risk this carries: on a machine slow enough that a robot subprocess takes over a second +# to reach its assertion, these cases quietly stop catching regressions. They never fail +# wrongly -- with ordering support present they pass regardless of timing -- so the degradation +# is silent, which is the dangerous direction. If this suite starts running on much slower +# hardware, re-measure the cutoff rather than assuming this value still has margin. +ORDER_HANDOFF_DELAY = 1.0 + + +def _handoff_sleep(reader_is_robot: bool) -> str: + """Sleep line for a sentinel writer, or nothing when the reader is in-process.""" + return f" time.sleep({ORDER_HANDOFF_DELAY})\n" if reader_is_robot else "" + + +@pytest.mark.parametrize("marker_style", ["numeric", "after", "before"]) +@pytest.mark.parametrize( + "first_type, middle_type, last_type", + [ + ("ROBOT", "ROBOT", None), + ("ROBOT", "PLAIN", None), + ("PLAIN", "ROBOT", None), + ("PLAIN", "PLAIN", None), + ("ROBOT", "ROBOT", "PLAIN"), + ("ROBOT", "PLAIN", "ROBOT"), + ("PLAIN", "ROBOT", "ROBOT"), + ("ROBOT", "PLAIN", "PLAIN"), + ("PLAIN", "ROBOT", "PLAIN"), + ("PLAIN", "PLAIN", "ROBOT"), + ], +) +def test_order_marker_enforces_sequencing( + pytester, first_type, middle_type, last_type, marker_style +): + """ + Order markers enforce a first->middle->last chain across all mixed + robot/non-robot permutations. + + test_first writes sentinel_1; test_middle reads sentinel_1 and writes + sentinel_2; test_last reads sentinel_2. The file lists them in reverse + (last, middle, first) so collection order would fail -- passing proves the + full chain was enforced. + + ROBOT = robot fixture (isolated subprocess) PLAIN = plain test (in-process) + + numeric: first=order(1), middle=order(2), last=order(3) + after: middle=order(after="test_first"), last=order(after="test_middle") + before: first=order(before="test_middle"), middle=order(before="test_last") + + Not every permutation can detect a broken run loop, and that is expected: + + - PLAIN-PLAIN-None has no robot test at all, so there is nothing for the plugin to + mis-order -- the old loop deferred non-robot tests but preserved their order among + themselves. These three cases assert the chain still works; they are not regression + guards and cannot be made into any. + - Where the sentinel *reader* is a robot test, the writer sleeps ORDER_HANDOFF_DELAY so the + reader cannot win the spawn race and pass by luck. See that constant for the measurement + and the slow-machine caveat. + """ + _make_robot_module(pytester) + _configure_isolated_plugin(pytester, parallelism=4) + + # Only a robot reader needs the writer slowed down; a plain reader runs in-process and + # already observes the violation directly. + first_sleep = _handoff_sleep(middle_type == "ROBOT") + middle_sleep = _handoff_sleep(last_type == "ROBOT") + + def params(t): + return "(robot)" if t == "ROBOT" else "()" + + if marker_style == "numeric": + first_mark = "@pytest.mark.order(1)\n" + middle_mark = "@pytest.mark.order(2)\n" + last_mark = "@pytest.mark.order(3)\n" + elif marker_style == "before": + first_mark = '@pytest.mark.order(before="test_middle")\n' + middle_mark = '@pytest.mark.order(before="test_last")\n' + last_mark = "" + else: + first_mark = "" + middle_mark = '@pytest.mark.order(after="test_first")\n' + last_mark = '@pytest.mark.order(after="test_middle")\n' + + pytester.makepyfile( + test_order_sequence=( + """\ +import pathlib +import time + +import pytest + + +""" + + ( + f"""{last_mark}def test_last{params(last_type)}: + assert pathlib.Path("sentinel_2.txt").exists(), "test_middle must run before test_last" + + +""" + if last_type is not None + else "" + ) + + f"""{middle_mark}def test_middle{params(middle_type)}: + assert pathlib.Path("sentinel_1.txt").exists(), "test_first must run before test_middle" +{middle_sleep} pathlib.Path("sentinel_2.txt").write_text("done") + + +{first_mark}def test_first{params(first_type)}: +{first_sleep} pathlib.Path("sentinel_1.txt").write_text("done") +""" + ) + ) + + result = pytester.runpytest_subprocess("-vv") + count_of_tests = sum(x is not None for x in [first_type, middle_type, last_type]) + result.assert_outcomes(passed=count_of_tests) + + +@pytest.mark.parametrize( + "a_fixture, b_fixture", + [("robot", "robot"), ("robot", "")], + ids=["RR", "RN"], +) +def test_unordered_tests_still_run_in_parallel(pytester, a_fixture, b_fixture): + """ + Tests WITHOUT @pytest.mark.order must not be serialised by order-marker + support. With parallelism=2, two 1.5 s tests must overlap in wall-clock + time when the first test uses the robot fixture (starts async subprocess, + allowing the second test to run concurrently). + + NR is omitted: a non-robot test runs in-process synchronously, so it + completes before the subsequent robot subprocess starts -- serial by design. + """ + _make_robot_module(pytester) + _configure_isolated_plugin(pytester, parallelism=2) + + def params(f): + return f"({f})" if f else "()" + + pytester.makepyfile(test_parallel_execution=f"""\ +import pathlib +import time + + +def test_a{params(a_fixture)}: + pathlib.Path("a_start.txt").write_text(str(time.monotonic())) + time.sleep(1.5) + pathlib.Path("a_end.txt").write_text(str(time.monotonic())) + + +def test_b{params(b_fixture)}: + pathlib.Path("b_start.txt").write_text(str(time.monotonic())) + time.sleep(1.5) + pathlib.Path("b_end.txt").write_text(str(time.monotonic())) +""") + + result = pytester.runpytest_subprocess("-vv") + result.assert_outcomes(passed=2) + + root = pathlib.Path(pytester.path) + a_end = float((root / "a_end.txt").read_text()) + b_start = float((root / "b_start.txt").read_text()) + assert ( + b_start < a_end + ), f"Expected parallel: b_start={b_start:.3f} a_end={a_end:.3f}" + + +def _read_times(pytester, *names): + root = pathlib.Path(pytester.path) + return {n: float((root / f"{n}.txt").read_text()) for n in names} + + +_TIMED_ROBOT_TEST = """\ +def {name}(robot): + pathlib.Path("{name}_start.txt").write_text(str(time.monotonic())) + time.sleep(1.0) + pathlib.Path("{name}_end.txt").write_text(str(time.monotonic())) +""" + + +def test_module_level_order_marker_parallel_within_group(pytester): + """ + A module-level order marker positions the module as a whole; pytest-order + explicitly does not constrain the order of the tests *inside* it ("the tests + inside each module will be run in the same order as without any ordering"). + + So the two modules must be serialised against each other, but the robot tests + within a module must still overlap. test_group_a is collected first but marked + order(2), so passing also proves the modules were actually reordered. + """ + _make_robot_module(pytester) + _configure_isolated_plugin(pytester, parallelism=4) + + def module_src(order, names): + return ( + "import pathlib\nimport time\nimport pytest\n\n" + f"pytestmark = pytest.mark.order({order})\n\n\n" + + "\n\n".join(_TIMED_ROBOT_TEST.format(name=n) for n in names) + ) + + pytester.makepyfile( + test_group_a=module_src(2, ["test_a1", "test_a2"]), + test_group_b=module_src(1, ["test_b1", "test_b2"]), + ) + + result = pytester.runpytest_subprocess("-vv") + result.assert_outcomes(passed=4) + + t = _read_times( + pytester, + "test_a1_start", + "test_a1_end", + "test_a2_start", + "test_a2_end", + "test_b1_start", + "test_b1_end", + "test_b2_start", + "test_b2_end", + ) + + # the order(1) module must fully complete before the order(2) module starts + assert max(t["test_b1_end"], t["test_b2_end"]) <= min( + t["test_a1_start"], t["test_a2_start"] + ), f"module boundary not enforced: {t}" + + # ...but within each module the tests must have overlapped + assert t["test_b2_start"] < t["test_b1_end"], f"module b serialised: {t}" + assert t["test_a2_start"] < t["test_a1_end"], f"module a serialised: {t}" + + +def test_class_level_order_marker_parallel_within_group(pytester): + """ + Same as the module-level case, for a class-level marker: "the class as a whole + will be reordered without changing the test order inside the test class". + TestAlpha is collected first but marked order(2). + """ + _make_robot_module(pytester) + _configure_isolated_plugin(pytester, parallelism=4) + + def class_src(cls, order, names): + body = "\n".join( + " " + line if line else "" + for n in names + for line in _TIMED_ROBOT_TEST.format(name=n) + .replace("(robot)", "(self, robot)") + .split("\n") + ) + return f"@pytest.mark.order({order})\nclass {cls}:\n{body}\n" + + pytester.makepyfile( + test_classes="import pathlib\nimport time\nimport pytest\n\n\n" + + class_src("TestAlpha", 2, ["test_x1", "test_x2"]) + + "\n" + + class_src("TestBeta", 1, ["test_y1", "test_y2"]) + ) + + result = pytester.runpytest_subprocess("-vv") + result.assert_outcomes(passed=4) + + t = _read_times( + pytester, + "test_x1_start", + "test_x1_end", + "test_x2_start", + "test_x2_end", + "test_y1_start", + "test_y1_end", + "test_y2_start", + "test_y2_end", + ) + + assert max(t["test_y1_end"], t["test_y2_end"]) <= min( + t["test_x1_start"], t["test_x2_start"] + ), f"class boundary not enforced: {t}" + + assert t["test_y2_start"] < t["test_y1_end"], f"TestBeta serialised: {t}" + assert t["test_x2_start"] < t["test_x1_end"], f"TestAlpha serialised: {t}" + + +def test_separate_markers_with_equal_value_are_not_grouped(pytester): + """ + Grouping is by marker *identity*, not equality. Inheritance hands every test in + a class/module the same Mark object, but two independent @pytest.mark.order(5) + decorators are distinct objects that merely compare equal. Those stay + serialised -- the conservative choice, since only the inherited case has + documented "order inside is unconstrained" semantics. + """ + _make_robot_module(pytester) + _configure_isolated_plugin(pytester, parallelism=4) + + pytester.makepyfile( + test_equal_markers="import pathlib\nimport time\nimport pytest\n\n\n" + + "@pytest.mark.order(5)\n" + + _TIMED_ROBOT_TEST.format(name="test_p") + + "\n\n@pytest.mark.order(5)\n" + + _TIMED_ROBOT_TEST.format(name="test_q") + ) + + result = pytester.runpytest_subprocess("-vv") + result.assert_outcomes(passed=2) + + t = _read_times(pytester, "test_p_end", "test_q_start") + assert ( + t["test_p_end"] <= t["test_q_start"] + ), f"equal-valued markers were merged into one group: {t}" + + +def test_robot_tests_ordered_relative_to_each_other(pytester): + """ + Two robot-fixture tests carrying order markers must run one after the other. + + Both run in isolated subprocesses, so without order support they are started together + and overlap. They are written to the file in reverse, so collection order alone fails. + """ + _make_robot_module(pytester) + _configure_isolated_plugin(pytester, parallelism=4) + + pytester.makepyfile(test_robot_chain=f"""\ +import pathlib +import time + +import pytest + + +@pytest.mark.order(2) +def test_robot_second(robot): + assert pathlib.Path("robot_first.txt").exists(), "test_robot_first must run first" + + +@pytest.mark.order(1) +def test_robot_first(robot): + time.sleep({ORDER_HANDOFF_DELAY}) + pathlib.Path("robot_first.txt").write_text("done") +""") + + result = pytester.runpytest_subprocess("-vv") + result.assert_outcomes(passed=2) + + +def test_module_level_order_vs_robot_tests(pytester): + """ + A module-level order marker must sequence a robot test against another module's tests. + + test_group_a is collected first but marked order(2), so passing also proves the modules + were reordered. The robot test is the reader, since that is the direction the old run + loop broke: robot tests were hoisted ahead of every non-robot test. + """ + _make_robot_module(pytester) + _configure_isolated_plugin(pytester, parallelism=4) + + pytester.makepyfile( + test_group_a="""\ +import pathlib + +import pytest + +pytestmark = pytest.mark.order(2) + + +def test_robot_reads(robot): + assert pathlib.Path("early.txt").exists(), "the order(1) module must run first" +""", + test_group_b=f"""\ +import pathlib +import time + +import pytest + +pytestmark = pytest.mark.order(1) + + +def test_plain_writes(): + time.sleep({ORDER_HANDOFF_DELAY}) + pathlib.Path("early.txt").write_text("done") +""", + ) + + result = pytester.runpytest_subprocess("-vv") + result.assert_outcomes(passed=2) + + +def test_class_level_order_vs_robot_tests(pytester): + """ + A class-level order marker must sequence a robot test against another class's tests. + + TestLate is written first but marked order(2), so passing also proves the classes were + reordered. + """ + _make_robot_module(pytester) + _configure_isolated_plugin(pytester, parallelism=4) + + pytester.makepyfile(test_class_chain=f"""\ +import pathlib +import time + +import pytest + + +@pytest.mark.order(2) +class TestLate: + def test_robot_reads(self, robot): + assert pathlib.Path("early.txt").exists(), "TestEarly must run first" + + +@pytest.mark.order(1) +class TestEarly: + def test_plain_writes(self): + time.sleep({ORDER_HANDOFF_DELAY}) + pathlib.Path("early.txt").write_text("done") +""") + + result = pytester.runpytest_subprocess("-vv") + result.assert_outcomes(passed=2) + + +def test_class_level_relative_order_vs_robot_tests(pytester): + """ + A class-level RELATIVE marker (`after=`) must sequence a robot test against another class. + + pytest-order documents referencing a test class by name from `before=`/`after=`, which is a + different code path from the ordinal markers covered above. + """ + _make_robot_module(pytester) + _configure_isolated_plugin(pytester, parallelism=4) + + pytester.makepyfile(test_class_relative=f"""\ +import pathlib +import time + +import pytest + + +@pytest.mark.order(after="TestEarly") +class TestLate: + def test_robot_reads(self, robot): + assert pathlib.Path("early.txt").exists(), "TestEarly must run first" + + +class TestEarly: + def test_plain_writes(self): + time.sleep({ORDER_HANDOFF_DELAY}) + pathlib.Path("early.txt").write_text("done") +""") + + result = pytester.runpytest_subprocess("-vv") + result.assert_outcomes(passed=2) + + +def test_module_level_relative_order_vs_robot_tests(pytester): + """ + A module-level RELATIVE marker must sequence a robot test against another module's test. + + Note this form -- `pytestmark = pytest.mark.order(after="path::test")` -- is not documented + upstream; pytest-order shows `before=`/`after=` only at function and class scope. It works + because pytestmark is ordinary pytest marker inheritance, but it is de facto rather than + de jure, so this test also serves to detect it changing. + """ + _make_robot_module(pytester) + _configure_isolated_plugin(pytester, parallelism=4) + + pytester.makepyfile( + test_rel_a="""\ +import pathlib + +import pytest + +pytestmark = pytest.mark.order(after="test_rel_b.py::test_plain_writes") + + +def test_robot_reads(robot): + assert pathlib.Path("early.txt").exists(), "test_rel_b must run first" +""", + test_rel_b=f"""\ +import pathlib +import time + + +def test_plain_writes(): + time.sleep({ORDER_HANDOFF_DELAY}) + pathlib.Path("early.txt").write_text("done") +""", + ) + + result = pytester.runpytest_subprocess("-vv") + result.assert_outcomes(passed=2) + + +def test_function_marker_inside_marked_class(pytester): + """ + A function-level marker inside an already-marked class wins, and forms its own group. + + get_closest_marker returns the function's own Mark rather than the class's inherited one, + so the marked method is a group of one: it is serialised against its own siblings, not + merged with them. Here the method marked order(1) must overtake its class-mates even though + the class as a whole is marked order(2). + """ + _make_robot_module(pytester) + _configure_isolated_plugin(pytester, parallelism=4) + + pytester.makepyfile(test_nested_marker=f"""\ +import pathlib +import time + +import pytest + + +@pytest.mark.order(2) +class TestGroup: + def test_robot_reads(self, robot): + assert pathlib.Path("early.txt").exists(), "the order(1) method must run first" + + @pytest.mark.order(1) + def test_plain_writes_first(self): + time.sleep({ORDER_HANDOFF_DELAY}) + pathlib.Path("early.txt").write_text("done") +""") + + result = pytester.runpytest_subprocess("-vv") + result.assert_outcomes(passed=2) diff --git a/subprojects/robotpy-wpilib/wpilib/testing/pytest_isolated_tests_plugin.py b/subprojects/robotpy-wpilib/wpilib/testing/pytest_isolated_tests_plugin.py index 4114cc4d4..e03852159 100644 --- a/subprojects/robotpy-wpilib/wpilib/testing/pytest_isolated_tests_plugin.py +++ b/subprojects/robotpy-wpilib/wpilib/testing/pytest_isolated_tests_plugin.py @@ -149,7 +149,24 @@ def _run_test( worker_plugin = WorkerPlugin(pipe) ec = pytest.main( - [item_nodeid, "--no-header", "-p", "no:terminalreporter", *config_args], + [ + item_nodeid, + "--no-header", + "-p", + "no:terminalreporter", + # "-p", "no:order" tells pytest in the isolated subprocess to not run pytest-order or look at + # the @pytest.mark.order decorators. This is fine because the isolated subprocess runs just one + # test at a time, so order does not matter at this level. The purpose of not running + # pytest-order is so that it doesn't give misleading warnings like: + # WARNING: cannot execute 'test_step2_reads_sentinel' relative to others: + # 'test_step1_writes_sentinel' - ignoring the marker. + # The warning would be accurate from the isolated subprocess point of view, + # it can't see the other test, but it is misleading because the main process + # successfully ordered the tests. + "-p", + "no:order", + *config_args, + ], plugins=[plugin, worker_plugin], ) @@ -237,28 +254,57 @@ def pytest_runtestloop(self, session: pytest.Session) -> bool: return True running: list[IsolatedTestJob] = [] - deferred: list[pytest.Function] = [] try: - # Start any tests that use the robot fixture first. Tests that don't - # use the robot fixture will be ran later - for item in session.items: + # Run tests in the order that they are given to us, running robot fixture tests in a + # subprocess, while preserving order marker boundaries. + # + # pytest-order has already sorted session.items during collection, so all this has to + # do is stop tests from overtaking each other across an ordering boundary. + # + # An order marker applied to a class or module is inherited by every test underneath + # it, and get_closest_marker returns the *same* Mark object for each of them. Comparing + # order group markers by identity therefore treats those tests as one group: the group + # is serialized against everything outside it, while its members -- whose relative + # order pytest-order explicitly does not constrain -- still run in parallel. Identity + # is required rather than ==, because two independent @pytest.mark.order(5) decorators + # compare equal but are separate constraints that must not be merged into one group. + prev_order_group_marker = None + for idx, item in enumerate(session.items): assert isinstance(item, pytest.Function) - if "robot" not in item.fixturenames: - deferred.append(item) - continue - while len(running) >= self._parallelism: - self._wait_for_jobs(running, session) + order_group_marker = item.get_closest_marker("order") + + if order_group_marker is not prev_order_group_marker: + # Crossing an ordering boundary: everything started so far has to finish before + # anything on the far side of the boundary begins. One drain covers both leaving a + # group -- so @pytest.mark.order(before="NAME") completes first -- and entering one, + # so @pytest.mark.order() and @pytest.mark.order(after="NAME") see the + # tests they were sorted behind already finished. Two unmarked tests in a row are + # both None, so they do not trigger a drain. + while running: + self._wait_for_jobs(running, session) + prev_order_group_marker = order_group_marker + + if "robot" in item.fixturenames: + # This test uses the "robot" fixture, run it in an available, isolated subprocess. + while len(running) >= self._parallelism: + self._wait_for_jobs(running, session) + + running.append(self._start_isolated_test(item)) + else: + # This test runs in this process. Only pass nextitem as a teardown-optimization + # hint when the next test also runs in this process -- otherwise the next item is + # handled by a subprocess and this one must tear down fully. + nextitem = ( + session.items[idx + 1] if idx + 1 < len(session.items) else None + ) + if nextitem is not None and "robot" in nextitem.fixturenames: + nextitem = None + + session.config.hook.pytest_runtest_protocol( + item=item, nextitem=nextitem + ) - running.append(self._start_isolated_test(item)) - self._maybe_raise(session) - - # Run the in-process tests now while the robot tests are finishing - for idx, item in enumerate(deferred): - nextitem = deferred[idx + 1] if idx + 1 < len(deferred) else None - session.config.hook.pytest_runtest_protocol( - item=item, nextitem=nextitem - ) self._maybe_raise(session) while running: