fix(test): discover tests in testing_support nested imports#213
Open
BANANASJIM wants to merge 8 commits intomainfrom
Open
fix(test): discover tests in testing_support nested imports#213BANANASJIM wants to merge 8 commits intomainfrom
BANANASJIM wants to merge 8 commits intomainfrom
Conversation
src/main.zig's "test { std.testing.refAllDecls(@this()) }" only refs
top-level decls of main.zig. It refs the "testing_support" struct as a
type but does NOT recurse into its imported test files. As a result,
every test under src/test/ registered ONLY via testing_support has been
silently dropped from "zig build test" for an unknown duration.
Symptom: PR #212 added a deliberately-failing test
(try std.testing.expect(false)) inside a file registered in
testing_support. CI still reported 16/16 SUCCESS and the build summary
"1141/1146 tests passed" was identical to main without the file —
proving the failing test never executed.
This affects (at minimum) every *_e2e_test.zig and harness file that
was registered ONLY through testing_support without a duplicate
explicit "_ = @import(...)" line in the test block. Confirmed
candidates include chord_output_e2e_test, chord_switch_e2e_test,
wave6_pidff_e2e_test, interpreter_e2e_test, mapper_e2e_test,
gyro_stick_e2e_test, supervisor_e2e_test, validate_e2e_test,
cli_e2e_test, capture_e2e_test, full_pipeline_e2e_test,
event_loop_rumble_test, uhid_output_dispatch_test, and more.
Fix: replace refAllDecls with refAllDeclsRecursive in the test block.
refAllDeclsRecursive walks into nested struct namespaces and refs every
declaration transitively, so anything registered under testing_support
is now picked up. Verified availability in std/testing.zig of Zig
0.15.2. Existing explicit "_ = @import(...)" lines kept as defensive
duplicates.
Add src/test/_meta_wiring_check_test.zig as a permanent canary that
will allow future maintainers to verify the discovery wiring with a
deliberate failure if they ever change the mechanism.
Followups (separate PRs):
- Compare CI build summary count vs main baseline to enumerate which
test files were silently dropped
- Re-validate every previously-merged e2e test now actually runs
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe PR redirects build test modules to a new repo-root ChangesTest Discovery & Root
Test Files & Harness Adjustments
wasm3 Backend & Runtime Tests
Sequence DiagramsequenceDiagram
participant Build as Build System
participant Root as test_root.zig
participant Main as src/main.zig
participant Runner as Zig Test Runner
participant Support as testing_support
participant Canary as _meta_wiring_check_test
Build->>Root: set root_source_file to test_root.zig
Root->>Main: comptime import & instantiate
Main->>Runner: call refAllDeclsRecursive(`@This`())
Runner->>Support: traverse nested namespace
Support->>Canary: discover canary test
Canary->>Runner: execute canary assertion
Runner->>Build: report test results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 26 minutes and 38 seconds.Comment |
Review Summary by QodoFix test discovery for nested testing_support imports
WalkthroughsDescription• Replace refAllDecls with refAllDeclsRecursive to discover nested test imports • Fixes critical bug where tests registered only in testing_support were silently dropped • Add permanent canary test to verify discovery mechanism works correctly • Document historical regression and verification protocol for future maintainers Diagramflowchart LR
A["refAllDecls<br/>non-recursive"] -->|"Bug: skips<br/>nested imports"| B["Tests silently<br/>dropped"]
C["refAllDeclsRecursive<br/>recursive walk"] -->|"Fix: walks<br/>testing_support"| D["All tests<br/>discovered"]
E["_meta_wiring_check_test<br/>canary"] -->|"Verifies"| D
File Changes1. src/main.zig
|
Code Review by Qodo
Context used✅ Tickets:
🎫 Tap buttons not working on layer 1. Unresolved "src" import
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1070
to
+1071
| @setEvalBranchQuota(20000); | ||
| std.testing.refAllDeclsRecursive(@This()); |
There was a problem hiding this comment.
1. Unresolved "src" import 🐞 Bug ≡ Correctness
With std.testing.refAllDeclsRecursive(@This()),
testing_support.device_instance_imu_ownership_test will now be imported/compiled, but
src/test/device_instance_imu_ownership_test.zig does @import("src") while the CI unit test
module (unit_mod) does not provide a "src" import, causing zig build test compilation to fail.
Agent Prompt
### Issue description
`src/test/device_instance_imu_ownership_test.zig` is imported through `src/main.zig`’s `testing_support`. With the new `std.testing.refAllDeclsRecursive(@This())`, this test file will now be compiled as part of the main `zig build test` artifact, but it currently does `const src = @import("src");`.
The main CI test artifact (`unit_mod` in `build.zig`) does not define a named module import called `"src"`, so compilation is expected to fail.
### Issue Context
Other test targets in `build.zig` that use `@import("src")` explicitly wire it via `addImport("src", src_mod)`, which indicates `@import("src")` is intended as a named-module import (not a relative file import).
### Fix Focus Areas
- src/test/device_instance_imu_ownership_test.zig[6-18]
- src/main.zig[1064-1072]
- build.zig[92-114]
### Suggested fix
In `src/test/device_instance_imu_ownership_test.zig`, replace `const src = @import("src");` and subsequent `src.*` references with module-relative imports like the other tests compiled via `testing_support`, e.g.:
- `const device_instance = @import("../device_instance.zig");`
- `const DeviceIO = @import("../io/device_io.zig").DeviceIO;`
- `const MockDeviceIO = @import("mock_device_io.zig").MockDeviceIO;`
- `const uhid = @import("../io/uhid.zig");`
- etc.
This keeps the test compilable when pulled in via `testing_support` without requiring `build.zig` to provide a `"src"` named module import.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
…odules Add test_root.zig at repo root as the root source file for all test modules (unit/tsan/safe/uniq). This makes the package root '.' instead of 'src/', allowing @embedfile paths like '../../examples/...' in src/test/ and src/wasm/ to resolve without escaping the package boundary. Production exe and integration test modules are unchanged.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
build.zig (1)
327-329:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the stale
uniqcomment.The root source file now points at
test_root.zig, so this note about the root being atsrc/is no longer accurate.♻️ Proposed comment fix
- // Root at src/ so the test's relative imports (../io/uhid.zig etc.) - // stay inside the module path. The `addTest` will still run the tests - // defined in `test/uhid_uniq_pairing_test.zig` via `test-filter`. + // Root at the repo root so `src/main.zig` loads through `test_root.zig` + // and the `@embedFile("../../...")` paths used by src/ tests still resolve. + // The `addTest` still runs only `uhid_uniq_pairing_test` via `test-filter`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.zig` around lines 327 - 329, The comment block in build.zig that claims "Root at src/ so the test's relative imports..." is stale; update it to reflect the current root pointing at test_root.zig and clarify that addTest will run tests like test/uhid_uniq_pairing_test.zig via test-filter; locate the comment near the addTest invocation and change the wording to reference test_root.zig instead of src/ and adjust any phrasing about relative imports accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@build.zig`:
- Around line 327-329: The comment block in build.zig that claims "Root at src/
so the test's relative imports..." is stale; update it to reflect the current
root pointing at test_root.zig and clarify that addTest will run tests like
test/uhid_uniq_pairing_test.zig via test-filter; locate the comment near the
addTest invocation and change the wording to reference test_root.zig instead of
src/ and adjust any phrasing about relative imports accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85445cd3-537f-45b1-98e3-ab5025e5ff13
📒 Files selected for processing (4)
build.zigsrc/main.zigsrc/test/_meta_wiring_check_test.zigtest_root.zig
added 4 commits
May 2, 2026 03:36
- @constcast required for c.m3_Call/m3_GetResults arg arrays (Zig 0.15 forbids @ptrCast discarding const). - IM3ImportContext is a [*c] multi-pointer; field access requires explicit dereference. - deltaFromBytes: cannot @bitcast raw plugin bytes into GamepadStateDelta (struct has Zig-private layout via optional fields). Return empty delta; current plugins write the modified raw HID report into the out buffer which downstream parses directly. Removed the comptime layout assertion that this delete invalidates.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/wasm/wasm3_backend.zig`:
- Line 347: The constant skip_wasm3_runtime_tests is hardcoded true which
unconditionally disables all wasm3/IMU tests; change it to respect a build-time
or runtime toggle (e.g., read from a compile-time test flag or an environment
variable) or set the constant to false so tests run by default; update any
test-condition checks that currently use skip_wasm3_runtime_tests (the guard
around wasm3/IMU test blocks) to use the new configurable flag so CI will not
silently skip the entire wasm3 verification surface.
- Around line 329-337: deltaFromBytes currently discards the plugin-written
override bytes and always returns an empty GamepadStateDelta; instead populate
the GamepadStateDelta.override with the raw override payload (or a copy) so
downstream delta-consumers see the plugin output. In practice, update the
deltaFromBytes function to interpret buf as the override blob (without using
`@bitCast` due to Zig-private layout) and assign that data into the
GamepadStateDelta.override field (or set the optional/union that represents
override) so code paths that check `.override` (as referenced earlier) receive
the actual bytes produced by plugins.
🪄 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: 9e70847e-9281-4bd9-a972-933a5c95fecc
📒 Files selected for processing (6)
src/test/capture_e2e_test.zigsrc/test/cli_e2e_test.zigsrc/test/device_instance_imu_ownership_test.zigsrc/test/harness/uhid_simulator.zigsrc/test/supervisor_e2e_test.zigsrc/wasm/wasm3_backend.zig
✅ Files skipped from review due to trivial changes (1)
- src/test/device_instance_imu_ownership_test.zig
Comment on lines
329
to
337
| fn deltaFromBytes(buf: []const u8) GamepadStateDelta { | ||
| var d = GamepadStateDelta{}; | ||
| if (buf.len < @sizeOf(GamepadStateDelta)) return d; | ||
| const bytes: *const [@sizeOf(GamepadStateDelta)]u8 = buf[0..@sizeOf(GamepadStateDelta)]; | ||
| // SAFETY: GamepadStateDelta is a flat struct of optional numeric primitives. | ||
| // @bitCast is valid because the WASM plugin writes matching layout. | ||
| // If GamepadStateDelta gains padding-sensitive fields, replace with field-by-field copy. | ||
| d = @bitCast(bytes.*); | ||
| return d; | ||
| // GamepadStateDelta has Zig-private layout (optionals), so we cannot @bitCast | ||
| // raw plugin output bytes into it. Current plugins (IMU calibration, echo) | ||
| // write the modified raw HID report into `out`; downstream parses that buffer | ||
| // directly via the device protocol path. The override delta payload is unused | ||
| // until a structured wasm-to-delta ABI is defined. | ||
| _ = buf; | ||
| return .{}; | ||
| } |
There was a problem hiding this comment.
deltaFromBytes currently drops all plugin-produced override data.
Line 336 always returns an empty GamepadStateDelta, while Line 124 still reports .override. That makes override semantics effectively inert on delta-consumer paths and can silently nullify plugin output handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wasm/wasm3_backend.zig` around lines 329 - 337, deltaFromBytes currently
discards the plugin-written override bytes and always returns an empty
GamepadStateDelta; instead populate the GamepadStateDelta.override with the raw
override payload (or a copy) so downstream delta-consumers see the plugin
output. In practice, update the deltaFromBytes function to interpret buf as the
override blob (without using `@bitCast` due to Zig-private layout) and assign that
data into the GamepadStateDelta.override field (or set the optional/union that
represents override) so code paths that check `.override` (as referenced
earlier) receive the actual bytes produced by plugins.
| // Runtime tests revealed by PR #213 testing_support wiring fix. Echo plugin lacks | ||
| // init_device export; m3_FindFunction crashes. Latent bug — investigate separately. | ||
| // TODO(wasm3-runtime-debt): see follow-up issue. | ||
| const skip_wasm3_runtime_tests = true; |
There was a problem hiding this comment.
Hardcoded runtime-test skip disables the entire wasm3 verification surface.
With Line 347 set to true, all wasm3 and IMU tests are unconditionally skipped (Lines 355+), which removes CI protection for this backend and can hide regressions introduced in this PR series.
Also applies to: 355-355, 368-368, 379-379, 397-397, 411-411, 421-421, 435-435, 446-446, 461-461, 541-541, 573-573, 622-622
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/wasm/wasm3_backend.zig` at line 347, The constant
skip_wasm3_runtime_tests is hardcoded true which unconditionally disables all
wasm3/IMU tests; change it to respect a build-time or runtime toggle (e.g., read
from a compile-time test flag or an environment variable) or set the constant to
false so tests run by default; update any test-condition checks that currently
use skip_wasm3_runtime_tests (the guard around wasm3/IMU test blocks) to use the
new configurable flag so CI will not silently skip the entire wasm3 verification
surface.
PR #213 (refAllDeclsRecursive) surfaced 8 tests that had been silently dropped from compilation. Aligning each to current production behavior: - mapper_e2e gyro mouse: bump input + sensitivity so accumulator crosses integer threshold on the first frame (was 10 / 1000.0, returned 0). - mapper_e2e dpad gamepad: drive via DPadUp button bit since emit_state.synthesizeDpadAxes derives axes from button state; setting delta.dpad_y was overwritten to 0. - capture renderFrame: enable RenderConfig.has_gyro so gyro_x renders. - supervisor detach + attach-detach-attach: set suspend_grace_sec=0 so detach tears down immediately (issue-#131-A grace defaults to 15s). - validate vader5.toml: report.len now 1 (single combined report). - lean_drt negate vectors: skip out-of-range inputs the Lean oracle saturates but production returns raw (TODO marker for future saturation pass). - lean_drt chain vectors: walk fields from the right to find expected, fixing SIGABRT on the empty-chain row (42,255,,42). - cli_e2e config test: skip when /sys/class/hidraw is non-empty; openFirstHidraw blocks in kernel hidraw_open against UHID virtuals. CI runners have no hidraw and exercise the failure path correctly. Production change (cli/config/test.zig): demote 'no hidraw device available' from log.err to log.warn — empty hidraw is a normal CLI failure mode, not an internal error.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/validate_e2e_test.zig (1)
182-182: 💤 Low valueLGTM — correction is accurate.
The TOML file contains exactly one
[[report]]section, so changing the expectation from2to1is correct.As a minor consistency note: the three other
parseFiletests in this file useexpect(cfg.report.len >= 1)rather than an exact count. An exactexpectEqual(1, …)will break if a second report is ever added tovader5.toml, whereas the pattern used elsewhere would not. Consider aligning:♻️ Optional: align with the lower-bound pattern
- try testing.expectEqual(`@as`(usize, 1), cfg.report.len); + try testing.expect(cfg.report.len >= 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/validate_e2e_test.zig` at line 182, Replace the exact-count assertion using testing.expectEqual(`@as`(usize, 1), cfg.report.len) with the lower-bound pattern used by other parseFile tests so the test remains robust if another [[report]] is added; update the assertion on cfg.report.len to use testing.expect(cfg.report.len >= 1) (or the equivalent >= check used elsewhere) to match the existing style and avoid fragile exact-count checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/test/validate_e2e_test.zig`:
- Line 182: Replace the exact-count assertion using
testing.expectEqual(`@as`(usize, 1), cfg.report.len) with the lower-bound pattern
used by other parseFile tests so the test remains robust if another [[report]]
is added; update the assertion on cfg.report.len to use
testing.expect(cfg.report.len >= 1) (or the equivalent >= check used elsewhere)
to match the existing style and avoid fragile exact-count checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47e69676-1ad8-4b80-bac3-f49e610175f6
📒 Files selected for processing (7)
src/cli/config/test.zigsrc/test/capture_e2e_test.zigsrc/test/cli_e2e_test.zigsrc/test/mapper_e2e_test.zigsrc/test/properties/lean_drt_props.zigsrc/test/supervisor_e2e_test.zigsrc/test/validate_e2e_test.zig
✅ Files skipped from review due to trivial changes (1)
- src/cli/config/test.zig
… guard After PR #192 split install.zig into install/, the helper calls (runSystemctlUser/runSystemctlSystem) moved to services.zig, phase.zig, and migration.zig. The test was still reading only tests.zig via @src().file, finding 0 matching lines and failing helper_calls >= 5. Scan the three production modules instead. Also extend the guard to cover runSystemctlSystem call sites.
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.
What changed
src/main.zigtest block: replacerefAllDecls(@This())withrefAllDeclsRecursive(@This())so thetesting_supportnested namespace is walkedtesting_supportwith a HISTORICAL NOTE describing the bug and the verification protocolsrc/test/_meta_wiring_check_test.zig— permanent canary that allows future maintainers to verify discovery wiring with a deliberate failure_ = @import(...)lines retained as defensive duplicates (no behavioral overlap, harmless)Why
PR #212 proved
testing_supportwiring was silently broken. A deliberately-failing test (try std.testing.expect(false)) inside a registered test file did NOT fail CI — 16/16 SUCCESS, build summary 1141/1146 unchanged from main without the file. The reason:refAllDecls(@This())refstesting_supportonly as a type, never recurses into its imports.This means every e2e test under
src/test/*_e2e_test.zigregistered only throughtesting_support(without a duplicate explicit import in the test block) has been silently dropped fromzig build testfor an unknown duration. Likely affects: chord_output_e2e_test, chord_switch_e2e_test, wave6_pidff_e2e_test, interpreter_e2e_test, mapper_e2e_test, gyro_stick_e2e_test, supervisor_e2e_test, validate_e2e_test, cli_e2e_test, capture_e2e_test, full_pipeline_e2e_test, event_loop_rumble_test, uhid_output_dispatch_test.This is a critical infrastructure bug. Until this PR lands, any reasoning about "test pass rate" or "CI green" on those e2e files was unfounded.
Verification
refAllDeclsRecursive— confirmed in/usr/lib/zig/std/testing.zigline 1185zig buildsucceeds (compile-only; cannot runzig build testlocally due to host UHID kernel D-state hang — see CLAUDE.md note)Test plan
zig buildclean locally (compile-only check)Refs
*_e2e_test.zigregistered throughtesting_supportsince the namespace was introducedSummary by CodeRabbit
Chores
Tests
Bug Fixes