Repository navigation
test(smoke): move moq-dev/smoke into test/smoke - #6046
Conversation
Verbatim copy of https://github.com/moq-dev/smoke/tree/66dd9f4 into test/smoke/, adapted to the monorepo in the following commits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adapt the imported moq-dev/smoke harness to test/smoke: its own nested flake and stable toolchain, a lockfile ban scoped to the directory, the Cloudflare client outside the cargo workspace, a path-filtered Smoke workflow, and the relay report published under /relays/ on moq's single gh-pages site. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 4 minutes. View limit details
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01d30e130b
ℹ️ 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".
| # c / c-pkgconfig / c-cmake: same libmoq client built three ways, to | ||
| # exercise the moq.pc and CMake-package consumer paths, not just -I/-L. | ||
| subs="rust,python,go,kotlin,c,c-pkgconfig,c-cmake,gst,$browser,$native" | ||
| if [ "$RUNNER_OS" = "macOS" ]; then subs="$subs,swift"; fi |
There was a problem hiding this comment.
Because every active matrix entry uses Ubuntu, this condition is never true, so the newly imported Swift client is not installed or exercised by any scheduled or PR run. A broken moq-swift release or Swift interoperability regression can therefore leave the nightly workflow green; keep at least one macOS lane active or add a separate nightly Swift job.
AGENTS.md reference: AGENTS.md:L27-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Declining here: the macOS cells (the only Swift runner) were disabled on purpose in moq-dev/smoke to cut CI spend, and this PR moves the harness without changing its coverage. Re-enabling them is the maintainer's budget call; it's uncommenting the matrix entries.
(Written by Claude Opus 5.5)
🤖 Addressed by Claude Code
| [[ "$status" -eq 134 ]] || return "$status" | ||
| echo "warning: moq-rs subscriber aborted during teardown; retrying once" >&2 | ||
| RUST_BACKTRACE="${RUST_BACKTRACE:-1}" run_client "$PY" "$CLIENTS/python/smoke.py" \ | ||
| subscribe --url "$URL" --broadcast "$broadcast" --timeout "$TIMEOUT" |
There was a problem hiding this comment.
When the published Python subscriber receives media and then exits with SIGABRT, this retry can make the cell pass on its second invocation, hiding the packaging/runtime failure the smoke suite is meant to report. The first failed exchange should remain red rather than being replaced by a fresh connection.
AGENTS.md reference: test/smoke/AGENTS.md:L11-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid against smoke's own no-retry rule, but this retry predates the move and removing it may turn the nightly red on a known moq-rs teardown abort. That needs the abort reproduced and fixed at its source, so it goes in its own PR rather than this verbatim move.
(Written by Claude Opus 5.5)
🤖 Addressed by Claude Code
There was a problem hiding this comment.
Follow-up: #6064 (stacked on this branch) removes this retry, keeping this PR a verbatim move.
(Written by Claude Opus 5.5)
| if (!reloaded && Date.now() - start > timeoutMs / 2) { | ||
| reloaded = true; | ||
| await page.reload({ waitUntil: "load" }).catch(() => {}); |
There was a problem hiding this comment.
If the published watch element loses its initial catalog subscription with RESET_STREAM, reloading halfway through creates a second subscription and can turn that real early-subscription failure into a green cell. This masks a user-visible timing regression whenever the subscriber starts before the publisher announcement; report the first exchange as failed instead of retrying it.
AGENTS.md reference: test/smoke/AGENTS.md:L11-L12
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid: the reload hides @moq/watch giving up on a catalog subscribed before the announce, which players should instead wait out. It predates the move, and fixing it means fixing @moq/watch and then deleting the reload, so it goes in its own PR rather than this verbatim move.
(Written by Claude Opus 5.5)
🤖 Addressed by Claude Code
There was a problem hiding this comment.
Follow-up: #6064 (stacked on this branch) removes this retry, keeping this PR a verbatim move.
(Written by Claude Opus 5.5)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The negative pass minted the unrelated key with the verifier under test and only warned when that failed, so a verifier could pass without ever being shown a foreign key. Mint it with the canonical generator, which just worked, and fail the cell otherwise. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary:
(Written by Claude Opus 5.5) |
Align the audit with main's benchmark and smoke replans (#6081, #6046, #6022): the trend site is now the m0 regression table plus m2 benchmark sources, so the audit's m2/bench-site edits fold into m0/bench-site (the unprovisioned git.moq.dev HTTPS certificate) and its links retarget there. catalog-readers stays deleted, and the compile-time audit takes main's recorded baselines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
test/smoke/flake.lock arrived with the smoke suite (#6046) without update coverage; its nixpkgs dates from May. It gets its own entry so a toolchain bump, which must carry the matching playwright pin, never blocks the root flake. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Moves moq-dev/smoke (published-package interop smoke) into
test/smoke/so it lives next to the code it tests. Imported as a plain copy of smokemainat66dd9f4, still its head (its open PRs, moq-dev/smoke#26 through moq-dev/smoke#30, moq-dev/smoke#50 and moq-dev/smoke#52, have not landed; they can be re-opened here); PRs here squash-merge, so a subtree merge would not have kept its history anyway.What changes
test/smoke/: the harness (smoke.sh,relays.sh,cloudflare.sh,moxygen.sh,token.sh,freshness.sh,clients/,report/,justfile,smoke*.toml, README) with its own nested flake (nix develop ./test/smoke, or direnv inside the directory). Its toolchain floats on its ownflake.lock, separate from moq's pins; the flake comment no longer claims no lock ships.CLAUDE.md+AGENTS.mdmerged into a nestedtest/smoke/AGENTS.mdthat keeps only the smoke-specific rules (the generic ones are moq's root rules). ItsPROMPTING.md,.editorconfig(folded into the root one) and dependabot config are dropped; moq's dependabot already covers the workflow.freshness.shbans lock files undertest/smoke/only (it used togit ls-filesthe whole repo and would have tripped on moq'sCargo.lock/bun.lock/uv.lock); the nested.gitignorestill ignores them. A nestedrust-toolchain.toml(stable) keeps the cargo channel on the newest stable instead of moq's pinned toolchain.clients/cloudflareis its own cargo[workspace]; Bun workspaces are an explicit list, so smoke's JS clients install standalone from npm (checked: localnode_modules, rootbun.lockuntouched);test/tsconfig.jsonexcludessmokeso tsc never type-checks the published-package clients against the in-tree@moq/*. Biome does lint them now: formatting plus four small HTML/regex fixes..github/workflows/smoke.yml: nightly at 08:00,workflow_dispatch, andpull_request/pushpath-filtered totest/smoke/**and the workflow, so ordinary moq PRs don't pay for the ~4 x 45-75 min matrix. Every step runs ajustrecipe (sh/gh/check-runs.ts), fromtest/smoke. Nomerge_grouptrigger, so the queue is unaffected.https://moq-dev.github.io/moq/relays/. moq has one Pages site (gh-pages, written by nightly'sbenchmark-site), so the newrelays-reportjob commits onlyrelays/to gh-pages (report/publish.sh, reusingbench-publish.sh init), deploys the whole branch, and joins nightly'sbenchmark-pagesconcurrency group. Each writer touches only its directory, so neither deploy drops the other's pages. History seeds from/moq/relays/history.json.Smokeis added toalert.yml's watched list and its schedule-only cron set.interop.yml,test/justfile,test/interop/{README.md,interop.sh}and two quests now point attest/smoke.just test smoke [recipe] [args]runs any smoke recipe inside the nested shell.Maintainer follow-ups after merge
/repos/moq-dev/moq/pagesis 404, and no gh-pages branch exists yet), so both this deploy and nightly'sbenchmark-sitedeploy fail until it is on.mainwithhistory_url=https://moq-dev.github.io/smoke/history.jsonbefore the first nightly publishes.The nix cell pushes moq HEAD builds to cachix when the
CACHIX_AUTH_TOKENsecret is present (moq has it forcachix.yml), as smoke did.Public API / wire impact
None. Test harness and CI only.
Validation
just check(moq root, branch scope) andjust gh check(actionlint, alert coverage, recipe-only steps): pass.nix develop ./test/smoke --command just check(shfmt, shellcheck, actionlint, freshness): pass.just test smoke(Rust-only matrix through the nested shell, publishedmoq-relay/moq0.17.7 fromcargo install --lockedviaRELAY_BIN/MOQ_BIN):rust -> rust [moq-lite-06]PASS.report/publish.shagainst a local bare remote: creates gh-pages when absent, replacesrelays/(stale logs removed) and leaves an existingbench/intact.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)