test: add in-tree cross-language interop smoke test#1871
Merged
Conversation
Build every client from this checkout and run a publish x subscriber matrix against a local relay, so an interop regression shows up before anything is published. The standalone moq-dev/smoke repo installs each client from its public registry to catch packaging breakage in a release; this is its in-tree companion, building from source (cargo/bun/uv/cc/gst) with no distribution-mechanism matrix. It lives under test/, wired through the existing `mod test`. - test/smoke/smoke.sh: orchestrator. Each client builds from the workspace: cargo for relay/cli/libmoq/moq-gst, `just py build` for the maturin editable install, the bun workspace for JS. - Clients: rust + python + browser publish; everyone subscribes (incl. native node/bun JS, C via libmoq, GStreamer moqsrc). - `just test smoke` / `smoke-full` / `smoke-negative`; a nightly + dispatch + PR-on-harness-change workflow runs it inside `nix develop`. - The two JS clients join the bun workspace so @moq/* resolve to source. - Native-JS subscribers are judged by the "received N bytes" marker, not the exit code: the @moq/web-transport NAPI addon can segfault during bun's exit teardown after a frame has arrived, which would otherwise turn a real success into a signal exit. Verified on macOS: full matrix 21/21 green, negative control passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR introduces a complete in-tree cross-language MOQ interop smoke test framework. A Bash harness ( 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
.github/workflows/smoke.yml (1)
15-17: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winBroaden PR path filters to include smoke wiring contracts.
The smoke job depends on
test/justfiletargets and rootpackage.jsonworkspace entries, but PR triggers currently exclude both. Adding them improves PR-time regression detection.Proposed update
pull_request: paths: - "test/smoke/**" + - "test/justfile" + - "package.json" - ".github/workflows/smoke.yml"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/smoke.yml around lines 15 - 17, The paths filter in the smoke workflow currently only includes test/smoke/** and .github/workflows/smoke.yml, but the smoke job also depends on test/justfile targets and root package.json workspace entries. Add both "test/justfile" and "package.json" to the paths list alongside the existing entries to ensure the smoke job is triggered on PRs that modify these dependencies, enabling better regression detection at PR-time.test/smoke/clients/c/subscribe.c (1)
64-64: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider extracting the latency constant.
The magic number
1000here matchesMAX_LATENCY_MSin the Python client. A named constant would improve clarity and keep cross-language test parameters in sync.+#define MAX_LATENCY_MS 1000 + // ... in on_catalog: - int32_t track = moq_consume_video_ordered((uint32_t)catalog, 0, 1000, on_frame, ud); + int32_t track = moq_consume_video_ordered((uint32_t)catalog, 0, MAX_LATENCY_MS, on_frame, ud);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/smoke/clients/c/subscribe.c` at line 64, Extract the magic number 1000 from the moq_consume_video_ordered function call into a named constant called MAX_LATENCY_MS to improve code clarity and maintain consistency with the Python client's latency parameter. Define this constant at the top of the file and replace the hardcoded 1000 with a reference to this named constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/smoke/clients/js-native/subscribe.ts`:
- Around line 35-39: The timeout value parsed on line 35 is not validated after
being calculated from Number.parseFloat. Add validation after the timeoutMs
assignment to check that the value is finite and positive using
Number.isFinite() and a comparison check. If validation fails, log an error
message indicating the invalid timeout value and call process.exit(2) to match
the existing error handling pattern for the role, url, and broadcast parameters
validated in the subsequent if statement.
- Around line 1-13: Add a module-level JSDoc documentation block at the very top
of the subscribe.ts file before any imports. Convert the existing plain comments
that describe the file's purpose and usage into a proper JSDoc block using the
/** ... `@module` */ format, ensuring it comes before the import statement for
parseArgs from "node:util". This block should document the module's purpose as
an entrypoint and describe its functionality.
In `@test/smoke/clients/js/driver.ts`:
- Line 27: The timeoutMs assignment accepts non-finite values from
Number.parseFloat without validation, which can cause hangs or unexpected
failures. After parsing the timeout value with Number.parseFloat, validate that
the resulting number is finite using Number.isFinite before multiplying by 1000.
If the parsed value is not a valid finite number, throw an error with a clear
message indicating invalid timeout input to provide appropriate usage feedback.
- Around line 1-7: The driver.ts file currently has plain-text comments at the
top instead of a module-level JSDoc block. Convert the existing top-of-file
comments (the lines explaining the script's purpose and describing how to drive
Chromium against the vite-built page, including the usage examples) into a JSDoc
documentation block using the /** ... `@module` */ format. Preserve all the
existing comment content but wrap it in proper JSDoc syntax with the `@module` tag
at the beginning, making this an official module-level documentation block as
required for entrypoint files.
In `@test/smoke/clients/js/src/main.ts`:
- Around line 1-2: Replace the plain comment lines at the top of the main.ts
entrypoint file with a proper JSDoc documentation block that includes the
`@module` tag. Convert the existing comments into a /** ... `@module` */ block
format to comply with the documentation guidelines for JS entrypoints. This
ensures the module has proper documentation that identifies it as an entrypoint
module.
In `@test/smoke/clients/js/vite.config.ts`:
- Around line 1-12: Add JSDoc documentation to this vite configuration file.
First, add a module-level documentation block at the very top of the file
(before any imports) using the `@module` tag to document the purpose of this
file. Then, add a doc comment directly above the `export default
defineConfig(...)` statement to document the exported configuration object.
Ensure both doc comments follow the project's JSDoc conventions and clearly
describe their respective purposes.
In `@test/smoke/README.md`:
- Around line 74-82: The fenced code block starting with smoke.sh is missing a
language tag, which violates markdownlint rule MD040. Add the language tag
"text" to the opening triple backticks of this code block by changing ``` to
```text to specify that it is plain text content.
In `@test/smoke/smoke.sh`:
- Around line 60-63: Add numeric validation for the TIMEOUT and SMOKE_PORT
variables immediately after they are assigned from command-line arguments (the
--timeout and --port options). For each variable, check if the assigned value
contains only digits and is within a valid range, and if validation fails,
output a clear error message and exit the script. This prevents unclear errors
later when these invalid values are passed to the timeout command or used in sed
replacements.
---
Nitpick comments:
In @.github/workflows/smoke.yml:
- Around line 15-17: The paths filter in the smoke workflow currently only
includes test/smoke/** and .github/workflows/smoke.yml, but the smoke job also
depends on test/justfile targets and root package.json workspace entries. Add
both "test/justfile" and "package.json" to the paths list alongside the existing
entries to ensure the smoke job is triggered on PRs that modify these
dependencies, enabling better regression detection at PR-time.
In `@test/smoke/clients/c/subscribe.c`:
- Line 64: Extract the magic number 1000 from the moq_consume_video_ordered
function call into a named constant called MAX_LATENCY_MS to improve code
clarity and maintain consistency with the Python client's latency parameter.
Define this constant at the top of the file and replace the hardcoded 1000 with
a reference to this named constant.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa4e332d-bf5b-44a6-a6a0-bf0f0268f6e3
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.github/workflows/smoke.ymlpackage.jsontest/justfiletest/smoke/README.mdtest/smoke/clients/c/subscribe.ctest/smoke/clients/js-native/package.jsontest/smoke/clients/js-native/subscribe.tstest/smoke/clients/js/driver.tstest/smoke/clients/js/index.htmltest/smoke/clients/js/package.jsontest/smoke/clients/js/src/main.tstest/smoke/clients/js/src/setup.tstest/smoke/clients/js/vite.config.tstest/smoke/clients/python/smoke.pytest/smoke/smoke.shtest/smoke/smoke.toml
- Add @module doc blocks to the JS entrypoints (driver, subscribe, main, vite.config) per the js/ entrypoint convention. - Trigger the smoke workflow on test/justfile + package.json changes too, since the job depends on the recipe and the workspace membership. - Tag the README layout code block as `text`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reject a non-finite/non-positive --timeout in the JS clients and a non-numeric --timeout / SMOKE_PORT in smoke.sh, so a fat-fingered value fails with a clear usage error instead of silent degenerate behavior (an instant-or-never timeout, a cryptic bind failure). The clients are documented for standalone debugging where these are hand-typed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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
An in-tree cross-language interop smoke test under
test/smoke/that builds every client from this checkout and runs a publish × subscribe matrix against a local relay. It proves the code in the tree interoperates across implementations before anything is published.This is the in-tree companion to moq-dev/smoke: that repo installs each client from its public registry (crates.io, PyPI, npm, ...) to catch packaging breakage in a release; this one builds from source (
cargo/bun/uv/cc/gst) to catch interop regressions in the current commit. No apt/brew/npm/PyPI, no distribution-mechanism matrix. It lives undertest/, wired through the existingmod test, next to wheretest/justfilealready points at the standalone repo.What's here
test/smoke/smoke.sh— the orchestrator. Same matrix engine as the upstream repo (mark_brokenisolation,kill_tree, negative control), but every client builds from the workspace:cargo buildfor the relay/cli/libmoq/moq-gst,just py build(maturin editable into.venv) for Python, the bun workspace for JS.moqsrc). Rust/python/browser publish; everyone subscribes.just test smoke/smoke-full/smoke-negative(intest/justfile)..github/workflows/smoke.yml— nightly + on-demand + on PRs that touchtest/smoke/, runningjust test smoke-fullinsidenix developonubuntu-latest.package.json+bun.lock) as private members, so@moq/*resolve to this checkout's source. The browser page reuses the in-repoworkletInline()vite plugin since@moq/publishis consumed as source.Why a few things look the way they do
cargo build -p moq-gstandgst-launch-1.0run against one consistent GStreamer insidenix develop.received N bytesmarker, not the exit code. The@moq/web-transportNAPI addon can segfault during bun's exit teardown after a frame has arrived, which would turn a real success into a signal exit. The data path (frame bytes arrived) is exactly our success criterion, so the harness reads the marker; the negative control confirms it still reports failure when there's genuinely no data.Scope notes
main.moq-rswraps themoq_ffibindings), and each binding is built + tested byjust go/swift/kt check. Adding them to the smoke would need toolchains absent from the devShell (go+uniffi-bindgen-go,java+gradle) and a macOS runner for swift — low marginal value. Easy to add later viamark_broken's per-client isolation.Test plan
Verified locally on macOS via
nix develop:just test smoke-full— 21/21 PASS (3 publishers × 7 subscribers), exit 0just test smoke-negative(incl.js-native-bun) — every subscriber correctly fails (negative pass)shfmt/shellcheck/biome/remark/actionlint/just --fmtall cleanjust js check/build/testcleanly skip the new workspace members; no build artifacts leak into git🤖 Generated with Claude Code
(Written by Claude)