hmon: add positive path unit tests for HMON FFI#83
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
There was a problem hiding this comment.
Pull request overview
This PR adds positive path unit tests for the Health Monitor FFI (Foreign Function Interface) layer. These tests verify that the C-compatible FFI functions work correctly for successful code paths. The tests cover builder creation, destruction, deadline monitor addition, health monitor building, deadline monitor retrieval, starting the health monitor, and resource cleanup.
Changes:
- Added 7 unit tests covering positive paths for all HMON FFI functions
- Tests verify handle creation, manipulation, and basic cleanup operations
- Negative path testing is deferred due to abort behavior in error cases
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c4f40a1 to
f9f7448
Compare
f9f7448 to
e985288
Compare
e985288 to
6ef7f1d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn test_health_monitor_builder_add_deadline_monitor_ok() { | ||
| let health_monitor_builder_handle = health_monitor_builder_create(); |
There was a problem hiding this comment.
Test names in this repo appear to use descriptive snake_case without a test_ prefix or _ok suffix. Consider renaming this test accordingly to match existing conventions.
| fn test_health_monitor_builder_build_ok() { | ||
| let health_monitor_builder_handle = health_monitor_builder_create(); |
There was a problem hiding this comment.
Test names in this repo appear to use descriptive snake_case without a test_ prefix or _ok suffix. Consider renaming this test accordingly to match existing conventions.
| fn test_health_monitor_get_deadline_monitor_ok() { | ||
| let health_monitor_builder_handle = health_monitor_builder_create(); |
There was a problem hiding this comment.
Test names in this repo appear to use descriptive snake_case without a test_ prefix or _ok suffix. Consider renaming this test accordingly to match existing conventions.
| fn test_health_monitor_start_ok() { | ||
| let health_monitor_builder_handle = health_monitor_builder_create(); |
There was a problem hiding this comment.
Test names in this repo appear to use descriptive snake_case without a test_ prefix or _ok suffix. Consider renaming this test accordingly to match existing conventions.
| let health_monitor_builder_handle = health_monitor_builder_create(); | ||
| let deadline_monitor_tag = IdentTag::new("deadline_monitor"); | ||
| let deadline_monitor_builder_handle = deadline_monitor_builder_create(); | ||
| health_monitor_builder_add_deadline_monitor( | ||
| health_monitor_builder_handle, | ||
| &deadline_monitor_tag as *const IdentTag, | ||
| deadline_monitor_builder_handle, | ||
| ); | ||
| let health_monitor_handle = health_monitor_builder_build(health_monitor_builder_handle, 200, 100); |
There was a problem hiding this comment.
These tests repeat the same FFI setup/teardown pattern (create builder, add deadline monitor, build, optionally get deadline monitor). Consider extracting a small helper (or RAII wrapper) to reduce duplication and make cleanup rules for consumed handles harder to get wrong as more cases are added.
6ef7f1d to
f41385b
Compare
| use crate::IdentTag; | ||
|
|
||
| #[test] | ||
| fn test_health_monitor_builder_create_ok() { |
Only positive path is tested. Negative path will require rework to avoid aborts.
f41385b to
ab9e0e8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let health_monitor_builder_handle = health_monitor_builder_create(); | ||
| let health_monitor_handle = health_monitor_builder_build(health_monitor_builder_handle, 200, 100); | ||
| assert!(!health_monitor_handle.is_null()); |
There was a problem hiding this comment.
The build calls repeat magic numbers for cycle durations (e.g., 200 and 100). Consider introducing local constants (e.g., SUPERVISOR_CYCLE_MS, INTERNAL_CYCLE_MS) so the units/intent are clearer and changes only need to be made in one place.
| #[test] | ||
| fn health_monitor_start_ok() { | ||
| let health_monitor_builder_handle = health_monitor_builder_create(); | ||
| let deadline_monitor_tag = IdentTag::new("deadline_monitor"); | ||
| let deadline_monitor_builder_handle = deadline_monitor_builder_create(); | ||
| health_monitor_builder_add_deadline_monitor( | ||
| health_monitor_builder_handle, | ||
| &deadline_monitor_tag as *const IdentTag, | ||
| deadline_monitor_builder_handle, | ||
| ); | ||
| let health_monitor_handle = health_monitor_builder_build(health_monitor_builder_handle, 200, 100); | ||
|
|
||
| let deadline_monitor_handle = | ||
| health_monitor_get_deadline_monitor(health_monitor_handle, &deadline_monitor_tag as *const IdentTag); | ||
| assert!(!deadline_monitor_handle.is_null()); | ||
|
|
||
| health_monitor_start(health_monitor_handle); | ||
|
|
||
| // Clean-up. | ||
| deadline_monitor_cpp_destroy(deadline_monitor_handle); | ||
| health_monitor_destroy(health_monitor_handle); |
There was a problem hiding this comment.
health_monitor_start_ok starts a background thread; the subsequent destroy/join can block for up to internal_cycle_ms because the worker may be sleeping. To keep the unit test suite fast and reduce timing sensitivity, consider using much smaller cycle values here (still respecting the multiple-of invariant).
Only positive path is tested.
Negative path will require rework to avoid aborts.