Skip to content

test(device_instance): add imu_name ownership ordering regression test#195

Merged
BANANASJIM merged 1 commit intomainfrom
chore/arch-imu-name-ownership-test
Apr 29, 2026
Merged

test(device_instance): add imu_name ownership ordering regression test#195
BANANASJIM merged 1 commit intomainfrom
chore/arch-imu-name-ownership-test

Conversation

@BANANASJIM
Copy link
Copy Markdown
Owner

@BANANASJIM BANANASJIM commented Apr 29, 2026

Summary

  • Adds src/test/device_instance_imu_ownership_test.zig with a recording-allocator harness
  • The allocator intercepts rawFree calls and records the order of destroy(imu_dev) vs free(imu_name_owned) in a global event log
  • Also reads UHID_DESTROY opcode from the pipe write-end to confirm imu_dev.close() actually ran
  • Wired into build.zig as an independent b.addTest module (same pattern as supervisor_uhid_routing_test.zig), hooked to test_step
  • Added to testing_support in src/main.zig for module graph completeness

Test plan

  • zig build compiles cleanly (no errors)
  • zig build check-fmt passes
  • If deinit body is reordered (free(imu_name_owned) before destroy(imu_dev)), g_log[0] becomes free_imu_name and expectEqual(destroy_imu_dev, g_log[0]) fails
  • If imu_dev.close() is skipped, UHID_DESTROY read returns 0 bytes and expect(n >= @sizeOf(u32)) fails

refs: architecture-review-v0.1.4.md finding #9 (option B)

Summary by CodeRabbit

  • Tests
    • Added a regression test for device instance IMU ownership and resource cleanup behavior.

@qodo-code-review
Copy link
Copy Markdown

ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 29, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

The PR adds a new regression test verifying that DeviceInstance deinit properly cleans up resources in the correct order, specifically testing that a UhidDevice is destroyed before its associated imu_name buffer is freed.

Changes

Cohort / File(s) Summary
Test Module Registration
src/main.zig
Imports new regression test module device_instance_imu_ownership_test into testing_support namespace.
Device Instance Cleanup Test
src/test/device_instance_imu_ownership_test.zig
New regression test that constructs a DeviceInstance with owned resources, calls deinit(), and verifies cleanup occurs in the correct order: UhidDevice.close() (via pipe monitoring for UHID_DESTROY message) followed by imu_name buffer deallocation. Uses RecordingAllocator to track and assert allocator events.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 A test hops through with careful eyes,
Checking cleanup in the right sequence and size,
UhidDevice first, then memory freed,
Resources dance as the rabbit decreed! 🎀

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: adding a regression test for imu_name ownership ordering in DeviceInstance, which directly corresponds to the new test file and the PR's primary objective.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch chore/arch-imu-name-ownership-test

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.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 60 minutes.

Comment @coderabbitai help to get the list of available commands and usage tips.

@BANANASJIM BANANASJIM force-pushed the chore/arch-imu-name-ownership-test branch 2 times, most recently from 8f36986 to ecf8d2b Compare April 29, 2026 16:02
@BANANASJIM BANANASJIM enabled auto-merge (squash) April 29, 2026 16:04
@BANANASJIM BANANASJIM force-pushed the chore/arch-imu-name-ownership-test branch from ecf8d2b to ac71376 Compare April 29, 2026 16:08
Adds a recording-allocator harness that captures the order of
allocator.destroy(imu_dev) and allocator.free(imu_name_owned) during
DeviceInstance.deinit. Also reads the UHID_DESTROY opcode from the pipe
to confirm close() ran. Any future reordering of the deinit body will
flip the log entries and fail the assertions.

refs: architecture-review-v0.1.4.md finding #9
@BANANASJIM BANANASJIM force-pushed the chore/arch-imu-name-ownership-test branch from ac71376 to 6879228 Compare April 29, 2026 16:28
@BANANASJIM BANANASJIM merged commit 92830f1 into main Apr 29, 2026
17 of 18 checks passed
@BANANASJIM BANANASJIM deleted the chore/arch-imu-name-ownership-test branch April 29, 2026 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant