test: overhaul firmware test infrastructure (parallel runner, tautology removal, xs unification, device smoke) - #543
Merged
Conversation
Stop deleting build output before every run (mcconfig regenerates the makefile each invocation and mc.xs.c depends on the manifest include chain, so incremental builds are safe; STACKCHAN_MODULE_TEST_CLEAN=1 restores clean builds) and run tests through a worker pool (STACKCHAN_MODULE_TEST_JOBS, default min(4, cores)). Parallel mcsim instances need two isolations: a private DBus session per test (mcsim is a fixed-application-id GtkApplication, so a second instance on the same bus hands off to the first and exits 0) and a per-test XDG_CONFIG_HOME (mcsim copies mc.so into its config directory and concurrent instances SIGBUS each other). One warm-up build per platform segment avoids racing on the shared XS core lib directory. Full suite: ~10min serial -> 3m39s cold / under 1min warm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nvariants Architecture tests that re-asserted implementation lines, manifest values, or constants broke on every legitimate edit while catching no regressions. Delete the pure mirrors (app-capabilities, splash-view, camera, consts.test, input-event.test) and trim the mixed files down to glob-based invariants, layer boundaries, and tombstones. Where a deleted assertion encoded a real constraint, keep it as a relational check instead of a literal mirror: subplatforms are now discovered dynamically and compared against their base target's creation block, camera-defining subplatforms must be gated into the camera/conversation manifests (catches the historical new-subplatform-forgotten case the old hardcoded list could not), and config.serial must align with config.driver.serial. Non-derivable whys move into source comments (boot ordering in main.ts, touch polling mode in the FT6206 driver). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ts into xs The Node and xs variants were near-verbatim duplicates (motion-controller) or the xs variant was a strict subset (camera-preview-utils). Port the Node-only cases (gaze release via lookAt(undefined), close cancelling polling and torque-release timers, duration helpers, big-endian conversion, frame copy byte order) into the xs tests, which run on the real XS engine, and delete the Node copies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Each manifest.test.json pays a full mcconfig build, which is not worth it for 10-line construct-and-trace-ok smokes. Merge the TTS engines, stt-whisper, network-service, and the Claude/Gemini dialogue smokes into host/modules/__tests__/module-smoke and mods/examples/provider-dialogues/__tests__/dialogue-smoke (29 -> 22 manifests). The led smoke stays separate because it is lighting's only test and check:architecture requires each runtime module to own one. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
npm run test:device installs the m5stackchan_smoke MOD through mcrun -dn (no xsbug GUI), bridges device traces via serial2xsbug into the shared xsbug log server, and judges pass/fail from the smoke MOD's completion line with automatic retries for the flaky CoreS3 xsbug serial bridge. --channel serial falls back to watching the raw console for crash markers (boot-stability only; trace() is not visible on raw serial). The device table moves to scripts/lib/devices.mjs, shared with firmware.mjs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughChangesTesting and smoke execution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant DeviceSmokeRunner
participant XsbugLogServer
participant Mcrun
participant Device
Operator->>DeviceSmokeRunner: run test:device
DeviceSmokeRunner->>XsbugLogServer: start temporary server
DeviceSmokeRunner->>Mcrun: launch smoke manifest
Mcrun->>Device: install and execute MOD
Device->>XsbugLogServer: send execution logs
XsbugLogServer->>Mcrun: respond with <go/>
DeviceSmokeRunner->>Operator: report result or retry
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 157d1a895d
ℹ️ 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".
![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: 2
🧹 Nitpick comments (1)
firmware/scripts/lib/devices.mjs (1)
1-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider exporting
resolveDevicealongsidedevicesandaliases.Both
firmware.mjs(lines 134-140) andrun-device-smoke.js(lines 49-55) define near-identicalresolveDevicefunctions that differ only in the log prefix ([stack-chan]vs[device-smoke]). Since this module was created to centralize device configuration, exporting a sharedresolveDevicehere would eliminate the duplication.♻️ Suggested addition to `devices.mjs`
export const aliases = { default: 'm5stackchan_cores3', m5stackchan: 'm5stackchan_cores3', rt: 'stackchan_rt', takao: 'takao_core2_sg90', } + +export function resolveDevice(value, prefix = '[stack-chan]') { + const name = aliases[value] ?? value + if (devices[name]) return name + console.error(`${prefix} Unknown device: ${value}`) + console.error(`${prefix} Supported devices: ${Object.keys(devices).join(', ')}`) + process.exit(1) +}Then in
firmware.mjsandrun-device-smoke.js, replace the localresolveDevicewith the imported one (passing the appropriate prefix).🤖 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 `@firmware/scripts/lib/devices.mjs` around lines 1 - 24, Export a shared resolveDevice function from the devices module alongside devices and aliases, accepting the caller’s log prefix so it preserves the existing firmware and smoke-test messages. Remove the duplicate local resolveDevice implementations in firmware.mjs and run-device-smoke.js, import the shared function, and update their calls to pass the appropriate prefix.
🤖 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 `@firmware/host/modules/ui/state/face-state.architecture.ts`:
- Around line 62-91: Add a non-empty result assertion immediately after each
extractMethodBlocks(source, 'onFaceState') call in the two tests, before
iterating, so both tests fail when no onFaceState block is detected. Keep the
existing per-block allocation assertions unchanged.
In `@firmware/scripts/lib/xsbug-log-server.js`:
- Around line 42-49: Update the server setup and returned close method in the
log-server factory to track each connected socket, remove sockets when they
close, and destroy all remaining sockets before or while invoking
server.close(). Ensure the close promise resolves deterministically even when
clients keep connections open, while preserving the existing ready and getLog
behavior.
---
Nitpick comments:
In `@firmware/scripts/lib/devices.mjs`:
- Around line 1-24: Export a shared resolveDevice function from the devices
module alongside devices and aliases, accepting the caller’s log prefix so it
preserves the existing firmware and smoke-test messages. Remove the duplicate
local resolveDevice implementations in firmware.mjs and run-device-smoke.js,
import the shared function, and update their calls to pass the appropriate
prefix.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 420df1d6-7f56-430c-9a01-82205ce773bb
📒 Files selected for processing (53)
.github/actions/setup/action.ymlCLAUDE.mdfirmware/docs/m5stackchan-cores3-smoke.mdfirmware/host/app/app-capabilities.architecture.tsfirmware/host/app/main.tsfirmware/host/modules/__tests__/module-smoke/manifest.test.jsonfirmware/host/modules/__tests__/module-smoke/module-smoke.test.tsfirmware/host/modules/audio/__tests__/audio-buffer-ownership.architecture.tsfirmware/host/modules/audio/__tests__/stt-whisper/stt-whisper.test.tsfirmware/host/modules/audio/__tests__/tts-elevenlabs/manifest.test.jsonfirmware/host/modules/audio/__tests__/tts-elevenlabs/tts-elevenlabs.test.tsfirmware/host/modules/audio/__tests__/tts-local/manifest.test.jsonfirmware/host/modules/audio/__tests__/tts-local/tts-local.test.tsfirmware/host/modules/audio/__tests__/tts-openai/manifest.test.jsonfirmware/host/modules/audio/__tests__/tts-openai/tts-openai.test.tsfirmware/host/modules/audio/__tests__/tts-voicevox-web/manifest.test.jsonfirmware/host/modules/audio/__tests__/tts-voicevox-web/tts-voicevox-web.test.tsfirmware/host/modules/audio/__tests__/tts-voicevox/manifest.test.jsonfirmware/host/modules/audio/__tests__/tts-voicevox/tts-voicevox.test.tsfirmware/host/modules/camera/camera.architecture.tsfirmware/host/modules/connectivity/__tests__/network-service/manifest.test.jsonfirmware/host/modules/connectivity/__tests__/network-service/network-service.test.tsfirmware/host/modules/input/input-event.test.tsfirmware/host/modules/motion/__tests__/m5stackchan-servo.test.tsfirmware/host/modules/motion/__tests__/motion-controller.test.tsfirmware/host/modules/motion/__tests__/motion-controller/motion-controller.test.tsfirmware/host/modules/preferences/consts.architecture.tsfirmware/host/modules/preferences/consts.test.tsfirmware/host/modules/testing/module-structure.architecture.tsfirmware/host/modules/ui/application/app-controller.architecture.tsfirmware/host/modules/ui/components/face/parts/__tests__/shape-cache.test.tsfirmware/host/modules/ui/components/face/parts/image/__tests__/image-avatar-pack.architecture.tsfirmware/host/modules/ui/components/face/parts/image/__tests__/image-avatar-pack.test.tsfirmware/host/modules/ui/state/face-state.architecture.tsfirmware/host/modules/ui/views/camera-preview/__tests__/camera-preview-utils/camera-preview-utils.test.tsfirmware/host/modules/ui/views/camera-preview/camera-preview-utils.test.tsfirmware/host/modules/ui/views/settings/settings-view.architecture.tsfirmware/host/modules/ui/views/splash/splash-view.architecture.tsfirmware/host/platforms/m5stackchan_cores3/host/ft6206_async_m5stackchan.jsfirmware/host/platforms/platform-manifest.architecture.tsfirmware/host/platforms/wasm/wasm-stubs.architecture.tsfirmware/host/platforms/wasm/wasm-stubs.test.tsfirmware/mods/examples/provider-dialogues/__tests__/dialogue-claude/dialogue-claude.test.tsfirmware/mods/examples/provider-dialogues/__tests__/dialogue-claude/manifest.test.jsonfirmware/mods/examples/provider-dialogues/__tests__/dialogue-gemini/dialogue-gemini.test.tsfirmware/mods/examples/provider-dialogues/__tests__/dialogue-smoke/dialogue-smoke.test.tsfirmware/mods/examples/provider-dialogues/__tests__/dialogue-smoke/manifest.test.jsonfirmware/package.jsonfirmware/scripts/firmware.mjsfirmware/scripts/lib/devices.mjsfirmware/scripts/lib/xsbug-log-server.jsfirmware/scripts/run-device-smoke.jsfirmware/scripts/run-module-tests.js
💤 Files with no reviewable changes (26)
- firmware/host/modules/connectivity/tests/network-service/manifest.test.json
- firmware/host/modules/ui/views/splash/splash-view.architecture.ts
- firmware/mods/examples/provider-dialogues/tests/dialogue-claude/manifest.test.json
- firmware/host/modules/audio/tests/tts-voicevox/tts-voicevox.test.ts
- firmware/host/modules/audio/tests/tts-elevenlabs/tts-elevenlabs.test.ts
- firmware/host/app/app-capabilities.architecture.ts
- firmware/mods/examples/provider-dialogues/tests/dialogue-claude/dialogue-claude.test.ts
- firmware/host/modules/preferences/consts.test.ts
- firmware/host/modules/audio/tests/tts-openai/manifest.test.json
- firmware/host/modules/audio/tests/tts-local/tts-local.test.ts
- firmware/host/modules/audio/tests/tts-openai/tts-openai.test.ts
- firmware/host/modules/audio/tests/tts-voicevox-web/manifest.test.json
- firmware/host/modules/motion/tests/motion-controller.test.ts
- firmware/host/modules/audio/tests/stt-whisper/stt-whisper.test.ts
- firmware/host/modules/ui/views/camera-preview/camera-preview-utils.test.ts
- firmware/host/modules/input/input-event.test.ts
- firmware/mods/examples/provider-dialogues/tests/dialogue-gemini/dialogue-gemini.test.ts
- firmware/host/modules/audio/tests/tts-elevenlabs/manifest.test.json
- firmware/host/modules/audio/tests/tts-voicevox/manifest.test.json
- firmware/host/modules/audio/tests/tts-voicevox-web/tts-voicevox-web.test.ts
- firmware/host/modules/audio/tests/tts-local/manifest.test.json
- firmware/host/modules/connectivity/tests/network-service/network-service.test.ts
- firmware/host/modules/camera/camera.architecture.ts
- firmware/host/modules/preferences/consts.architecture.ts
- firmware/host/modules/ui/components/face/parts/image/tests/image-avatar-pack.test.ts
- firmware/host/modules/motion/tests/m5stackchan-servo.test.ts
biome ci enforces organizeImports, which lint/format do not. The piu test builds timed out on 4-core CI runners: each mcconfig already compiles with make --jobs 8 internally, so four concurrent builds oversubscribed the runner. Default the worker pool to min(4, cores - 2) and double the build timeout for cold caches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- assert extractMethodBlocks found an onFaceState block before iterating, so a regex miss cannot silently pass the allocation checks - destroy lingering sockets in the xsbug log server's close() so shutdown cannot hang on a surviving simulator or serial2xsbug connection - share resolveDevice from scripts/lib/devices.mjs instead of duplicating it in firmware.mjs and run-device-smoke.js Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 14, 2026
This was referenced Jul 28, 2026
This was referenced Aug 6, 2026
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.
概要
firmwareテストのレビュー結果に基づく4領域の改善です。
1. Moddableテストランナーの高速化（インクリメンタル＋並列）
run-module-tests.jsは毎回クリーンビルド×完全直列でした。mcconfigはmakefileを毎回再生成しmc.xs.cがmanifest include チェーンに依存するためインクリメンタルビルドは安全と確認し、クリーン削除をSTACKCHAN_MODULE_TEST_CLEAN=1に退避。ワーカープール（STACKCHAN_MODULE_TEST_JOBS、既定 min(4, cores)）で並列化しました。mcsimの並列実行には2つの隔離が必要でした：
dbus-run-session）: mcsimは固定application-idのGtkApplicationで、同一バス上の2個目は1個目に委譲して即終了するmc.soを config dir にコピーして実行するため、共有すると相互にSIGBUS2. 追認型テストの削減
実装行・manifest値をそのまま再掲するarchitectureテストを削除・変換しました。純ミラーの5ファイル（app-capabilities / splash-view / camera / consts.test / input-event.test）を削除、7ファイルをglob型不変条件・レイヤ境界・tombstoneのみにトリム。実質的な制約は関係チェックに変換：サブプラットフォームを動的走査してベースターゲットとのcreation一致を検証、
defines.cameraを持つsubplatformの camera/conversation manifest ゲート必須チェック（過去に実際に発生した追加漏れを検出可能）など。非自明なwhyはソースコメントへ移設。3. Node/xs重複テストのxs一本化
motion-controller（ほぼ逐語重複）と camera-preview-utils（xs版が部分集合）を、Node版のみのケースを移植した上でxs版へ一本化。
4. スモーク統合＋実機スモークランナー
コンストラクタスモーク級のテスト9件を
module-smoke/dialogue-smokeの複合manifestへ統合（29→22 manifest）。npm run test:deviceを新設し、mcrun -dn＋xsbugログサーバで実機スモークMODの完走を自動判定（CoreS3のxsbugブリッジ不安定対策のリトライ、--channel serialフォールバック付き）。リリース影響
none — テスト・CI・開発スクリプトのみの変更のため、リリースノート/changesetは不要です。プロダクションソースへの変更はwhyコメント2箇所（
main.tsの起動順序、ft6206_async_m5stackchan.jsのポーリング理由）のみで動作変更はありません。CI変更はaptパッケージdbusの明示追加のみ。検証
origin/developベースで以下すべて通過：npm run lint/npm run formatnpm run test:unit（141件）npm run check:architecture（53件）npm run check:legacy-names/npm run check:manifest（6ターゲット）npm run test:moddable（22 manifest、5回連続の並列安定性確認済み）npm run smoke:lin補足：ローカルで
mcp-server-serviceテストが失敗しますが、開発機の live-server が8080を占有しているためで、JOBS=1（従来同等の直列動作）でも再現する既存の環境衝突です。本PRとは無関係で、CIでは発生しません。未検証:
test:deviceの実デバイス経路はハードウェア未接続のため未実施です（引数処理・エラー経路のみ確認済み）。実機での動作確認をお願いします。🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
Chores