Skip to content

Isolated robot-fixture tests execute in marker order - #299

Open
MikeStitt wants to merge 4 commits into
robotpy:mainfrom
MikeStitt:marker-order-on-main
Open

Isolated robot-fixture tests execute in marker order#299
MikeStitt wants to merge 4 commits into
robotpy:mainfrom
MikeStitt:marker-order-on-main

Conversation

@MikeStitt

Copy link
Copy Markdown
Collaborator

Forward-port of pyfrc#257 to the testing code's new
home in robotpy-wpilib, per comment there:

pyfrc is being archived and testing stuff is in robotpy-wpilib in mostrobotpy starting in 2027.
Will try to bring this along soon.

pyfrc#257 was itself a more robust replacement for pyfrc#256.

What this fixes

Under IsolatedTestsPlugin, tests carrying @pytest.mark.order now run in the order pytest-order
sorted them. Concretely, robot-fixture tests are correctly sequenced:

  • relative to each other, and
  • relative to pytest module-, class-, and function-ordered tests.

Unordered tests keep running isolated and in parallel exactly as before.

Problem

IsolatedTestsPlugin.pytest_runtestloop hoists every robot-fixture test to the front and defers
the rest:

for item in session.items:
    if "robot" not in item.fixturenames:
        deferred.append(item)      # <-- runs later
        continue
    running.append(self._start_isolated_test(item))

This discards collection order. So @pytest.mark.order has no effect under this plugin, even
though pytest-order sorted session.items correctly during collection. Ordering-dependent tests
silently run in the wrong sequence.

The failure is specifically between robot-fixture tests and non-robot-fixture tests. A chain of
only non-robot-fixture tests still passes today, because the deferred list preserves their
relative order among themselves, and are not spawned into subprocesses.
This is why tests below mixes the two.

Fix

Walk session.items in the pre-sorted order given, and drain in-flight subprocesses at ordering boundaries.
Unordered robot-fixture tests still run isolated and still run in parallel. Only the boundaries serialize.

The subprocess also gets -p no:order. It runs exactly one test and cannot see the others, so
pytest-order would otherwise emit misleading warnings like cannot execute 'test_b' relative to others: 'test_a' - ignoring the marker. That warning is accurate from inside the subprocess, but
it is wrong about the outcome, because the parent did order them.

Design note: grouping by marker identity

This is a refinement over pyfrc#257, which drained around every item carrying an order marker.
An order marker on a class or module is inherited by every test underneath it. pytest-order
documents that this does not constrain the interior:

all tests in this class will be handled as having the same ordinal marker, e.g. the class as a
whole will be reordered without changing the test order inside the test class

So draining around each such test would serialize an entire marked module for no correctness
benefit. get_closest_marker returns the same Mark object to every test that inherits it,
which means comparing by identity groups them naturally:

order_group_marker = item.get_closest_marker("order")
if order_group_marker is not prev_order_group_marker:
    while running:
        self._wait_for_jobs(running, session)
prev_order_group_marker = order_group_marker

The group is serialized against everything outside it, and its members stay parallel. This also
folds pyfrc#257's separate pre-drain and post-drain into one. Leaving a group covers before=,
and entering one covers ordinals and after=.

The fix uses identity rather than == to compare markers. This is deliberate and conservative.
Two independent @pytest.mark.order(5) decorators compare equal but are distinct constraints, so they stay
serialized. Only the inherited case is relaxed, which is the case documented as internally
unconstrained.

Changes

All under subprojects/robotpy-wpilib/:

File Change
wpilib/testing/pytest_isolated_tests_plugin.py order-aware run loop, -p no:order for the subprocess
pyproject.toml add pytest-order dependency
tests/requirements.txt add pytest-order
tests/test_pytest_plugins.py 41 new tests

Tests

test_pytest_plugins.py goes from 19 tests to 60. These six state the claim directly:

Test Asserts
test_robot_tests_ordered_relative_to_each_other two ordered robot tests run in sequence
test_module_level_order_vs_robot_tests robot test sequenced against a module-ordered test
test_class_level_order_vs_robot_tests robot test sequenced against a class-ordered test
test_module_level_relative_order_vs_robot_tests same, using after= rather than an ordinal
test_class_level_relative_order_vs_robot_tests same, using after="TestName"
test_function_marker_inside_marked_class a method's own marker overrides its class's, forming a group

Each one is written to its file in reverse order, so passing also proves reordering occurred.
Each one fails against the current loop.

There is supporting coverage too. 30 parametrized test_order_marker_enforces_sequencing cases
run a first, middle, last chain across all 10 robot/plain permutations, times ordinal, before=
and after=. There are also regression guards that unordered tests still overlap in wall-clock
time, and that a marked group's interior is not needlessly serialized.

Full robotpy-wpilib suite via tests/run_tests.py, from a source build on macOS and Python
3.14:

============= 242 passed, 8 skipped, 1 xpassed in 66.66s (0:01:06) =============

Every ordering test was checked against the unfixed loop

A test that passes with and without the fix guards nothing, so each one was run against current
main. That exposed a real problem, which is now fixed.

A sentinel written by a plain in-process test lands within milliseconds. A robot test needs a few
hundred milliseconds to spawn its subprocess and reach its first assertion. So whenever the
sentinel reader was a robot test, an unfixed run still found the sentinel already written, and it
passed without proving anything. Sweeping the delay against the unfixed plugin put the cutoff
between 150 ms and 200 ms on an M-series Mac. At 150 ms it caught the bug in 0 runs out of 5, and
at 200 ms it caught it in 5 out of 5.

ORDER_HANDOFF_DELAY = 1.0 now makes the writer sleep whenever its reader is a robot test. This
is applied selectively, because a plain reader observes the violation directly and needs no
delay. Here is the result across the 30 permutations:

before after
cases that pass without the fix 9 to 11, varying 3, deterministic
cases that are genuine guards 17 27

The 3 remaining ones are the PLAIN-PLAIN-None variants. They contain 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 cases assert that the chain still works. They confirm two
ordered plain tests run in the right order. What they cannot do is catch this particular bug,
because the bug is robot tests jumping ahead of non-robot tests, and these cases have no robot test.
They pass the same way before and after the fix. Adding a delay to guard against race conditions
between spawned and not spawned tests is not needed because these tests are not spawned.
The test docstring says this, so nobody later mistakes them for broken guards.

Two caveats are recorded alongside the ORDER_HANDOFF_DELAY:

  • Cost. The delays take the suite from 38 s to 67 s.
  • Slow machines. If a robot subprocess ever takes more than a second to reach its assertion,
    these cases quietly stop catching regressions. They never fail wrongly, because with ordering
    support present they pass regardless of timing. That makes the degradation silent, which is the
    dangerous direction. The measured cutoff is in the comment, so a future maintainer can
    re-measure rather than guess.

pyfrc#257 used the same logic, minus the identity grouping, and was green across all 24 CI jobs
there.

The default robot tests keep their parallelism

This was measured on a project containing only the default tests, which is an empty TimedRobot
plus whatever robotpy add-tests generates. 3 runs per configuration, median wall-clock:

-j current main this PR
8 0.48s 0.50s
1 1.03s 0.99s

These are identical within noise, and the run-to-run spread was 0.01s. -j 8 is still about
twice as fast as -j 1, which shows the concurrency is intact rather than merely fast enough to
hide a regression.

This is what the implementation predicts. The default tests carry no order marker, so
get_closest_marker("order") returns None for every item. The boundary condition
order_group_marker is not prev_order_group_marker never fires, and no drain is ever inserted. A
suite with no ordering pays nothing.

To reproduce:

mkdir demo && cd demo
printf 'import wpilib\n\n\nclass MyRobot(wpilib.TimedRobot):\n    pass\n' > robot.py
python -m robotpy add-tests
time python -m robotpy test -j 8 -- -q
time python -m robotpy test -j 1 -- -q

Behaviour across isolation and job settings

The order-demo.zip
project has 19 tests. Here it is run under each mode against both
versions:

Mode current main this PR
--no-isolation 19 passed 19 passed
-j 1 4 failed, 15 pass 19 passed
-j default (15) 5 failed, 14 pass 19 passed

Three things this establishes.

--no-isolation is unaffected. That path uses RobotTestingPlugin and pytest's own run
loop, so it never reaches the code this PR changes. Ordering already works there, before and
after. The defect is specific to isolated mode.

The defect is not a concurrency race. It reproduces at -j 1, where at most one robot
subprocess exists at a time. Turning down -j is not a workaround.

-j 1 does mask one case, and it is worth being precise about which one. The failure that
goes missing at -j 1 is the robot to robot chain,
test_ordered_robot.py::test_robot_second. In the current loop,
while len(running) >= self._parallelism with parallelism == 1 forces each robot subprocess to
finish before the next one starts. The hoisted robot tests are already in pytest-order's sorted
sequence, so serialising them accidentally yields the right order. Robot against robot ordering
therefore appears to work at -j 1, and it breaks as soon as concurrency is allowed. The mixed
robot and non-robot chains fail at every -j, because those break from the hoisting itself,
which puts robot tests ahead of all non-robot tests regardless of the job limit.

Reproducing it yourself with order-demo.zip

Attached is a complete minimal robot project, an empty TimedRobot plus a tests/ directory,
that demonstrates the bug and the fix through the real robotpy test entry point.

unzip order-demo.zip && cd order-demo
python -m robotpy test -- -q
python check_parallel.py

The demo clears its own state at session start, so it can be run repeatedly, and in either order
against either version, without manual cleanup.

With this PR, 19 passed:

...................                                                      [100%]
19 passed

distinct pids: 3 of 3 -> isolated
overlapping pairs: [('par_a','par_b'), ('par_a','par_c'), ('par_b','par_c')] -> PARALLEL

Without it, 5 failed and 14 passed, which is one failure per ordering style:

FAILED tests/test_ordered_robot.py::test_robot_second
FAILED tests/test_ordered_function.py::test_fn_second
FAILED tests/test_ordered_function.py::test_fn_third
FAILED tests/test_ordered_class.py::TestClassSecond::test_reads_sentinel
FAILED tests/test_ordered_module_b.py::test_module_b_robot

The demo's tests/robot_test.py is exactly what robotpy add-tests generates, and
check_parallel.py confirms the default tests still run isolated and concurrently.

Both figures above were reproduced three times each, in both orders, with no cleanup between
runs. A stale sentinel would make an unfixed run report fewer failures than it should. So
tests/_sentinel.py also asserts that a sentinel does not already exist before writing it, which
turns that into a loud error rather than a reassuring wrong answer.

There is one caveat baked into the demo. The first test of each chain sleeps 1.0s before writing
its sentinel. Without that delay the chains are races rather than deterministic failures, because
a robot subprocess takes a few hundred milliseconds to spawn and a fast in-process test often
wins. The chain then passes by luck. An earlier draft showed only 2 failures for exactly that
reason.

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
@auscompgeek
auscompgeek self-requested a review August 2, 2026 14:39
@virtuald

virtuald commented Aug 3, 2026

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 12f6eac80c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread subprojects/robotpy-wpilib/wpilib/testing/pytest_isolated_tests_plugin.py Outdated

# For running robot tests
"pytest>=3.9",
"pytest-order",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is pytest-order a required dependency for robot projects now?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f087bd7. It was an oversight from earlier code. pytest appropriately returns None for get_closest_marker when pytest-order is not installed.

Passing "-p no:order" to the worker unloads pytest-order entirely, including
its registration of the "order" marker. The @pytest.mark.order decorators are
still on the tests the worker collects, so the marker became unknown there.

Projects that escalate unknown markers therefore could not run ordered robot
tests at all: --strict-markers made it a collection error, and
filterwarnings=error promoted the PytestUnknownMarkWarning into one. Both
settings reach the worker, which inherits the parent's command line and chdirs
to the project root before reading the ini. The worker died during its own
collection, so the parent reported "no tests ran" with no indication of why.

WorkerPlugin now re-registers the marker in pytest_configure. This restores
only the registration that --strict-markers and the unknown-mark check consult;
it does not reload pytest-order, so the reordering stays off in the worker
where a single collected test cannot satisfy a relative marker anyway.

- Register "order" from WorkerPlugin.pytest_configure, which runs before
  collection in the isolated subprocess
- Note at the "-p no:order" site that the two halves belong together
- Add a regression test covering --strict-markers and filterwarnings=error,
  plus a relative-marker case that fails if the registration ever turns back
  into a plugin reload
Walking session.items in collection order preserved ordering but gave up the
overlap the previous run loop got for free. That loop deferred every non-robot
test to the end, so isolated subprocesses were always started first and
in-process tests ran while they were in flight. Following collection order
means an in-process test collected before any robot test runs with nothing in
flight, and its time is added to the run rather than hidden inside it.

pytest-order does not constrain the order within a group, so the sequence
inside one carries no meaning and the loop is free to schedule for throughput.
It now treats each group as two queues: fill every subprocess slot, run
in-process tests in the time those subprocesses take, and block only once there
is nothing left to overlap with. Boundaries between groups are untouched and
still drain.

Reaping is a poll between in-process tests, or a wait when none are left.
Slots are only released inside _wait_for_jobs, so a loop that only polled
would spin once the in-process queue emptied, and one that only waited would
hold the subprocess count at its high-water mark through a long run of
in-process tests and refuse to start anything new.

Measured with in-process tests collected ahead of four 1.5s robot tests at -j 4,
each in-process test sleeping 1ms:

    tests     before    after
      50       2.01s    1.95s
     250       2.37s    1.95s
     500       2.86s    1.94s
    1000       3.74s    2.06s

The old loop grows with the volume of in-process work. The new one is flat,
because that work now fits inside the robot tests. Suites with no in-process
tests, and suites with more robot tests than -j, measured unchanged.

- Schedule each order marker group as an isolated queue and an in-process
  queue drained against each other
- Add a timeout argument to _wait_for_jobs so finished jobs can be reaped
  without stalling this process
- Extract _order_marker_groups, carrying the Mark-identity rationale with it
- Add the NR permutation to test_unordered_tests_still_run_in_parallel; the
  case it documented as serial by design now overlaps, and it fails without
  this change
The isolated test plugin reads order markers through get_closest_marker, which
returns None for every test when the plugin is absent, and passes "-p no:order"
to its subprocesses, which pytest ignores for a plugin name it does not know
about.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants