Skip to content

Run wasm tools in hub test instead of skipping them - #32

Open
jorgeMFS wants to merge 4 commits into
masterfrom
test-wasm-tools
Open

jorgeMFS wants to merge 4 commits into
masterfrom
test-wasm-tools

Conversation

@jorgeMFS

@jorgeMFS jorgeMFS commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

The problem

hub test looks only for runtime/native/{bin}, prints [SKIP] when it isn't there, and returns a pass.

25 of the 32 recipes have no native build. So for 78% of the catalogue, a green test run says only that the build produced a file of the expected name. Anything that links cleanly and then traps at run time goes unnoticed — that is how four newick-utils tools reached review, and why kallisto took three blind CI rounds.

What this does

Runs the wasm artifact when there is no native binary. A small Node harness (hub/tests/run_wasm.js) loads the module, writes the generated inputs into its filesystem, calls main, and returns stdout, stderr and whatever files the tool left behind. The Python side writes those into the temporary directory, so the existing output discovery and type checking apply unchanged — every recipe is exercised the same way, whichever runtime it ships.

The harness returns the whole working directory rather than a named list, because the test never tells a tool where to write: it looks afterwards for a file named after the output. The same discovery has to work for both runtimes.

One build flag is required

The recipes are compiled with ENVIRONMENT=web,worker, and such a module refuses to load anywhere else:

FAILS: not compiled for this environment (did you build to HTML and try to run it not on the web?)

That is the actual reason wasm tools could only ever be skipped. Adding node:

build flags Node
A ENVIRONMENT=web,worker (current) fails to load
B ENVIRONMENT=web,worker,node loads and runs

It costs ~3 KB of JS shim (168,381 → 171,410 bytes on a test tool) and changes nothing for the browser targets.

A module that cannot run now fails

Previously an aborting module still produced VERDICT: PASS. A load failure or abort is now a test failure. A tool's own non-zero exit is still only reported, matching how the native path treats it, since some tools exit non-zero by design.

Verification

Against artifacts built with the hub's own EM_FLAGS, using emsdk 4.0.18 (the version the recipes pin):

case result
working wasm-only tool passes, and genuinely runs (Testing tool democopy (wasm))
truncated .wasm fails with the load error
tool emitting the wrong output type failsDetected: UNKNOWN, Expected: ['FASTA']
native tool passes, still receiving absolute paths — unchanged
neither artifact present still skips, not a failure

The third case is the one that matters: it proves the type checking has teeth through the new path rather than passing vacuously.

Run against real recipes

Built with the hub's own pipeline (validatebuildtest) on emsdk 4.0.18, for the five wasm-only recipes that use the emscripten strategy — every one of them previously [SKIP]ped:

recipe before after
edlib SKIP runs, passes
abpoa SKIP runs, passes
cgranges SKIP runs, passes
fermi-lite SKIP runs, passes — but [WARNING] Empty output
tn93 SKIP runs, passes — but [WARNING] Output file not found

So five real tools now execute where nothing did before, and none of them regress CI.

Two things this exposes, which are worth knowing before merging:

  1. The pass criterion is still weak. fermi-lite and tn93 run and produce nothing usable, and still pass, because empty or missing output is only a [WARNING]. That is pre-existing behaviour and I have not changed it here — the cause is most likely that the generated example input is not meaningful for those tools rather than the tools being broken. Making it fail would need better per-type fixtures first, otherwise it would just turn recipes red without telling anyone anything useful.

  2. Where a native build exists, the wasm artifact is still never tested. Native takes precedence, which preserves existing behaviour, but the wasm build is what the SPA actually runs. That affects ksw2, lastz, prodigal and samblaster. Arguably both should run; I have kept the change minimal.

Not covered here: the 22 biowasm/auto recipes, which carry 167 of the 176 operations and need Docker to build. Their behaviour under this change is unknown until CI runs it, and that is the real blast radius — a 9-operation sample cannot speak for 167.

Note this PR also needs #33 (lazy Docker client) to be buildable without a daemon, though the two are independent as changes.

Review found three things that would have made this harmful

Worth reading before the rest, because they change what the PR is:

1. hub test could not fail. test_cmd printed the failing tools and returned, so the command exited 0 and CI passed regardless. Everything here was log text. It now exits non-zero — verified: exit 1 on a failing tool, 0 on a skip.

2. Nineteen recipes would have flipped from skipped to failing, blaming the recipe. Only the emscripten builder gained node; the other nineteen are built by the biowasm container, whose flags live in its own bin/shared.sh and were out of reach of the original change. Those modules abort with not compiled for this environment — which says nothing about whether the tool works.

Two changes: the harness reports that refusal distinctly, and the test skips it with a plain reason rather than failing; and the biowasm image is patched at build time to add node, so those tools become testable once the image is rebuilt.

3. -1 was both a sentinel and a real exit status. return -1 is an ordinary way for a tool to reject its arguments, and it was indistinguishable from a trap — so the tool's output was discarded and it was reported broken. Loading and completion are separate fields now.

Also fixed: non-UTF-8 stderr raised UnicodeEncodeError out of print(), which escaped and left every remaining recipe untested; and each tool is now tested inside its own handler so one cannot end the run.

case result
browser-only module (biowasm-style) skips, does not fail
genuinely broken .wasm fails with a real diagnostic
tool returning -1 keeps its output, not mistaken for a trap
non-UTF-8 stderr run completes
hub test with a failing tool exit 1
real edlib, rebuilt still passes

Honest limits

  • The biowasm Dockerfile change is unverified — it needs Docker, which I do not have. Until that image is rebuilt, those nineteen recipes skip with an explicit reason rather than silently, which is still an improvement on the previous behaviour but is not the same as testing them.
  • Separately, that Dockerfile clones biowasm unpinned (git clone ... . with no ref), so what it builds depends on when the image was made. Not addressed here.
  • The pass criterion remains weak: [WARNING] Empty output and a missing output file do not fail, and no output flag is ever passed to a tool. So for a large share of operations the check still cannot go red. Strengthening it needs better per-type fixtures and belongs in its own change.

Review notes

  • Independent of the R work (Add a CI job that cross-compiles R packages to WebAssembly #27Build recipes that declare the R strategy #29) — different files, and it branches from master.
  • The native execution path is deliberately untouched: it still builds absolute paths and still ignores the tool's return code. Both are arguable, but changing them risks the seven recipes that currently rely on it, and that belongs in its own change.
  • Existing published artifacts were built without the node flag. This matters only if hub test is ever run against a registry pull rather than a fresh hub build; in CI the build immediately precedes the test, so the artifacts carry the flag.
  • node becomes a requirement for hub test. It is already present on the runners, and the failure mode is an explicit message rather than a crash.

Merge order

sbom-implementation is the priority branch and rewrites parts of the builders. This PR conflicts with it on hub/builders/emscripten.py. Rebase after it lands. The resolution is one line — -s ENVIRONMENT=web,worker becomes -s ENVIRONMENT=web,worker,node; EM_FLAGS is otherwise unchanged there.

Independent of the R work (#27, #28, #29) and of #25/#26.

Interaction with #36

Checked against sbom-implementation as it stands on 2026-08-10, since #36 is now open.

  • It does not touch hub/tests/, so nothing here overlaps it.
  • Its EM_FLAGS still reads -s ENVIRONMENT=web,worker, so the one-line resolution described above still applies.
  • It does not change test_cmd, so the SystemExit(1) added here is still needed and does not conflict — both branches edit hub/hub.py in different places and merge cleanly.

Worth knowing while both are in flight: hub test appears in validate-recipes.yml but not in publish-recipes.yml, which runs validate → build → sbom → sbom-check → publish → sign-attest. So on the publishing path the tools are never executed, and on the PR path the command currently exits 0 whatever happens. Until this PR lands, a bundle can be signed and attested without any tool in it having been run successfully. That is an observation about how the two fit together, not a claim about #36.

hub test looked only for runtime/native/{bin}, printed [SKIP] when it was
not there, and returned a pass. Twenty-five of the thirty-two recipes
have no native build, so for most of the catalogue a green test run said
only that the build had produced a file of the expected name. Anything
that linked cleanly and then trapped at run time went unnoticed: that is
how four newick-utils tools reached review, and why kallisto took three
blind CI rounds.

The wasm artifact is now run when there is no native binary. A small
Node harness loads the module, writes the generated inputs into its
filesystem, calls main, and returns stdout, stderr and whatever files
the tool left behind; the Python side writes those into the temporary
directory so the existing output discovery and type checking apply
unchanged. Every recipe is therefore exercised the same way, whichever
runtime it ships.

This needs one build flag. The recipes were compiled with
ENVIRONMENT=web,worker, and such a module refuses to load anywhere else
-- "not compiled for this environment" -- which is the reason wasm tools
could only ever be skipped. Adding node costs about 3 KB of JS shim and
changes nothing for the browser.

A module that cannot run at all now fails rather than passing quietly. A
tool's own non-zero exit is still only reported, matching how the native
path treats it, since some tools exit non-zero by design.

Verified against artifacts built with the hub's own EM_FLAGS: a working
wasm-only tool passes and genuinely runs; a truncated module fails with
the load error; a tool emitting the wrong output type fails on the type
mismatch rather than passing; a native tool behaves exactly as before,
still receiving absolute paths; and a recipe with neither artifact still
skips.
Two faults found by driving the harness with deliberately awkward tools.

The exit status was always reported as zero. The code assumed
EXIT_RUNTIME=1 makes returning from main throw ExitStatus, so it read
the status only from a caught exception. callMain in fact returns main's
value and throws nothing for a normal return -- a tool returning 3 was
recorded as 0, and the "exited with status" warning could never fire.
Both paths are handled now: the return value, and a thrown ExitStatus
for a program that calls exit() rather than returning.

Files matching an input were not written back, on the reasoning that a
tool should not overwrite what it was given. But editing a file in place
is a normal pattern, and under the native runtime the tool writes
straight into the working directory, so the two runtimes disagreed about
what had been produced. Everything except the harness's own spec file is
written back now.

Also corrected the message for exit -1: it means the module did not
complete, which covers a trap or an abort part way through, not only a
failure to load.

Verified against tools built with the hub's EM_FLAGS that: return 3 is
reported as 3 and warns; a file edited in place survives; an abort after
a partial write still fails; binary content with NUL and 0xFF bytes
round-trips unchanged; and a tool writing a file named like the
harness's spec cannot corrupt the result. Rebuilding and retesting the
real edlib recipe still passes.
Review of the previous commits found three things that would have made
this change worse than useless.

hub test could not fail. test_cmd printed the list of failing tools and
returned, so the command exited 0 and CI passed regardless. Everything
these commits add was log text. It now exits non-zero.

Nineteen recipes would have gone from skipped to failing, with a
diagnosis that blamed the recipe. Only the emscripten builder gained
node in ENVIRONMENT; the other nineteen are built by the biowasm
container, whose flags live in its own bin/shared.sh and were out of
reach. Such a module refuses to start outside a browser, which says
nothing about whether the tool works. Two changes: the harness reports a
refusal of that kind distinctly, and the test skips it with a plain
reason instead of failing; and the biowasm image is patched at build
time to add node, so these tools become testable once the image is
rebuilt. Passing the wasm bytes to the module also removes its need to
fetch them, which it cannot do here.

The -1 sentinel collided with a real exit status. Returning -1 is an
ordinary way for a tool to reject its arguments, and it was
indistinguishable from a trap, so the output was discarded and the tool
reported as broken. Loading and completion are separate fields now, and
an exit status is only ever an exit status.

Also: a tool writing non-UTF-8 to stderr raised UnicodeEncodeError out
of print(), which escaped and left every remaining recipe untested; that
text is now made printable, and each tool is tested inside its own
handler so one cannot end the run.

Verified: a browser-only module skips and does not fail; a genuinely
broken module still fails with a real diagnostic; a tool returning -1
keeps its output and is not mistaken for a trap; non-UTF-8 stderr no
longer aborts the run; hub test exits 1 on failure and 0 on a skip; and
rebuilding and retesting the real edlib recipe still passes.
The previous commit added node to biowasm's ENVIRONMENT with a sed
against bin/shared.sh in the builder image. That file is being rewritten
on sbom-implementation, which pins the biowasm commit by SHA and
verifies it, pins the base image by digest, records both in a lockfile,
and drops privileges in the container. Patching the same file here would
conflict with that for no good reason, and the version there is the
better base for the change.

Nothing else in this branch depended on it. Recipes built by the biowasm
container still refuse to start outside a browser, and the test still
reports that as a skip with a plain reason rather than a failure, which
is the behaviour that matters for turning this on safely. Adding node
belongs wherever that Dockerfile settles.
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.

1 participant