ci(go): make the FFI tests run instead of silently skipping - #273
Open
YuanYuYuan wants to merge 5 commits into
Open
ci(go): make the FFI tests run instead of silently skipping#273YuanYuYuan wants to merge 5 commits into
YuanYuYuan wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Ensures Go FFI CI tests execute against a built Rust FFI library and fail when the library is unavailable.
Changes:
- Builds
hirozwith theffifeature before Go FFI tests. - Makes
--ffi-onlyrequire the Rust library instead of silently skipping.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.github/workflows/ci.yml |
Builds the required static FFI library. |
scripts/test-go.nu |
Enforces library availability for FFI-only tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The "Go FFI tests" step gated on `has-ffi-library` and, finding none, logged "Skipping hiroz FFI tests" and reported success. Nothing in CI ever built with `--features ffi`, so the step has never executed a single test while appearing green on every PR. That is how a missing re-entrancy guard in `RawPublisher::publish_bytes` shipped unnoticed -- the path `rmw-zenoh-rs` publishes through -- and it is why the committed cbindgen header has drifted from the generator. Build the library in the job, and turn the skip into an error so the absence of a library can never again read as a pass. Refs #270
Adversarial review caught that `error make` sat in `test-runtime`, which is reached by three dispatch paths: --ffi-only (intended), --runtime-only, and the no-flag default. A Go-only contributor with no Rust toolchain running `nu scripts/test-go.nu` after a fresh clone previously got warnings and all four phases; with the error they got a hard abort at phase 2, and test-examples and test-vet -- neither of which needs the library -- never ran. Gate it behind --require-ffi, set only by the --ffi-only path that CI uses. The default and --runtime-only keep skipping with the warning. --codegen-only was never affected.
The step name and issue reference carry it; the rest belonged in the PR description, where it already is.
YuanYuYuan
force-pushed
the
ci/ffi-tests-must-run
branch
from
August 14, 2026 18:23
198e4e3 to
594d462
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
Go FFI testsstep in theGo Testsjob has never run a test. It reports success anyway.Part of #270, and the cheapest part of it.
The defect
nu scripts/test-go.nu --ffi-onlyci.ymlL617-L618libhiroz.aexistshas-ffi-library, guarding the FFI branchgo-testsjob builthirozwith--features ffigo-testsjob in.github/workflows/ci.ymltest-runtimeinscripts/test-go.nuEvery run logged
Skipping hiroz FFI tests (Rust library not built). The step concludedsuccess.What that leaves unchecked:
pub unsafe extern "C"functionscrates/hiroz/src/ffi/ffi.github/workflows/,scripts/test-pure-rust.nu,scripts/test-ros.nuInterop Testsbuilds it with--features ffi, then runs--integrationpaths-ignorelisthirozpackage unit tests--ffi-onlycoversThe gap is not hypothetical. #270 records a re-entrancy defect on
publish_bytesthat manual review found, not any check. That is the pathrmw-zenoh-rspublishes through. Its fix is #250, still open.What this PR does
hirozwith--features ffi --releasebefore the FFI step, so the tests have something to link against.github/workflows/ci.yml--require-ffitotest-runtime; a missing library becomes anerror make, not a warningscripts/test-go.nu--require-ffionly from--ffi-onlyscripts/test-go.nuTwo files, +21/-2.
--ffi-onlyis the path CI uses, and running the FFI tests is its entire point. The default and--runtime-onlypaths still warn and skip, so a Go-only contributor without a Rust toolchain can run the suite.--codegen-onlyand--vet-onlynever touched the library. This PR does not change them.Evidence
All measurements taken on
198e4e3d4a50b412bae9822c0bfdcba325d414a4, the current head.Go FFI testsstep ofGo Tests (ubuntu-latest)printed 119--- PASSlines andok github.com/ZettaScaleLabs/hiroz/crates/hiroz-go/hiroz--- FAILlinesSkipping hiroz FFI testsdoes not appear in that job's logGo Tests (ubuntu-latest)andGo Tests (macos-latest)are bothSUCCESSBreaking changes
None to library code.
test-go.nu --ffi-onlywith nolibhiroz.acargo build -p hiroz --features ffi --releasefirstThe default invocation,
--runtime-only,--codegen-onlyand--vet-onlyare not affected by BC1.Coverage this does not have
None of these block this change. They bound what green CI proves.
Important
This does not close #270.
cbindgenis not installed on the runners.build.rsturns its absence into a non-fatalcargo:warning=. The Go side compiles against the committedcrates/hiroz-go/hiroz/hiroz_ffi.h.--features ffifor clippy is deliberately out of scope.test-integrationandtest-examplesstill skip and succeed when the library is absenttest-integrationandtest-examples.Caveats on the table above:
hiroz_context_config_tends atbool enable_logging, while Rust'sCContextConfigcarries a furthernamespace: *const c_char. fix(ffi): regenerate the drifted cbindgen header and gate it in CI #277 fixes that drift and adds the gate. Nothing here detects drift of either kind. FFI module is never compiled, linted or tested; the "Go FFI tests" CI step silently skips #270 tracks installingcbindgenand gating ongit diff --exit-code. TheGo Testslog on this head containswarning: hiroz@0.1.0: cbindgen not found (No such file or directory (os error 2)), skipping header generation.# Safetysection — 13 inaction.rs, 6 inservice.rs, 3 inserialize.rs.hiroz_free_bytesalso has the implicitfrom_raw_parts_mutcast. Writing 22 perfunctory# Safetyblocks without establishing each contract would be worse than leaving them. Those are items 1-3 of FFI module is never compiled, linted or tested; the "Go FFI tests" CI step silently skips #270.Go Testsjob invokes neither. TheInterop Testsworkflow builds the library before calling--integration, so neither skips in practice today. This PR fixes neither.