petercorke / petercorke/robotics-toolbox-python

Test the pure-Python fallback path (and, optionally, real Pyodide) in CI

Open
#633 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

tech-debt
Dominant language
C++
Stars
3.5k
Forks
624
Avg merge
2d 4h
Merged PRs (30d)
53

Description

Background

ci.yml's test-core/test jobs currently assert _C_AVAILABLE == True for both _fknm_c and _frne_c -- meaning the real test suite only ever exercises the compiled C path. tests/test_fknm_fallback.py does narrow, targeted C-vs-Python cross-validation, but nothing runs the whole suite against the pure-Python fallback path.

This gap is exactly how a real bug shipped undetected: a stale branch, feat/pyodide-pure-wheel-v2 (pushed to origin 2026-07-23, 4 commits, never merged -- 109 commits behind current main as of 2026-08-25, too stale to merge as-is), ran the full test suite inside a real Pyodide environment (pyodide venv, Node.js-backed) with zero compiled extensions present, and found 4 real issues:

  1. test_ETS.py::test_insert -- a genuine off-by-one (7-joint model, 6-element q) that the C extension's lack of bounds checking silently tolerated (q[6] read past the array end, undefined behaviour, happened not to crash). The pure-Python path correctly raised IndexError.
  2. URDFRobot.py's _load_rd_module -- the sys.platform == "emscripten" guard only checked inside except Exception, not except ImportError; GitPython's subprocess failure under Pyodide surfaces as a plain ImportError, so the guard never fired and users got a misleading "model renamed" error instead of the real, actionable one.
  3. test_ET.py's .fknm-identity tests -- not bugs, just needed skipUnless(_C_AVAILABLE) gating (meaningless assertions with no compiled extension present at all).
  4. test_trajectory.py::test_quintic -- a velocity-tolerance too tight for cross-platform float noise; Pyodide's WASM-compiled numpy gives ~-3.7e-15 at a point desktop numpy gives ~-1e-16 for.

Also logged there (in that branch's now-defunct tech-debt.md addition, not carried forward as its own issue): IK.py's _calc_qnull divides by a near-zero quantity, causing an overflow that recovers gracefully on desktop but hard-crashes (math.cos(-inf)) under Pyodide's WASM numpy for the same seeded case. Not yet independently verified against current main -- worth checking if this is picked up.

What was cherry-picked from that branch (2026-08-25, this session)

Findings 1 and 2 are real, live bugs, confirmed still present on main at the time of filing -- fixed directly (with fail/fix/pass verification against a real editable install of this fix, not just eyeballing the diff), independent of everything else on that branch. Findings 3 and 4 were not re-verified or ported in this pass -- 3 is cosmetic (gating, not a bug); 4 (the WASM-numpy tolerance) can't be reproduced without a real Pyodide/WASM numpy build, which wasn't set up this session. Worth reverifying both if/when the discussion below is acted on.

The actual open question: is real-Pyodide CI testing still worth it?

Splitting what that branch's test-pyodide CI job was actually catching:

  • Findings 1, 2, and 3 are pure Python-vs-C-path coverage gaps -- nothing WASM-specific about them. They'd be caught by running the whole suite against the pure-Python fallback path on an ordinary desktop Linux runner. No Pyodide, no Emscripten, no Node.js needed.
  • Finding 4 is the only genuinely Pyodide/WASM-specific one -- a real difference between WASM-compiled numpy's floating-point behaviour and desktop numpy's. No desktop test run would ever surface it; either real Pyodide execution or a defensively-generous tolerance (not derived from observing the exact WASM behaviour) are the only ways to handle it.

Since this session's separate change (roboticstoolbox-python#632) made the Pyodide/JupyterLite wheel a genuine py3-none-any build, the cheap fix for the first bullet is now nearly free: a CI job that installs that wheel into a normal ubuntu-latest runner and runs the full pytest tests/ suite (no Pyodide, no Node.js, no pyodide venv) would catch findings-1/2/3-class issues directly. The old branch's real-Pyodide job (pyodide venv + Node.js) would still be needed to catch finding-4-class issues, but that's a narrower, optional value-add on top, not a prerequisite.

Next steps, if picked up

  1. Add the cheap desktop job first (install the RTB_PURE_WHEEL=1 wheel, run the full suite on a normal runner) -- low cost, catches the bulk of what was found.
  2. Re-verify findings 3 and 4 against current main before deciding whether the heavier real-Pyodide job is worth adding on top.
  3. Re-check whether the IK.py _calc_qnull numerical issue (noted above, never independently filed) is still live; file its own issue if so.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with ci.yml and the existing test-core/test jobs, then trace how RTB_PURE_WHEEL=1 installs the pure-Python wheel. Add a normal Linux CI job that runs pytest tests/ against that wheel, and reverify the listed fallback findings before deciding whether real Pyodide coverage is also needed.

Written by the indexing model from the issue text.

Assessment

Tech stack
github-actions, python
Domain
ci-cd, testing-qa
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
64/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.