Repository navigation
Conversation
- Reapply the shared property harness across the affected suites after the earlier rollback. - Restore the baseline logging/failure behavior needed for later macro fixes.
- Rework the `sinex_prop` macro path so property suites can move back onto a shared harness. - Keep the harness configuration centralized instead of scattering runner setup through individual tests.
Replace the nested executor path with `Handle::current().block_on(...)` so async `sinex_prop` cases run inside the active Tokio runtime while preserving per-case timeouts.
Teach the test macros to write JSON failure artifacts under `target/test-artifacts/` and document the new `SINEX_TEST_FAIL_DIR` override. Include captured `TestContext` metadata when a context-backed test fails.
Sinity
added
type:test
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Test Macro Infrastructure crate/lib/sinex-test-utils/macros/src/lib.rs |
Added new #[sinex_prop] proc-macro and sinex_proptest! block macro for property testing. Removed proptest detection/transformation from #[sinex_test]. Implemented strategy parameter validation, async/sync test generation, failure snapshot persistence, and seed management via config or env vars. |
Test Utilities Core crate/lib/sinex-test-utils/src/lib.rs, src/property_testing.rs, src/snapshot_helper.rs |
Removed PropertyTester struct and trait, replaced with direct #[sinex_prop] macro usage. Added persist_failure() function to write timestamped JSON artifacts on test failure with error details, pool stats, and captured logs. |
Database & Fixture Updates crate/lib/sinex-test-utils/src/database_pool.rs, src/fixture_generator.rs, src/test_context.rs, src/static_fixtures.rs |
Added Serialize derive to PoolStats. Changed fixture generator return types from TestResult to Result. Made CreatedEventInfo visibility pub(crate). Updated imports to conditionally include TestResult. |
sinex-core Property Tests crate/lib/sinex-core/tests/adversarial/enhanced_boundary_test.rs, tests/event_property.rs, tests/property/*.rs, tests/property_tests.rs |
Migrated ~12 property test files from #[sinex_test] + proptest!(...) pattern to #[sinex_prop] or sinex_proptest! macros. Updated test signatures to accept #[strategy(...)] parameters directly; removed manual TestContext construction where now injected. |
sinex-satellite-sdk Property Tests crate/lib/sinex-satellite-sdk/tests/property/*.rs, tests/property_checkpoint_state.rs |
Migrated ~7 property test files from closure-based proptest to #[sinex_prop]/sinex_proptest! macros. Removed custom Tokio runtime plumbing and run_async helpers; async tests now directly use async fn with TestResult<()>. |
sinex-schema Property Tests crate/lib/sinex-schema/tests/ulid_property_tests.rs, tests/ulid_property_from_main_tests.rs, tests/ulid_tests.rs |
Converted property tests from proptest! { ... } to sinex_proptest! macros. Updated ULID test assertions and parameter bindings to use explicit strategy generators; removed standalone test function definitions in favor of macro-generated ones. |
Test Pattern Documentation docs/TEST_PATTERNS.md, docs/TEST_PATTERNS_GUIDE.md |
Replaced examples using PropertyTester and ctx.property_tester() with new #[sinex_prop] and sinex_proptest! patterns. Updated guidance to emphasize #[strategy(...)] attributes, macro configuration options (cases, timeout, seed), and artifact persistence to target/test-artifacts/. |
Test Utilities Documentation crate/lib/sinex-test-utils/README.md |
Replaced "Logging & Metrics" section (per-test JSON logs, SINEX_TEST_METRICS_PATH config) with simplified "Logging" section emphasizing ctx.captured_logs() and failure-only JSON artifacts written to target/test-artifacts/ with SINEX_TEST_FAIL_DIR override. |
Configuration & Scripts .cargo-machete.toml, scripts/dev-env-banner.sh |
Removed duplicate sinex-db ignored features in cargo-machete config; restored trailing newline. Removed test logging and metrics environment variable references from dev banner (SINEX_TEST_LOG_ALL, SINEX_TEST_METRICS_PATH). |
Minor Cleanup crate/lib/sinex-core/src/types/bin/sinex-schema.rs, crate/lib/sinex-core/tests/unit/preflight_test.rs, crate/lib/sinex-satellite-sdk/tests/security/path_validation_test.rs, crate/satellites/sinex-system-satellite/src/unified_processor.rs |
Removed unused uuid::Uuid import. Reformatted test bindings in preflight tests. Wrapped event_tx in Some(...) for BlobManager::new() calls. Made WatcherHandle::name field private by renaming to _name. |
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
🐰 Old closures tucked in
proptest!tight,
Now strategies shine with#[sinex_prop]light,
Parameters dance on function signatures bare,
Failures persist with artifacts to spare,
The harness tests faster, with context so fair! ✨
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The PR title follows Conventional Commits format with type(scope): subject structure, clearly describing a property-test harness restoration with failure snapshots. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feature/test/sinex-prop-failure-snapshots
Comment @coderabbitai help to get the list of available commands and usage tips.
Sinity
added a commit
that referenced
this pull request
Apr 9, 2026
Restore the macro-based property harness on a stable footing, execute async property cases on the active Tokio runtime, and persist structured failure artifacts so regressions are debuggable again. sinex-test-utils macros: - Re-export `sinex_prop` and `sinex_proptest` from `crate/lib/sinex-test-utils/src/lib.rs` and rebuild the procedural macro expansion in `macros/src/lib.rs`, which puts property tests back on a first-class harness instead of a mix of ad-hoc `proptest!` patterns. - Move property-runner setup behind shared helper logic so cases, seeds, shrink behavior, and failure persistence can be configured consistently across crates. runtime execution and failure handling: - Change the async property path in `macros/src/lib.rs` to use `tokio::runtime::Handle::current().block_on(...)`, which fixes the broken “run async property cases from a detached executor” behavior and keeps timeouts tied to the active test runtime. - Add structured failure persistence in `crate/lib/sinex-test-utils/src/snapshot_helper.rs`, writing JSON artifacts under `target/test-artifacts/` (or `SINEX_TEST_FAIL_DIR`) with error details, pool stats, and captured logs/context when available. - Make pool stats serializable and thread the failure snapshot hook through both `sinex_test` and `sinex_prop`, so ordinary async tests and property tests produce comparable failure artifacts. suite migration and docs: - Reapply the harness across the property suites in `sinex-core`, `sinex-schema`, and `sinex-satellite-sdk`, converting older inline `proptest!` blocks to `#[sinex_prop]`/`sinex_proptest!` where needed. - Update `TESTING.md`, `docs/TEST_PATTERNS.md`, `docs/TEST_PATTERNS_GUIDE.md`, and the `sinex-test-utils` README so the preferred property-test surface, seed env vars, and failure-artifact path are documented together. - Remove the short-lived logging/runlog detour and other branch churn so the final harness is centered on macro execution plus snapshot artifacts, not extra always-on telemetry files.
There was a problem hiding this comment.
Pull request overview
Restores a unified property-testing harness (#[sinex_prop] / sinex_proptest!) across the workspace, updates docs to match, and adds JSON failure artifacts so failing tests carry debugging context (including pool stats and optional TestContext logs).
Changes:
- Reintroduce and re-export the
sinex_prop/sinex_proptestmacros; remove legacyPropertyTesterAPI and migrate suites back onto the harness. - Add failure snapshot persistence to
target/test-artifacts/(overridable viaSINEX_TEST_FAIL_DIR) and wire it into#[sinex_test]/#[sinex_prop]. - Update property-test docs and convert property suites across
sinex-core,sinex-schema, andsinex-satellite-sdk.
Reviewed changes
Copilot reviewed 38 out of 39 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| TESTING.md | Updates property-test guidance to use #[sinex_prop] / sinex_proptest!. |
| scripts/dev-env-banner.sh | Removes banner hints related to prior test logging/metrics mode. |
| docs/TEST_PATTERNS.md | Rewrites property-test section to document macro-based harness + env overrides + artifact paths. |
| docs/TEST_PATTERNS_GUIDE.md | Updates examples to the restored macro-based property testing approach. |
| crate/satellites/sinex-system-satellite/src/unified_processor.rs | Renames unused watcher name field to _name to silence warnings. |
| crate/lib/sinex-test-utils/src/test_context.rs | Widens visibility of CreatedEventInfo to pub(crate) for internal use. |
| crate/lib/sinex-test-utils/src/static_fixtures.rs | Adjusts imports to avoid pulling TestResult in non-test builds. |
| crate/lib/sinex-test-utils/src/snapshot_helper.rs | Adds JSON failure artifact persistence (pool stats, timestamp, optional context+logs). |
| crate/lib/sinex-test-utils/src/property_testing.rs | Removes legacy PropertyTester and shifts tests to #[sinex_prop]. |
| crate/lib/sinex-test-utils/src/lib.rs | Re-exports sinex_prop/sinex_proptest and updates prelude accordingly. |
| crate/lib/sinex-test-utils/src/fixture_generator.rs | Cleans up imports/types and switches several APIs to return Result vs TestResult. |
| crate/lib/sinex-test-utils/src/database_pool.rs | Derives Serialize for PoolStats so it can be embedded in failure artifacts. |
| crate/lib/sinex-test-utils/README.md | Updates harness logging docs to describe failure artifacts + captured logs. |
| crate/lib/sinex-test-utils/macros/src/lib.rs | Implements #[sinex_prop] + sinex_proptest!, and wires persist_failure into sinex_test. |
| crate/lib/sinex-schema/tests/ulid_tests.rs | Migrates ULID proptests to sinex_proptest!. |
| crate/lib/sinex-schema/tests/ulid_property_tests.rs | Migrates extensive ULID property suites to sinex_proptest!. |
| crate/lib/sinex-schema/tests/ulid_property_from_main_tests.rs | Replaces manual proptest usage with sinex_proptest! block tests. |
| crate/lib/sinex-schema/docs/architecture.md | Updates docs examples to use sinex_proptest!. |
| crate/lib/sinex-satellite-sdk/tests/security/path_validation_test.rs | Updates BlobManager::new call signature to pass Some(event_tx). |
| crate/lib/sinex-satellite-sdk/tests/property/validation_invariants_property_test.rs | Converts async property execution to macro harness (removes custom runtime). |
| crate/lib/sinex-satellite-sdk/tests/property/satellite_property_test.rs | Migrates multiple satellite property suites to #[sinex_prop] / sinex_proptest!. |
| crate/lib/sinex-satellite-sdk/tests/property/queue_property_test.rs | Migrates property tests to macro harness; uses &TestContext for DB-backed property. |
| crate/lib/sinex-satellite-sdk/tests/property/error_handling_property_test.rs | Migrates error-handling proptests to sinex_proptest!. |
| crate/lib/sinex-satellite-sdk/tests/property/checkpoint_property_test.rs | Migrates checkpoint property suites to #[sinex_prop] (removes custom runtime). |
| crate/lib/sinex-satellite-sdk/tests/property/automation_property_test.rs | Migrates automation-related property tests to sinex_proptest!. |
| crate/lib/sinex-satellite-sdk/tests/property_checkpoint_state.rs | Converts checkpoint-state property tests to sinex_proptest!. |
| crate/lib/sinex-core/tests/unit/preflight_test.rs | Minor formatting cleanup for integration verification calls. |
| crate/lib/sinex-core/tests/property/validation_roundtrip_property_test.rs | Moves DB-backed sanitization property to #[sinex_prop] with &TestContext. |
| crate/lib/sinex-core/tests/property/ulid_property_test.rs | Migrates ULID property suites to sinex_proptest!. |
| crate/lib/sinex-core/tests/property/time_range_property_test.rs | Replaces #[sinex_test(proptest)] with #[sinex_prop] strategies. |
| crate/lib/sinex-core/tests/property/schema_property_test.rs | Migrates schema validation proptests to #[sinex_prop] and strategy attributes. |
| crate/lib/sinex-core/tests/property/path_sanitization_property_test.rs | Migrates path sanitization/validation proptests to #[sinex_prop]. |
| crate/lib/sinex-core/tests/property/event_validation_property_test.rs | Migrates event validation proptests to sinex_proptest! blocks. |
| crate/lib/sinex-core/tests/property/event_model_fuzzing_test.rs | Migrates fuzzing proptests to sinex_proptest! and harness conventions. |
| crate/lib/sinex-core/tests/property_tests.rs | Migrates general core property tests to sinex_proptest!. |
| crate/lib/sinex-core/tests/event_property.rs | Migrates event property tests from manual proptest to sinex_proptest!. |
| crate/lib/sinex-core/tests/adversarial/enhanced_boundary_test.rs | Converts boundary proptest to #[sinex_prop] without building ad-hoc runtimes. |
| crate/lib/sinex-core/src/types/bin/sinex-schema.rs | Removes unused uuid::Uuid import. |
| .cargo-machete.toml | Cleans up machete configuration (removes/normalizes entries). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+294
to
+302
| }).unwrap_or_else(|| { | ||
| quote! { | ||
| if let Ok(seed_env) = std::env::var("SINEX_PROPTEST_SEED").ok().and_then(|s| s.parse::<u64>().ok()) { | ||
| cfg.rng_algorithm = ::proptest::test_runner::RngAlgorithm::ChaCha; | ||
| cfg.rng_seed = Some(seed_env); | ||
| } | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
SINEX_PROPTEST_SEED / SINEX_PROPTEST_CASES env parsing is using if let Ok(...) = std::env::var(...).ok().and_then(...), but .ok().and_then(...) returns an Option, not a Result, so this won’t compile. Switch these to if let Some(val) = ... (or avoid .ok() and match the Result properly) so the runner config can be overridden at runtime.
Comment on lines
+94
to
+95
| Add `cases = 256` (or any number) to lock the runner config. Capture any | ||
| failing seeds in that crate’s `tests/property/*.proptest-regressions` files. |
There was a problem hiding this comment.
The docs say failing seeds should be captured in tests/property/*.proptest-regressions, but the restored harness config in #[sinex_prop] sets proptest failure persistence to target/proptest-regressions by default (and optionally SINEX_PROPTEST_DIR). Please align this guidance with the actual persistence path (either update the doc text or change the default persistence location).
Suggested change
| Add `cases = 256` (or any number) to lock the runner config. Capture any | |
| failing seeds in that crate’s `tests/property/*.proptest-regressions` files. | |
| Add `cases = 256` (or any number) to lock the runner config. Failing seeds | |
| are persisted under `target/proptest-regressions` by default, or under the | |
| directory specified by `SINEX_PROPTEST_DIR` when that override is set. |
Comment on lines
+858
to
+869
| match &result { | ||
| Ok(_) => { | ||
| eprintln!("✅ {} ({:.1?})", test_name.replace('_', " "), elapsed); | ||
| } | ||
| Err(err) => { | ||
| eprintln!("❌ {} ({:.1?})", test_name.replace('_', " "), elapsed); | ||
| sinex_test_utils::snapshot_helper::persist_failure( | ||
| test_name, | ||
| format!("{err:?}"), | ||
| None, | ||
| ); | ||
| } |
There was a problem hiding this comment.
In the rstest branch, failures call persist_failure(..., None), so the JSON artifact will never include TestContext info/logs even when the test case actually uses a context. If has_ctx_param is true, consider restructuring so you can pass Some(&ctx) to persist_failure (or otherwise include context snapshot data).
Comment on lines
+102
to
+108
| let sanitized = test_name.replace("::", "_"); | ||
| let filename = format!( | ||
| "{}-{}.json", | ||
| Utc::now().format("%Y%m%dT%H%M%S%3f"), | ||
| sanitized | ||
| ); | ||
| let path = snapshot_dir.join(filename); |
There was a problem hiding this comment.
Failure snapshot filenames are based on test_name (which is just stringify!(fn_name) in the macros) and only sanitize ::. This can collide or become ambiguous when different modules/crates have the same test function name, and makes artifacts harder to attribute. Consider using a fully-qualified name (e.g., concat!(module_path!(), "::", stringify!(fn_name))) and sanitizing a broader set of characters for filesystem safety.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
.cargo-machete.toml (1)
19-23:⚠️ Potential issue | 🔴 CriticalFix duplicate keys in
[ignored]table—TOML parsing fails.Lines 18–23 contain two duplicate key definitions that prevent the config from parsing:
sinex-terminal-satellite(lines 19, 22)sinex-system-satellite(lines 18, 23)Merge both into single entries with all dependency variants combined:
Proposed fix
sinex-desktop-satellite = ["tracing-subscriber"] sinex-fs-watcher = ["tracing-subscriber"] -sinex-system-satellite = ["tracing-subscriber"] -sinex-terminal-satellite = ["tracing-subscriber"] +sinex-terminal-satellite = ["tracing-subscriber", "xterm-query"] +sinex-system-satellite = ["tracing-subscriber", "battery", "procfs", "systemstat"] # Platform-specific or conditional dependencies -sinex-terminal-satellite = ["xterm-query"] -sinex-system-satellite = ["battery", "procfs", "systemstat"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.cargo-machete.toml around lines 19 - 23, The TOML has duplicate keys in the [ignored] table — specifically the keys "sinex-terminal-satellite" and "sinex-system-satellite" are declared twice; merge each duplicate into a single key and combine their dependency arrays into one array per key (e.g., for sinex-terminal-satellite include both "tracing-subscriber" and "xterm-query" in a single array, and for sinex-system-satellite include "battery", "procfs", and "systemstat" in a single array) so the TOML parser can read the file.crate/lib/sinex-test-utils/src/static_fixtures.rs (1)
419-420:⚠️ Potential issue | 🟡 MinorRemove duplicate import.
use crate::sinex_test;is imported twice on consecutive lines.🔧 Proposed fix
use super::*; use crate::sinex_test; - use crate::sinex_test;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crate/lib/sinex-test-utils/src/static_fixtures.rs` around lines 419 - 420, The file contains a duplicate import of the same symbol; remove the redundant line importing crate::sinex_test so there is only a single `use crate::sinex_test;` statement (locate the duplicate import in static_fixtures.rs and delete the extra occurrence).crate/lib/sinex-schema/tests/ulid_tests.rs (1)
347-393:⚠️ Potential issue | 🔴 CriticalMissing
Ok(())return statements in property tests.The tests within this
sinex_proptest!block don't have explicitOk(())returns. The macro generates functions with-> sinex_test_utils::TestResult<()>return type, but these test bodies end withprop_assert_eq!which returns()on success, notOk(()).Compare with
ulid_property_from_main_tests.rswhere each test explicitly ends withOk(()).🐛 Proposed fix to add Ok(()) returns
sinex_proptest! { fn test_ulid_string_roundtrip(ulid: Ulid in ulid_strategy()) { let ulid_str = ulid.to_string(); let parsed = ulid_str.parse::<Ulid>().unwrap(); prop_assert_eq!(ulid, parsed); + Ok(()) } fn test_ulid_bytes_roundtrip(ulid: Ulid in ulid_strategy()) { let bytes = ulid.to_bytes(); let restored = Ulid::from_bytes(bytes).unwrap(); prop_assert_eq!(ulid, restored); + Ok(()) } fn test_ulid_uuid_roundtrip(ulid: Ulid in ulid_strategy()) { let uuid = ulid.to_uuid(); let restored = Ulid::from_uuid(uuid); prop_assert_eq!(ulid, restored); + Ok(()) } fn test_ulid_ordering_consistency( ulid1: Ulid in ulid_strategy(), ulid2: Ulid in ulid_strategy() ) { // Compare ULIDs let ulid_cmp = ulid1.cmp(&ulid2); // Compare their string representations let str_cmp = ulid1.to_string().cmp(&ulid2.to_string()); // Compare their UUID representations let uuid_cmp = ulid1.to_uuid().cmp(&ulid2.to_uuid()); prop_assert_eq!(ulid_cmp, str_cmp); prop_assert_eq!(ulid_cmp, uuid_cmp); + Ok(()) } fn test_timestamp_extraction_reasonable(ulid: Ulid in ulid_strategy()) { let timestamp = ulid.timestamp(); // Should be within reasonable range (1970 to far future) let unix_epoch = DateTime::from_timestamp(0, 0).unwrap(); let far_future = DateTime::from_timestamp(253402300799, 0).unwrap(); // Year 9999 prop_assert!(timestamp >= unix_epoch); prop_assert!(timestamp <= far_future); + Ok(()) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crate/lib/sinex-schema/tests/ulid_tests.rs` around lines 347 - 393, The property tests inside the sinex_proptest! block (functions test_ulid_string_roundtrip, test_ulid_bytes_roundtrip, test_ulid_uuid_roundtrip, test_ulid_ordering_consistency, test_timestamp_extraction_reasonable) are missing the final Ok(()) return expected by the generated -> sinex_test_utils::TestResult<()> signature; update each test body to return Ok(()) as the last expression so successful assertions produce Ok(()), ensuring compatibility with the macro-generated return type.crate/lib/sinex-core/tests/property/event_model_fuzzing_test.rs (1)
529-576:⚠️ Potential issue | 🔴 CriticalCritical: Orphaned property test functions outside
sinex_proptest!block.
test_system_events_robustness(lines 530-555) andtest_json_serialization_robustness(lines 558-576) are defined outside anysinex_proptest!block. They use proptest parameter syntax (payload in ...) and returnTestResult<()>, but without the macro wrapper they won't compile or register as tests.The
sinex_proptest!block at line 500 closes at line 527, leaving these functions orphaned before the next block opens at line 578.🐛 Proposed fix: wrap orphaned functions in sinex_proptest!
} /// Test system events with extreme payloads +sinex_proptest! { fn test_system_events_robustness( payload in fuzzed_system_payloads(), source in prop_oneof![ Just("dbus"), Just("journald"), ], event_type in prop_oneof![ Just("signal.received"), Just("method.called"), Just("entry.written"), Just("state.changed"), ], ) -> TestResult<()> { // ... existing body ... Ok(()) } /// Test JSON serialization robustness with extreme payloads fn test_json_serialization_robustness( payload in malformed_json_values() ) -> TestResult<()> { // ... existing body ... Ok(()) } +} sinex_proptest! { /// Test ULID robustness with extreme timestamps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crate/lib/sinex-core/tests/property/event_model_fuzzing_test.rs` around lines 529 - 576, Both test_system_events_robustness and test_json_serialization_robustness are defined using proptest-style parameters but sit outside any sinex_proptest! macro, so wrap these two functions inside a sinex_proptest! { ... } block to register them properly; specifically, move the definitions of test_system_events_robustness and test_json_serialization_robustness into a sinex_proptest! macro invocation (or reopen the existing sinex_proptest! block) so their `payload in ...` signatures and TestResult return types are compiled and executed by the proptest harness.crate/lib/sinex-satellite-sdk/tests/property/satellite_property_test.rs (1)
254-278:⚠️ Potential issue | 🔴 CriticalAdd
Ok(())at the end of the test function body.The
sinex_proptest!macro expands this function to returnsinex_test_utils::TestResult<()>(which isResult<(), Error>), but the current body ends with()from the finalassert_eq!call. This is a type mismatch.Required fix
// Test environment-based loading doesn't panic let env_config = SatelliteConfig::load_from_env(&service_name); assert_eq!(env_config.service_name, service_name); + + Ok(()) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crate/lib/sinex-satellite-sdk/tests/property/satellite_property_test.rs` around lines 254 - 278, The test body inside the sinex_proptest! macro (function satellite_config_validation_is_robust) currently ends with assertions that return () causing a type mismatch with the macro's expected sinex_test_utils::TestResult<()>; fix it by appending Ok(()) as the final expression of the function so the return type matches; locate the test defined by sinex_proptest! and add Ok(()) after the last assert_eq! (the code using SatelliteConfig::builder(), config.validate_config(), and SatelliteConfig::load_from_env()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crate/lib/sinex-core/tests/adversarial/enhanced_boundary_test.rs`:
- Around line 343-374: Change the test function signature for
test_property_based_boundaries to return TestResult<()> instead of Result<()>
for consistency with other migrated tests; update the signature from async fn
test_property_based_boundaries(...) -> Result<()> to -> TestResult<()>, ensure
TestResult is in scope (import or fully-qualify, e.g., color_eyre::eyre::Result
as TestResult or whatever project alias is used), and keep the existing Ok(())
return so the body requires no further changes.
In `@crate/lib/sinex-core/tests/property/event_validation_property_test.rs`:
- Around line 354-488: The test file contains a duplicated sinex_proptest! block
re-defining property_payload_size_validation,
property_event_timestamp_consistency, property_event_uniqueness,
property_source_event_id_validation, property_json_schema_compatibility,
property_event_metadata_fields, and property_boundary_condition_handling; remove
the redundant block so each property_* function is defined only once (or
consolidate any intentional differences into the original definitions), leaving
a single sinex_proptest! invocation that contains each of those property tests
exactly once and ensuring helper generators like arbitrary_event() and
metadata_rich_events() are still referenced.
- Around line 501-531: The test currently asserts per-worker ordering on the raw
collected ops but the generator emits (worker_id, operation_id) pairs in
arbitrary order; to fix, sort each worker's ops vector before asserting. In the
property_concurrent_event_ordering test (using concurrent_operation_events and
the by_worker map where ops holds op_id as u64), call a sort (e.g.,
sort_unstable()) on each ops Vec before iterating windows and performing
prop_assert on window[0] <= window[1], so the check validates ordering rather
than generator order.
- Around line 335-350: The property test
test_validation_preserves_error_hierarchy is vacuous because arbitrary_event()
always produces valid events so validate_event(&event) never returns Err; change
the test to either use a generator that can produce invalid events (e.g.
arbitrary_invalid_event or a combined strategy) or mutate the generated event
before validation (e.g. set event.source = "" and/or event.event_type = "") so
that validate_event(&event) can fail and the Err branch runs; update the
assertions accordingly (and make the same change for the analogous test block at
lines 606-636) referencing validate_event, arbitrary_event, and
test_validation_preserves_error_hierarchy to find and fix the code.
In `@crate/lib/sinex-satellite-sdk/tests/property/checkpoint_property_test.rs`:
- Around line 311-365: The test checkpoint_cleanup_maintains_consistency only
creates checkpoints and never invokes the cleanup path; locate the appropriate
cleanup method on CheckpointManager (e.g., cleanup, cleanup_checkpoints or
similar) and call it (preferably on cleaned_manager or a central manager) using
cleanup_threshold before re-reading stats; after calling the real cleanup API
assert that the target automaton's checkpoint was cleaned/aged out while other
managers (created via CheckpointManager::new and saved with save_checkpoint)
still report correct stats via get_checkpoint_stats.
- Around line 95-129: The test incorrectly sets expected_final_count to the last
checkpoint's processed_count instead of the maximum seen, causing failures when
later checkpoints decrease; in the loop that iterates over checkpoints (using
checkpoints, processed_count, CheckpointState), update expected_final_count =
expected_final_count.max(*processed_count) (or otherwise compute the max across
all processed_count values) before saving with
checkpoint_manager.save_checkpoint(), then assert
prop_assert_eq!(state.max_processed, expected_final_count) against
checkpoint_manager.get_checkpoint_stats().
- Around line 278-281: The test assumes a checkpoint exists after the "load"
branch but that may not be true on a clean manager; update the assertion after
checkpoint_manager.get_checkpoint_stats() in the "load" arm so it tolerates no
last_update when nothing was saved. Concretely, replace the unconditional
prop_assert!(stats.last_update.is_some()) with a conditional/assertion that
expects Some only when processed_count > 0 (e.g. assert that
stats.last_update.is_some() == (processed_count > 0)); keep the existing check
of stats.max_processed against processed_count and leave
checkpoint_manager.get_checkpoint_stats() and processed_count unchanged.
In `@crate/lib/sinex-satellite-sdk/tests/property/satellite_property_test.rs`:
- Around line 458-478: The sort currently compares only Id::timestamp() in the
closure inside the loop over events_by_source, which collapses ULIDs with the
same millisecond; update the sort_by for source_events to compare the full Id
(e.g., id_a.cmp(id_b) or id_a.to_bytes()/to_string() comparison) so identical
timestamps maintain deterministic ULID ordering; keep the same Option handling
branches but replace timestamp comparisons with full-id comparisons in the
closure used by source_events.sort_by.
- Around line 207-223: The spawned tasks call TestContext::new() which acquires
separate DB slots so their inserts are invisible to the outer ctx.pool; instead
pass the outer pool into the tasks (or create a shared TestContext using the
outer ctx.pool) so all tasks use the same connection slot. Locate the
tokio::spawn block where ctx_clone = TestContext::new() is created and replace
construction with a clone or reference to ctx.pool (e.g., pass ctx.pool.clone()
or ctx.shared_context) so Event inserts use the same pool as the final
assertions (apply same change to the other occurrences referenced: the blocks
around the other spawn points at the noted ranges).
In `@crate/lib/sinex-schema/tests/ulid_property_from_main_tests.rs`:
- Line 33: The tests test_ulid_string_properties and test_ulid_bytes_roundtrip
currently use a dummy parameter (_dummy: u8 in 0u8..1) to satisfy the
property-test signature; instead convert them to standalone sinex tests by
removing the dummy parameter and changing the attribute to #[sinex_test] (or the
appropriate non-parameterized test attribute used in this crate), ensuring any
imports for the #[sinex_test] macro are present; update the function signatures
(no parameters) and leave their internal ULID generation and assertions
unchanged.
In `@crate/lib/sinex-test-utils/macros/src/lib.rs`:
- Around line 659-670: The generated test function currently always emits a
synchronous fn with #[sinex_prop], which breaks when a context parameter is
present; update the macro so that when ctx (the ctx_tokens mapping) is Some(...)
the generated function is declared async (emit async fn `#name`(...)) instead of
fn, keeping the same params, attributes (`#passthrough_attrs` and #[sinex_prop
`#meta_tokens`]), return type (sinex_test_utils::TestResult<()>), and body; in
short, branch on ctx_tokens presence and switch the emitted "fn" token to "async
fn" for the case generation (refer to ctx_tokens, `#name`, #[sinex_prop
`#meta_tokens`], and the surrounding quote! block).
- Around line 289-300: The pattern match is wrong: the fallback branch uses
.ok().and_then(|s| s.parse::<u64>().ok()) which returns an Option, so change the
conditional from if let Ok(seed_env) to if let Some(seed_env) to match the
Option; update the code around seed_stmt (the opts.seed map/unwrap_or_else block
that sets cfg.rng_algorithm and cfg.rng_seed and reads SINEX_PROPTEST_SEED) so
the environment parse path uses if let Some(seed_env) and assigns cfg.rng_seed =
Some(seed_env) accordingly.
- Around line 388-405: The current use of __sinex_prop_handle.block_on(...)
around an async block that uses tokio::time::timeout is unsafe on Tokio's test
runtime; replace the block_on path so the future is executed inside the test
runtime rather than driving it with block_on. Concretely, inside the runner.run
closure (where __sinex_prop_handle, Handle::block_on and tokio::time::timeout
are used) either call __sinex_prop_handle.enter() and execute the async block on
the entered runtime, or spawn the async work with __sinex_prop_handle.spawn(...)
and use tokio::time::timeout to await the JoinHandle; ensure the code no longer
calls Handle::block_on directly and still maps Ok(Ok(())), Ok(Err(err)) and
timeout Err to the same TestCaseError branches.
In `@crate/lib/sinex-test-utils/src/snapshot_helper.rs`:
- Around line 102-107: The current sanitization only replaces "::" with "_" and
can still produce invalid filenames; update the sanitization around the
sanitized variable (where test_name is used) to replace any characters not
allowed in filenames—e.g., use a regex to replace all characters except ASCII
letters, digits, hyphen, underscore and dot with "_" and collapse repeated
underscores, and optionally trim the result to a safe max length before building
filename with Utc::now().format(...) (update the `sanitized` assignment and
ensure `filename = format!(...)` uses the cleaned `sanitized`).
In `@docs/TEST_PATTERNS.md`:
- Around line 1070-1079: The sinex_proptest! block mixes inner and outer
attribute syntax; change the timeout attribute to use inner-block syntax so both
block attributes are parsed by sinex_proptest!'s parse_inner(): replace the
outer attribute #[timeout = "45s"] with an inner attribute #![timeout = "45s"]
in the sinex_proptest! block that contains fn property_ulids_roundtrip(...) (the
block already uses #![cases = 64], so make both attributes consistent).
In `@TESTING.md`:
- Line 93: Add a single blank line immediately after the Markdown heading that
precedes or contains the text "harness provides tracing, timeouts, context
wiring, and seed persistence." to satisfy MD022 (blanks-around-headings); edit
the TESTING.md heading block so there is one empty line between the heading and
the following paragraph/line.
- Around line 92-95: Update the doc string that currently claims failing seeds
are saved under tests/property/*.proptest-regressions to match the macro
implementation: failing seeds default to target/proptest-regressions (this
default is controlled by the SINEX_PROPTEST_DIR environment variable); mention
both the default path and how to override it via SINEX_PROPTEST_DIR in the
documentation for #[sinex_prop] / sinex_proptest! so readers see the actual
behavior implemented in macros::lib (the code that reads SINEX_PROPTEST_DIR).
---
Outside diff comments:
In @.cargo-machete.toml:
- Around line 19-23: The TOML has duplicate keys in the [ignored] table —
specifically the keys "sinex-terminal-satellite" and "sinex-system-satellite"
are declared twice; merge each duplicate into a single key and combine their
dependency arrays into one array per key (e.g., for sinex-terminal-satellite
include both "tracing-subscriber" and "xterm-query" in a single array, and for
sinex-system-satellite include "battery", "procfs", and "systemstat" in a single
array) so the TOML parser can read the file.
In `@crate/lib/sinex-core/tests/property/event_model_fuzzing_test.rs`:
- Around line 529-576: Both test_system_events_robustness and
test_json_serialization_robustness are defined using proptest-style parameters
but sit outside any sinex_proptest! macro, so wrap these two functions inside a
sinex_proptest! { ... } block to register them properly; specifically, move the
definitions of test_system_events_robustness and
test_json_serialization_robustness into a sinex_proptest! macro invocation (or
reopen the existing sinex_proptest! block) so their `payload in ...` signatures
and TestResult return types are compiled and executed by the proptest harness.
In `@crate/lib/sinex-satellite-sdk/tests/property/satellite_property_test.rs`:
- Around line 254-278: The test body inside the sinex_proptest! macro (function
satellite_config_validation_is_robust) currently ends with assertions that
return () causing a type mismatch with the macro's expected
sinex_test_utils::TestResult<()>; fix it by appending Ok(()) as the final
expression of the function so the return type matches; locate the test defined
by sinex_proptest! and add Ok(()) after the last assert_eq! (the code using
SatelliteConfig::builder(), config.validate_config(), and
SatelliteConfig::load_from_env()).
In `@crate/lib/sinex-schema/tests/ulid_tests.rs`:
- Around line 347-393: The property tests inside the sinex_proptest! block
(functions test_ulid_string_roundtrip, test_ulid_bytes_roundtrip,
test_ulid_uuid_roundtrip, test_ulid_ordering_consistency,
test_timestamp_extraction_reasonable) are missing the final Ok(()) return
expected by the generated -> sinex_test_utils::TestResult<()> signature; update
each test body to return Ok(()) as the last expression so successful assertions
produce Ok(()), ensuring compatibility with the macro-generated return type.
In `@crate/lib/sinex-test-utils/src/static_fixtures.rs`:
- Around line 419-420: The file contains a duplicate import of the same symbol;
remove the redundant line importing crate::sinex_test so there is only a single
`use crate::sinex_test;` statement (locate the duplicate import in
static_fixtures.rs and delete the extra occurrence).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 420c3f55-8d64-408e-9ed8-8cce9ce9f927
📒 Files selected for processing (39)
.cargo-machete.tomlTESTING.mdcrate/lib/sinex-core/src/types/bin/sinex-schema.rscrate/lib/sinex-core/tests/adversarial/enhanced_boundary_test.rscrate/lib/sinex-core/tests/event_property.rscrate/lib/sinex-core/tests/property/event_model_fuzzing_test.rscrate/lib/sinex-core/tests/property/event_validation_property_test.rscrate/lib/sinex-core/tests/property/path_sanitization_property_test.rscrate/lib/sinex-core/tests/property/schema_property_test.rscrate/lib/sinex-core/tests/property/time_range_property_test.rscrate/lib/sinex-core/tests/property/ulid_property_test.rscrate/lib/sinex-core/tests/property/validation_roundtrip_property_test.rscrate/lib/sinex-core/tests/property_tests.rscrate/lib/sinex-core/tests/unit/preflight_test.rscrate/lib/sinex-satellite-sdk/tests/property/automation_property_test.rscrate/lib/sinex-satellite-sdk/tests/property/checkpoint_property_test.rscrate/lib/sinex-satellite-sdk/tests/property/error_handling_property_test.rscrate/lib/sinex-satellite-sdk/tests/property/queue_property_test.rscrate/lib/sinex-satellite-sdk/tests/property/satellite_property_test.rscrate/lib/sinex-satellite-sdk/tests/property/validation_invariants_property_test.rscrate/lib/sinex-satellite-sdk/tests/property_checkpoint_state.rscrate/lib/sinex-satellite-sdk/tests/security/path_validation_test.rscrate/lib/sinex-schema/docs/architecture.mdcrate/lib/sinex-schema/tests/ulid_property_from_main_tests.rscrate/lib/sinex-schema/tests/ulid_property_tests.rscrate/lib/sinex-schema/tests/ulid_tests.rscrate/lib/sinex-test-utils/README.mdcrate/lib/sinex-test-utils/macros/src/lib.rscrate/lib/sinex-test-utils/src/database_pool.rscrate/lib/sinex-test-utils/src/fixture_generator.rscrate/lib/sinex-test-utils/src/lib.rscrate/lib/sinex-test-utils/src/property_testing.rscrate/lib/sinex-test-utils/src/snapshot_helper.rscrate/lib/sinex-test-utils/src/static_fixtures.rscrate/lib/sinex-test-utils/src/test_context.rscrate/satellites/sinex-system-satellite/src/unified_processor.rsdocs/TEST_PATTERNS.mddocs/TEST_PATTERNS_GUIDE.mdscripts/dev-env-banner.sh
💤 Files with no reviewable changes (2)
- crate/lib/sinex-core/src/types/bin/sinex-schema.rs
- scripts/dev-env-banner.sh
Comment on lines
+343
to
374
| #[sinex_prop] | ||
| async fn test_property_based_boundaries( | ||
| ctx: &TestContext, | ||
| #[strategy(0..1000usize)] array_size: usize, | ||
| #[strategy(0..10000usize)] string_len: usize, | ||
| #[strategy(0..50usize)] nest_depth: usize, | ||
| ) -> Result<()> { | ||
| let array: Vec<i32> = (0..array_size as i32).collect(); | ||
| let text = "x".repeat(string_len); | ||
|
|
||
| let mut nested = serde_json::json!("leaf"); | ||
| for _ in 0..nest_depth { | ||
| nested = serde_json::json!({ "child": nested }); | ||
| } | ||
|
|
||
| let event = ctx.create_test_event( | ||
| "property_test", | ||
| "boundary", | ||
| serde_json::json!({ | ||
| "array": array, | ||
| "text": text, | ||
| "nested": nested, | ||
| "array_size": array_size, | ||
| "string_len": string_len, | ||
| "nest_depth": nest_depth | ||
| }), | ||
| ); | ||
|
|
||
| // We don't assert success, just that it doesn't panic | ||
| let _ = ctx.pool.events().insert(event).await; | ||
| }); | ||
| }); | ||
| let event = ctx.create_test_event( | ||
| "property_test", | ||
| "boundary", | ||
| serde_json::json!({ | ||
| "array": array, | ||
| "text": text, | ||
| "nested": nested, | ||
| "array_size": array_size, | ||
| "string_len": string_len, | ||
| "nest_depth": nest_depth | ||
| }), | ||
| ); | ||
|
|
||
| // We don't assert success, just that it doesn't panic | ||
| let _ = ctx.pool.events().insert(event).await; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using TestResult<()> for consistency.
The return type Result<()> resolves to std::result::Result<(), SinexError> per sinex_test_utils. Other migrated tests use TestResult<()> (color_eyre::eyre::Result<()>), which provides richer error context. Consider aligning for consistency.
♻️ Suggested change
#[sinex_prop]
async fn test_property_based_boundaries(
ctx: &TestContext,
#[strategy(0..1000usize)] array_size: usize,
#[strategy(0..10000usize)] string_len: usize,
#[strategy(0..50usize)] nest_depth: usize,
-) -> Result<()> {
+) -> TestResult<()> {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[sinex_prop] | |
| async fn test_property_based_boundaries( | |
| ctx: &TestContext, | |
| #[strategy(0..1000usize)] array_size: usize, | |
| #[strategy(0..10000usize)] string_len: usize, | |
| #[strategy(0..50usize)] nest_depth: usize, | |
| ) -> Result<()> { | |
| let array: Vec<i32> = (0..array_size as i32).collect(); | |
| let text = "x".repeat(string_len); | |
| let mut nested = serde_json::json!("leaf"); | |
| for _ in 0..nest_depth { | |
| nested = serde_json::json!({ "child": nested }); | |
| } | |
| let event = ctx.create_test_event( | |
| "property_test", | |
| "boundary", | |
| serde_json::json!({ | |
| "array": array, | |
| "text": text, | |
| "nested": nested, | |
| "array_size": array_size, | |
| "string_len": string_len, | |
| "nest_depth": nest_depth | |
| }), | |
| ); | |
| // We don't assert success, just that it doesn't panic | |
| let _ = ctx.pool.events().insert(event).await; | |
| }); | |
| }); | |
| let event = ctx.create_test_event( | |
| "property_test", | |
| "boundary", | |
| serde_json::json!({ | |
| "array": array, | |
| "text": text, | |
| "nested": nested, | |
| "array_size": array_size, | |
| "string_len": string_len, | |
| "nest_depth": nest_depth | |
| }), | |
| ); | |
| // We don't assert success, just that it doesn't panic | |
| let _ = ctx.pool.events().insert(event).await; | |
| Ok(()) | |
| } | |
| #[sinex_prop] | |
| async fn test_property_based_boundaries( | |
| ctx: &TestContext, | |
| #[strategy(0..1000usize)] array_size: usize, | |
| #[strategy(0..10000usize)] string_len: usize, | |
| #[strategy(0..50usize)] nest_depth: usize, | |
| ) -> TestResult<()> { | |
| let array: Vec<i32> = (0..array_size as i32).collect(); | |
| let text = "x".repeat(string_len); | |
| let mut nested = serde_json::json!("leaf"); | |
| for _ in 0..nest_depth { | |
| nested = serde_json::json!({ "child": nested }); | |
| } | |
| let event = ctx.create_test_event( | |
| "property_test", | |
| "boundary", | |
| serde_json::json!({ | |
| "array": array, | |
| "text": text, | |
| "nested": nested, | |
| "array_size": array_size, | |
| "string_len": string_len, | |
| "nest_depth": nest_depth | |
| }), | |
| ); | |
| // We don't assert success, just that it doesn't panic | |
| let _ = ctx.pool.events().insert(event).await; | |
| Ok(()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crate/lib/sinex-core/tests/adversarial/enhanced_boundary_test.rs` around
lines 343 - 374, Change the test function signature for
test_property_based_boundaries to return TestResult<()> instead of Result<()>
for consistency with other migrated tests; update the signature from async fn
test_property_based_boundaries(...) -> Result<()> to -> TestResult<()>, ensure
TestResult is in scope (import or fully-qualify, e.g., color_eyre::eyre::Result
as TestResult or whatever project alias is used), and keep the existing Ok(())
return so the body requires no further changes.
Comment on lines
+335
to
+350
| fn test_validation_preserves_error_hierarchy( | ||
| event in arbitrary_event() | ||
| ) -> color_eyre::eyre::Result<()> { | ||
| let result = validate_event(&event); | ||
|
|
||
| // Verify size is roughly what we expect (with JSON overhead) | ||
| if let Ok(json_str) = serialized { | ||
| prop_assert!(json_str.len() > size_kb * 1024, "Serialized size should exceed payload size"); | ||
| if let Err(error) = result { | ||
| let error_string = error.to_string(); | ||
| prop_assert!(!error_string.is_empty(), "Error message should not be empty"); | ||
| if event.source.is_empty() { | ||
| prop_assert!(error_string.contains("source")); | ||
| } | ||
| if event.event_type.is_empty() { | ||
| prop_assert!(error_string.contains("event_type") || error_string.contains("type")); | ||
| } | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
These error-hierarchy properties are currently vacuous.
arbitrary_event() always builds valid events, so validate_event(&event) does not hit the Err branch here. The assertions on Line 340 and Line 612 therefore never run.
Also applies to: 606-636
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crate/lib/sinex-core/tests/property/event_validation_property_test.rs` around
lines 335 - 350, The property test test_validation_preserves_error_hierarchy is
vacuous because arbitrary_event() always produces valid events so
validate_event(&event) never returns Err; change the test to either use a
generator that can produce invalid events (e.g. arbitrary_invalid_event or a
combined strategy) or mutate the generated event before validation (e.g. set
event.source = "" and/or event.event_type = "") so that validate_event(&event)
can fail and the Err branch runs; update the assertions accordingly (and make
the same change for the analogous test block at lines 606-636) referencing
validate_event, arbitrary_event, and test_validation_preserves_error_hierarchy
to find and fix the code.
Comment on lines
+354
to
488
| sinex_proptest! { | ||
| fn property_payload_size_validation( | ||
| size_kb in 1usize..=1000usize // Reduced size for faster tests | ||
| ) -> color_eyre::eyre::Result<()> { | ||
| // Property: Events should handle various payload sizes gracefully | ||
| let large_data = "x".repeat(size_kb * 1024); | ||
| let payload = json!({ | ||
| "data": large_data, | ||
| "size_kb": size_kb | ||
| }); | ||
|
|
||
| // If ts_orig exists, it should be reasonable | ||
| if let Some(ts_orig) = event.ts_orig { | ||
| let now = chrono::Utc::now(); | ||
| let diff = (now - ts_orig).num_days().abs(); | ||
| prop_assert!(diff < 365 * 100, "Timestamp should be within 100 years of now"); | ||
| } | ||
| let mut event = RawEvent::test_event( | ||
| EventSource::new("test"), | ||
| EventType::new("payload.size.test"), | ||
| payload, | ||
| ); | ||
| event.id = Some(Id::from_ulid(Ulid::new())); | ||
|
|
||
| // Check that large payloads are handled | ||
| let serialized = serde_json::to_string(&event); | ||
| prop_assert!(serialized.is_ok(), "Should serialize large payload"); | ||
|
|
||
| // Verify size is roughly what we expect (with JSON overhead) | ||
| if let Ok(json_str) = serialized { | ||
| prop_assert!( | ||
| json_str.len() > size_kb * 1024, | ||
| "Serialized size should exceed payload size" | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[sinex_test] | ||
| fn test_event_uniqueness_properties() -> color_eyre::eyre::Result<()> { | ||
| proptest::proptest! { | ||
| fn property_event_uniqueness( | ||
| events in proptest::collection::vec(arbitrary_event(), 2..100) | ||
| ) { | ||
| // Property: Events should have unique IDs and maintain ordering | ||
| let mut ids: Vec<Ulid> = events.iter().filter_map(|e| e.id.clone().map(|id| id.into())).collect(); | ||
|
|
||
| // Check uniqueness (though IDs are generated and should be unique) | ||
| let unique_ids: std::collections::HashSet<_> = ids.iter().cloned().collect(); | ||
| prop_assert_eq!(ids.len(), unique_ids.len(), "All event IDs should be unique"); | ||
|
|
||
| // Check that sorting by ID gives consistent order | ||
| ids.sort(); | ||
| for window in ids.windows(2) { | ||
| prop_assert!(window[0] <= window[1], "IDs should maintain sort order"); | ||
| } | ||
| fn property_event_timestamp_consistency( | ||
| event in arbitrary_event() | ||
| ) -> color_eyre::eyre::Result<()> { | ||
| // Property: ULID timestamp (ingest) should be >= ts_orig when both exist | ||
| if let Some(id) = event.id { | ||
| let ingest_ts = id.timestamp(); | ||
| let ts_orig = event.ts_orig.unwrap_or(ingest_ts); | ||
| prop_assert!( | ||
| ingest_ts >= ts_orig, | ||
| "ULID timestamp should not be before origin time" | ||
| ); | ||
| } | ||
|
|
||
| // If ts_orig exists, it should be reasonable | ||
| if let Some(ts_orig) = event.ts_orig { | ||
| let now = chrono::Utc::now(); | ||
| let diff = (now - ts_orig).num_days().abs(); | ||
| prop_assert!(diff < 365 * 100, "Timestamp should be within 100 years of now"); | ||
| } | ||
| Ok(()) | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[sinex_test] | ||
| fn test_source_event_id_validation() -> color_eyre::eyre::Result<()> { | ||
| proptest::proptest! { | ||
| fn property_source_event_id_validation( | ||
| parent_events in proptest::collection::vec(Just(()).prop_map(|_| Ulid::new()), 0..10), | ||
| _event in arbitrary_event() | ||
| ) { | ||
| // Property: Source event IDs should be valid ULIDs | ||
| // Note: source_event_ids field is not available in the current Event type | ||
| // This property test validates the ULID values themselves | ||
| for id in parent_events { | ||
| // ULIDs don't have version numbers like UUIDs, but should not be nil | ||
| prop_assert!(!id.is_nil(), "ULID should not be nil"); | ||
| } | ||
| fn property_event_uniqueness( | ||
| events in proptest::collection::vec(arbitrary_event(), 2..100) | ||
| ) -> color_eyre::eyre::Result<()> { | ||
| // Property: Events should have unique IDs and maintain ordering | ||
| let mut ids: Vec<Ulid> = events | ||
| .iter() | ||
| .filter_map(|e| e.id.clone().map(|id| id.into())) | ||
| .collect(); | ||
|
|
||
| // Check uniqueness (though IDs are generated and should be unique) | ||
| let unique_ids: std::collections::HashSet<_> = ids.iter().cloned().collect(); | ||
| prop_assert_eq!(ids.len(), unique_ids.len(), "All event IDs should be unique"); | ||
|
|
||
| // Check that sorting by ID gives consistent order | ||
| ids.sort(); | ||
| for window in ids.windows(2) { | ||
| prop_assert!(window[0] <= window[1], "IDs should maintain sort order"); | ||
| } | ||
| Ok(()) | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[sinex_test] | ||
| fn test_json_schema_compatibility() -> color_eyre::eyre::Result<()> { | ||
| proptest::proptest! { | ||
| fn property_json_schema_compatibility( | ||
| event in arbitrary_event() | ||
| ) { | ||
| // Property: Event payloads should be valid JSON that can be schema-validated | ||
| let payload_str = serde_json::to_string(&event.payload); | ||
| prop_assert!(payload_str.is_ok(), "Payload should serialize to JSON"); | ||
|
|
||
| if let Ok(json_str) = payload_str { | ||
| let parsed: std::result::Result<Value, _> = serde_json::from_str(&json_str); | ||
| prop_assert!(parsed.is_ok(), "Payload should round-trip through JSON"); | ||
| } | ||
| fn property_source_event_id_validation( | ||
| parent_events in proptest::collection::vec(Just(()).prop_map(|_| Ulid::new()), 0..10), | ||
| _event in arbitrary_event() | ||
| ) -> color_eyre::eyre::Result<()> { | ||
| // Property: Source event IDs should be valid ULIDs | ||
| // Note: source_event_ids field is not available in the current Event type | ||
| // This property test validates the ULID values themselves | ||
| for id in parent_events { | ||
| // ULIDs don't have version numbers like UUIDs, but should not be nil | ||
| prop_assert!(!id.is_nil(), "ULID should not be nil"); | ||
| } | ||
| Ok(()) | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[sinex_test] | ||
| fn test_event_metadata_fields() -> color_eyre::eyre::Result<()> { | ||
| proptest::proptest! { | ||
| fn property_event_metadata_fields( | ||
| event in metadata_rich_events() | ||
| ) { | ||
| // Property: Metadata-rich events should have expected optional fields | ||
| // Note: source_material fields are not available in Event, focusing on payload metadata | ||
|
|
||
| // Check payload has metadata if it's an object | ||
| if let Value::Object(ref map) = event.payload { | ||
| if let Some(metadata) = map.get("_metadata") { | ||
| prop_assert!(metadata.is_object(), "Metadata should be an object"); | ||
| } | ||
| } | ||
| fn property_json_schema_compatibility( | ||
| event in arbitrary_event() | ||
| ) -> color_eyre::eyre::Result<()> { | ||
| // Property: Event payloads should be valid JSON that can be schema-validated | ||
| let payload_str = serde_json::to_string(&event.payload); | ||
| prop_assert!(payload_str.is_ok(), "Payload should serialize to JSON"); | ||
|
|
||
| if let Ok(json_str) = payload_str { | ||
| let parsed: std::result::Result<Value, _> = serde_json::from_str(&json_str); | ||
| prop_assert!(parsed.is_ok(), "Payload should round-trip through JSON"); | ||
| } | ||
| Ok(()) | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[sinex_test] | ||
| fn test_boundary_condition_handling() -> color_eyre::eyre::Result<()> { | ||
| proptest::proptest! { | ||
| fn property_boundary_condition_handling( | ||
| event in boundary_condition_events() | ||
| ) { | ||
| // Property: Boundary condition events should be processable | ||
| // Even boundary cases should have valid structure | ||
| prop_assert!(!event.source.is_empty(), "Source should not be empty"); | ||
| prop_assert!(!event.event_type.is_empty(), "Event type should not be empty"); | ||
| if let Some(id) = event.id { | ||
| prop_assert_ne!(Into::<Ulid>::into(id), Ulid::nil(), "ID should not be nil"); | ||
| fn property_event_metadata_fields( | ||
| event in metadata_rich_events() | ||
| ) -> color_eyre::eyre::Result<()> { | ||
| // Property: Metadata-rich events should have expected optional fields | ||
| // Note: source_material fields are not available in Event, focusing on payload metadata | ||
|
|
||
| // Check payload has metadata if it's an object | ||
| if let Value::Object(ref map) = event.payload { | ||
| if let Some(metadata) = map.get("_metadata") { | ||
| prop_assert!(metadata.is_object(), "Metadata should be an object"); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| // Payload should be valid JSON | ||
| let _ = serde_json::to_string(&event.payload) | ||
| .expect("Boundary payload should be serializable"); | ||
| fn property_boundary_condition_handling( | ||
| event in boundary_condition_events() | ||
| ) -> color_eyre::eyre::Result<()> { | ||
| // Property: Boundary condition events should be processable | ||
| // Even boundary cases should have valid structure | ||
| prop_assert!(!event.source.is_empty(), "Source should not be empty"); | ||
| prop_assert!(!event.event_type.is_empty(), "Event type should not be empty"); | ||
| if let Some(id) = event.id { | ||
| prop_assert_ne!(Into::<Ulid>::into(id), Ulid::nil(), "ID should not be nil"); | ||
| } | ||
|
|
||
| // Payload should be valid JSON | ||
| let _ = serde_json::to_string(&event.payload) | ||
| .expect("Boundary payload should be serializable"); | ||
| Ok(()) | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Collapse the duplicated property block.
Lines 354-488 reintroduce the same payload/timestamp/uniqueness/metadata/boundary properties already defined above. Keeping both doubles runtime and makes the two copies easy to drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crate/lib/sinex-core/tests/property/event_validation_property_test.rs` around
lines 354 - 488, The test file contains a duplicated sinex_proptest! block
re-defining property_payload_size_validation,
property_event_timestamp_consistency, property_event_uniqueness,
property_source_event_id_validation, property_json_schema_compatibility,
property_event_metadata_fields, and property_boundary_condition_handling; remove
the redundant block so each property_* function is defined only once (or
consolidate any intentional differences into the original definitions), leaving
a single sinex_proptest! invocation that contains each of those property tests
exactly once and ensuring helper generators like arbitrary_event() and
metadata_rich_events() are still referenced.
Comment on lines
+501
to
531
| sinex_proptest! { | ||
| fn property_concurrent_event_ordering( | ||
| events in concurrent_operation_events() | ||
| ) -> color_eyre::eyre::Result<()> { | ||
| // Property: Concurrent events should maintain per-worker ordering | ||
| let mut by_worker: std::collections::HashMap<usize, Vec<_>> = | ||
| std::collections::HashMap::new(); | ||
|
|
||
| for event in events { | ||
| if let Value::Object(ref map) = event.payload { | ||
| if let (Some(Value::Number(worker_id)), Some(Value::Number(op_id))) = | ||
| (map.get("worker_id"), map.get("operation_id")) | ||
| { | ||
| let worker = worker_id.as_u64().unwrap() as usize; | ||
| let op = op_id.as_u64().unwrap(); | ||
| by_worker.entry(worker).or_default().push(op); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Each worker's operations should be in order | ||
| for (_, ops) in by_worker { | ||
| for window in ops.windows(2) { | ||
| prop_assert!(window[0] <= window[1], | ||
| "Worker operations should be ordered"); | ||
| } | ||
| // Each worker's operations should be in order | ||
| for (_, ops) in by_worker { | ||
| for window in ops.windows(2) { | ||
| prop_assert!( | ||
| window[0] <= window[1], | ||
| "Worker operations should be ordered" | ||
| ); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
This asserts generator order, not per-worker ordering.
concurrent_operation_events() generates arbitrary (worker_id, operation_id) pairs, so each worker’s ops vector is not guaranteed sorted before the check on Lines 523-527. This can fail without any product bug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crate/lib/sinex-core/tests/property/event_validation_property_test.rs` around
lines 501 - 531, The test currently asserts per-worker ordering on the raw
collected ops but the generator emits (worker_id, operation_id) pairs in
arbitrary order; to fix, sort each worker's ops vector before asserting. In the
property_concurrent_event_ordering test (using concurrent_operation_events and
the by_worker map where ops holds op_id as u64), call a sort (e.g.,
sort_unstable()) on each ops Vec before iterating windows and performing
prop_assert on window[0] <= window[1], so the check validates ordering rather
than generator order.
Comment on lines
+95
to
+129
| #[strategy(proptest::collection::vec((0u64..1000u64, checkpoint_data()), 1..=10))] | ||
| checkpoints: Vec<(u64, serde_json::Value)>, | ||
| ) -> TestResult<()> { | ||
| let ctx = TestContext::new().await?; | ||
| let pool = ctx.pool.clone(); | ||
|
|
||
| let checkpoint_manager = CheckpointManager::new( | ||
| pool.clone(), | ||
| processor_name.clone(), | ||
| format!("{processor_name}-group"), | ||
| format!("{processor_name}-consumer"), | ||
| ); | ||
|
|
||
| // Save multiple checkpoints with increasing counts | ||
| let mut expected_final_count = 0u64; | ||
| for (i, (processed_count, data)) in checkpoints.iter().enumerate() { | ||
| expected_final_count = *processed_count; | ||
|
|
||
| let state = CheckpointState { | ||
| checkpoint: Checkpoint::Stream { | ||
| message_id: format!("message-{i}"), | ||
| event_id: None, | ||
| }, | ||
| processed_count: *processed_count, | ||
| last_activity: chrono::Utc::now(), | ||
| data: Some(data.clone()), | ||
| version: 2, | ||
| }; | ||
|
|
||
| checkpoint_manager.save_checkpoint(&state).await?; | ||
| } | ||
| } | ||
|
|
||
| // Test concurrent checkpoint access | ||
| proptest! { | ||
| fn concurrent_checkpoint_access_is_safe( | ||
| processor_name in processor_names(), | ||
| concurrent_updates in proptest::collection::vec(0u64..1000u64, 1..=20), | ||
| ) { | ||
| run_async(async { | ||
| let ctx = TestContext::new().await.unwrap(); | ||
| let pool = ctx.pool.clone(); | ||
|
|
||
| let checkpoint_manager = Arc::new(CheckpointManager::new( | ||
| pool.clone(), | ||
| processor_name.clone(), | ||
| format!("{processor_name}-group"), | ||
| format!("{processor_name}-consumer"), | ||
| )); | ||
|
|
||
| // Launch concurrent update tasks | ||
| let mut handles = Vec::new(); | ||
| for (i, processed_count) in concurrent_updates.iter().enumerate() { | ||
| let manager = checkpoint_manager.clone(); | ||
| let count = *processed_count; | ||
| let handle = tokio::spawn(async move { | ||
| let state = CheckpointState { | ||
| checkpoint: Checkpoint::Stream { | ||
| message_id: format!("concurrent-{i}"), | ||
| event_id: None, | ||
| }, | ||
| processed_count: count, | ||
| last_activity: chrono::Utc::now(), | ||
| data: Some(serde_json::json!({"task": i})), | ||
| version: 2, | ||
| }; | ||
|
|
||
| manager.save_checkpoint(&state).await | ||
| }); | ||
| handles.push(handle); | ||
| } | ||
|
|
||
| // Wait for all updates to complete | ||
| let mut results = Vec::new(); | ||
| for handle in handles { | ||
| results.push(handle.await.unwrap()); | ||
| } | ||
|
|
||
| // Verify all updates succeeded (or failed gracefully) | ||
| let successful_updates = results.iter().filter(|r| r.is_ok()).count(); | ||
| prop_assert!(successful_updates > 0, "At least one update should succeed"); | ||
|
|
||
| // Verify final state is consistent | ||
| let final_state = checkpoint_manager.get_checkpoint_stats().await.unwrap(); | ||
| prop_assert!(final_state.last_update.is_some()); | ||
|
|
||
| Ok::<(), proptest::test_runner::TestCaseError>(()) | ||
| })?; | ||
| } | ||
| // Verify final state | ||
| let state = checkpoint_manager.get_checkpoint_stats().await?; | ||
| prop_assert_eq!(state.max_processed, expected_final_count); |
There was a problem hiding this comment.
Track the maximum processed count here, not the last sample.
Line 108 says these checkpoints are “increasing”, but the strategy on Lines 95-96 generates arbitrary counts. If a later case is smaller, Line 129 compares max_processed against the last write and fails even when recovery is correct.
Suggested fix
- let mut expected_final_count = 0u64;
+ let mut expected_final_count = 0u64;
for (i, (processed_count, data)) in checkpoints.iter().enumerate() {
- expected_final_count = *processed_count;
+ expected_final_count = expected_final_count.max(*processed_count);
let state = CheckpointState {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[strategy(proptest::collection::vec((0u64..1000u64, checkpoint_data()), 1..=10))] | |
| checkpoints: Vec<(u64, serde_json::Value)>, | |
| ) -> TestResult<()> { | |
| let ctx = TestContext::new().await?; | |
| let pool = ctx.pool.clone(); | |
| let checkpoint_manager = CheckpointManager::new( | |
| pool.clone(), | |
| processor_name.clone(), | |
| format!("{processor_name}-group"), | |
| format!("{processor_name}-consumer"), | |
| ); | |
| // Save multiple checkpoints with increasing counts | |
| let mut expected_final_count = 0u64; | |
| for (i, (processed_count, data)) in checkpoints.iter().enumerate() { | |
| expected_final_count = *processed_count; | |
| let state = CheckpointState { | |
| checkpoint: Checkpoint::Stream { | |
| message_id: format!("message-{i}"), | |
| event_id: None, | |
| }, | |
| processed_count: *processed_count, | |
| last_activity: chrono::Utc::now(), | |
| data: Some(data.clone()), | |
| version: 2, | |
| }; | |
| checkpoint_manager.save_checkpoint(&state).await?; | |
| } | |
| } | |
| // Test concurrent checkpoint access | |
| proptest! { | |
| fn concurrent_checkpoint_access_is_safe( | |
| processor_name in processor_names(), | |
| concurrent_updates in proptest::collection::vec(0u64..1000u64, 1..=20), | |
| ) { | |
| run_async(async { | |
| let ctx = TestContext::new().await.unwrap(); | |
| let pool = ctx.pool.clone(); | |
| let checkpoint_manager = Arc::new(CheckpointManager::new( | |
| pool.clone(), | |
| processor_name.clone(), | |
| format!("{processor_name}-group"), | |
| format!("{processor_name}-consumer"), | |
| )); | |
| // Launch concurrent update tasks | |
| let mut handles = Vec::new(); | |
| for (i, processed_count) in concurrent_updates.iter().enumerate() { | |
| let manager = checkpoint_manager.clone(); | |
| let count = *processed_count; | |
| let handle = tokio::spawn(async move { | |
| let state = CheckpointState { | |
| checkpoint: Checkpoint::Stream { | |
| message_id: format!("concurrent-{i}"), | |
| event_id: None, | |
| }, | |
| processed_count: count, | |
| last_activity: chrono::Utc::now(), | |
| data: Some(serde_json::json!({"task": i})), | |
| version: 2, | |
| }; | |
| manager.save_checkpoint(&state).await | |
| }); | |
| handles.push(handle); | |
| } | |
| // Wait for all updates to complete | |
| let mut results = Vec::new(); | |
| for handle in handles { | |
| results.push(handle.await.unwrap()); | |
| } | |
| // Verify all updates succeeded (or failed gracefully) | |
| let successful_updates = results.iter().filter(|r| r.is_ok()).count(); | |
| prop_assert!(successful_updates > 0, "At least one update should succeed"); | |
| // Verify final state is consistent | |
| let final_state = checkpoint_manager.get_checkpoint_stats().await.unwrap(); | |
| prop_assert!(final_state.last_update.is_some()); | |
| Ok::<(), proptest::test_runner::TestCaseError>(()) | |
| })?; | |
| } | |
| // Verify final state | |
| let state = checkpoint_manager.get_checkpoint_stats().await?; | |
| prop_assert_eq!(state.max_processed, expected_final_count); | |
| #[strategy(proptest::collection::vec((0u64..1000u64, checkpoint_data()), 1..=10))] | |
| checkpoints: Vec<(u64, serde_json::Value)>, | |
| ) -> TestResult<()> { | |
| let ctx = TestContext::new().await?; | |
| let pool = ctx.pool.clone(); | |
| let checkpoint_manager = CheckpointManager::new( | |
| pool.clone(), | |
| processor_name.clone(), | |
| format!("{processor_name}-group"), | |
| format!("{processor_name}-consumer"), | |
| ); | |
| // Save multiple checkpoints with increasing counts | |
| let mut expected_final_count = 0u64; | |
| for (i, (processed_count, data)) in checkpoints.iter().enumerate() { | |
| expected_final_count = expected_final_count.max(*processed_count); | |
| let state = CheckpointState { | |
| checkpoint: Checkpoint::Stream { | |
| message_id: format!("message-{i}"), | |
| event_id: None, | |
| }, | |
| processed_count: *processed_count, | |
| last_activity: chrono::Utc::now(), | |
| data: Some(data.clone()), | |
| version: 2, | |
| }; | |
| checkpoint_manager.save_checkpoint(&state).await?; | |
| } | |
| // Verify final state | |
| let state = checkpoint_manager.get_checkpoint_stats().await?; | |
| prop_assert_eq!(state.max_processed, expected_final_count); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crate/lib/sinex-satellite-sdk/tests/property/checkpoint_property_test.rs`
around lines 95 - 129, The test incorrectly sets expected_final_count to the
last checkpoint's processed_count instead of the maximum seen, causing failures
when later checkpoints decrease; in the loop that iterates over checkpoints
(using checkpoints, processed_count, CheckpointState), update
expected_final_count = expected_final_count.max(*processed_count) (or otherwise
compute the max across all processed_count values) before saving with
checkpoint_manager.save_checkpoint(), then assert
prop_assert_eq!(state.max_processed, expected_final_count) against
checkpoint_manager.get_checkpoint_stats().
Comment on lines
+659
to
+670
| let ctx_tokens = ctx | ||
| .as_ref() | ||
| .map(|(pat, ty)| quote!( #pat: #ty, )) | ||
| .unwrap_or_else(|| quote!()); | ||
|
|
||
| out.push(quote! { | ||
| #(#passthrough_attrs)* | ||
| #[sinex_prop #meta_tokens] | ||
| fn #name( #ctx_tokens #( #param_defs ),* ) -> sinex_test_utils::TestResult<()> { | ||
| #( #destructures )* | ||
| #body | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the specific file and lines mentioned
cat -n crate/lib/sinex-test-utils/macros/src/lib.rs | sed -n '650,680p'Repository: Sinity/sinex
Length of output: 1465
🏁 Script executed:
# Let's understand the overall structure - find where sinex_prop is defined
fd -t f "\.rs$" | xargs grep -l "sinex_prop" | head -20Repository: Sinity/sinex
Length of output: 1340
🏁 Script executed:
# Search for sinex_prop attribute or macro definition
rg "sinex_prop" -A 5 -B 2 | head -100Repository: Sinity/sinex
Length of output: 6220
🏁 Script executed:
# Find the sinex_prop attribute implementation
fd -t f "\.rs$" crate/lib/sinex-test-utils | xargs grep -l "sinex_prop" | grep -E "(lib|src)" | head -10Repository: Sinity/sinex
Length of output: 189
🏁 Script executed:
# Look for the actual sinex_prop macro/attribute definition
rg "pub fn sinex_prop|pub fn sinex_test|#\[proc_macro" crate/lib/sinex-test-utils -A 10 | head -150Repository: Sinity/sinex
Length of output: 3875
🏁 Script executed:
# Check what TestContext requires - is it async-specific?
rg "TestContext" crate/lib/sinex-test-utils/src -A 2 -B 2 | head -80Repository: Sinity/sinex
Length of output: 5753
🏁 Script executed:
# Get the full sinex_prop implementation
cat -n crate/lib/sinex-test-utils/macros/src/lib.rs | sed -n '260,450p'Repository: Sinity/sinex
Length of output: 8260
🏁 Script executed:
# Look for any validation logic that checks for TestContext or async status
rg "TestContext" crate/lib/sinex-test-utils/macros/src -B 3 -A 3Repository: Sinity/sinex
Length of output: 10901
🏁 Script executed:
# Check if there's any error or rejection of TestContext in non-async contexts
rg "async|TestContext" crate/lib/sinex-test-utils/macros/src/lib.rs | grep -i "error\|reject\|fail\|invalid\|unsupported" -B 2 -A 2Repository: Sinity/sinex
Length of output: 874
sinex_proptest! cannot currently emit context-aware cases.
When a case declares a context parameter, sinex_proptest! generates a synchronous fn but applies the #[sinex_prop] attribute, which explicitly rejects TestContext on non-async functions (lines 272-279). Any such DSL case will fail to compile.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crate/lib/sinex-test-utils/macros/src/lib.rs` around lines 659 - 670, The
generated test function currently always emits a synchronous fn with
#[sinex_prop], which breaks when a context parameter is present; update the
macro so that when ctx (the ctx_tokens mapping) is Some(...) the generated
function is declared async (emit async fn `#name`(...)) instead of fn, keeping the
same params, attributes (`#passthrough_attrs` and #[sinex_prop `#meta_tokens`]),
return type (sinex_test_utils::TestResult<()>), and body; in short, branch on
ctx_tokens presence and switch the emitted "fn" token to "async fn" for the case
generation (refer to ctx_tokens, `#name`, #[sinex_prop `#meta_tokens`], and the
surrounding quote! block).
Comment on lines
+102
to
+107
| let sanitized = test_name.replace("::", "_"); | ||
| let filename = format!( | ||
| "{}-{}.json", | ||
| Utc::now().format("%Y%m%dT%H%M%S%3f"), | ||
| sanitized | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider more robust filename sanitization.
The current sanitization only replaces :: with _. Test names with other characters that are invalid in filenames (e.g., /, \, <, >, :, ", |, ?, *, or control characters) could cause write failures on some platforms.
♻️ Suggested more robust sanitization
- let sanitized = test_name.replace("::", "_");
+ let sanitized: String = test_name
+ .chars()
+ .map(|c| if c.is_alphanumeric() || c == '_' || c == '-' { c } else { '_' })
+ .collect();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let sanitized = test_name.replace("::", "_"); | |
| let filename = format!( | |
| "{}-{}.json", | |
| Utc::now().format("%Y%m%dT%H%M%S%3f"), | |
| sanitized | |
| ); | |
| let sanitized: String = test_name | |
| .chars() | |
| .map(|c| if c.is_alphanumeric() || c == '_' || c == '-' { c } else { '_' }) | |
| .collect(); | |
| let filename = format!( | |
| "{}-{}.json", | |
| Utc::now().format("%Y%m%dT%H%M%S%3f"), | |
| sanitized | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crate/lib/sinex-test-utils/src/snapshot_helper.rs` around lines 102 - 107,
The current sanitization only replaces "::" with "_" and can still produce
invalid filenames; update the sanitization around the sanitized variable (where
test_name is used) to replace any characters not allowed in filenames—e.g., use
a regex to replace all characters except ASCII letters, digits, hyphen,
underscore and dot with "_" and collapse repeated underscores, and optionally
trim the result to a safe max length before building filename with
Utc::now().format(...) (update the `sanitized` assignment and ensure `filename =
format!(...)` uses the cleaned `sanitized`).
Comment on lines
+1070
to
+1079
| sinex_proptest! { | ||
| #![cases = 64] | ||
| #[timeout = "45s"] | ||
| fn property_ulids_roundtrip(value in SinexStrategies::json_payload()) -> TestResult<()> { | ||
| let body = value.to_string(); | ||
| let decoded: Value = serde_json::from_str(&body)?; | ||
| prop_assert_eq!(decoded, value); | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for sinex_proptest macro implementation to verify inner attribute support
# Check macro implementation for cases/timeout handling
ast-grep --pattern $'#[proc_macro]
pub fn sinex_proptest($$$) -> TokenStream {
$$$
}'
# Also search for any tests using this inner attribute syntax
rg -n '#!\[cases' --type rust
rg -n '#\[timeout' --type rust -g '!target/*'Repository: Sinity/sinex
Length of output: 38
🏁 Script executed:
# Find the sinex_proptest macro definition
fd -t f -e rs . | xargs rg -l 'sinex_proptest' | head -20Repository: Sinity/sinex
Length of output: 1016
🏁 Script executed:
# Search for macro definition more broadly
rg -n 'macro.*sinex_proptest|sinex_proptest.*macro' --type rustRepository: Sinity/sinex
Length of output: 250
🏁 Script executed:
# Look for the actual macro implementation
fd -t f -e rs src | xargs rg -B5 -A15 'sinex_proptest'Repository: Sinity/sinex
Length of output: 50371
🏁 Script executed:
# Check if there are actual test examples using this macro
rg -B2 -A8 'sinex_proptest!' --type rust -m 5Repository: Sinity/sinex
Length of output: 38004
🏁 Script executed:
wc -l crate/lib/sinex-test-utils/macros/src/lib.rsRepository: Sinity/sinex
Length of output: 105
🏁 Script executed:
# Extract the sinex_proptest macro implementation
sed -n '485,600p' crate/lib/sinex-test-utils/macros/src/lib.rsRepository: Sinity/sinex
Length of output: 4071
🏁 Script executed:
# Get more context around the attribute parsing to see the complete picture
sed -n '485,650p' crate/lib/sinex-test-utils/macros/src/lib.rsRepository: Sinity/sinex
Length of output: 5770
🏁 Script executed:
# Check how the TEST_PATTERNS.md documentation shows the macro usage patterns
grep -B5 -A10 "sinex_proptest!" docs/TEST_PATTERNS.md | head -60Repository: Sinity/sinex
Length of output: 1766
🏁 Script executed:
# Let me verify the exact syntax issue by examining if there's a mix of inner and outer attributes
sed -n '1070,1079p' docs/TEST_PATTERNS.mdRepository: Sinity/sinex
Length of output: 370
Fix syntax inconsistency in sinex_proptest! block attributes.
The example mixes inner and outer attribute syntax. The sinex_proptest! macro uses parse_inner() for block-level attributes, requiring #! syntax for both cases and timeout. Line 1073 should use #![timeout = "45s"] instead of #[timeout = "45s"].
Corrected example
sinex_proptest! {
#![cases = 64]
#![timeout = "45s"]
fn property_ulids_roundtrip(value in SinexStrategies::json_payload()) -> TestResult<()> {
let body = value.to_string();
let decoded: Value = serde_json::from_str(&body)?;
prop_assert_eq!(decoded, value);
Ok(())
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/TEST_PATTERNS.md` around lines 1070 - 1079, The sinex_proptest! block
mixes inner and outer attribute syntax; change the timeout attribute to use
inner-block syntax so both block attributes are parsed by sinex_proptest!'s
parse_inner(): replace the outer attribute #[timeout = "45s"] with an inner
attribute #![timeout = "45s"] in the sinex_proptest! block that contains fn
property_ulids_roundtrip(...) (the block already uses #![cases = 64], so make
both attributes consistent).
Comment on lines
+92
to
+95
| below). Use `#[sinex_prop]` (or the block-style `sinex_proptest!`) so the | ||
| harness provides tracing, timeouts, context wiring, and seed persistence. | ||
| Add `cases = 256` (or any number) to lock the runner config. Capture any | ||
| failing seeds in that crate’s `tests/property/*.proptest-regressions` files. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the default failure persistence path in the macro implementation
rg -n "proptest-regressions" crate/lib/sinex-test-utils/macros/src/lib.rsRepository: Sinity/sinex
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Read the TESTING.md file around lines 92-95 to see full context
sed -n '85,100p' TESTING.md | cat -nRepository: Sinity/sinex
Length of output: 866
🏁 Script executed:
#!/bin/bash
# Read the macro implementation around line 367 to see env var handling
sed -n '360,375p' crate/lib/sinex-test-utils/macros/src/lib.rs | cat -nRepository: Sinity/sinex
Length of output: 784
Documentation path is inconsistent with implementation.
Line 95 states that failing seeds are captured in tests/property/*.proptest-regressions, but the macro implementation (macros/src/lib.rs, lines 365-368) defaults to target/proptest-regressions. The default is configurable via the SINEX_PROPTEST_DIR environment variable.
Update the documentation to reflect the actual default path and document the environment variable override option.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 93-93: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TESTING.md` around lines 92 - 95, Update the doc string that currently claims
failing seeds are saved under tests/property/*.proptest-regressions to match the
macro implementation: failing seeds default to target/proptest-regressions (this
default is controlled by the SINEX_PROPTEST_DIR environment variable); mention
both the default path and how to override it via SINEX_PROPTEST_DIR in the
documentation for #[sinex_prop] / sinex_proptest! so readers see the actual
behavior implemented in macros::lib (the code that reads SINEX_PROPTEST_DIR).
| the `PROPTEST_CASES` used by `TestRunner`. Capture any new failing seeds in | ||
| that crate’s `tests/property/*.proptest-regressions` files. | ||
| below). Use `#[sinex_prop]` (or the block-style `sinex_proptest!`) so the | ||
| harness provides tracing, timeouts, context wiring, and seed persistence. |
There was a problem hiding this comment.
Add blank line after heading for markdown consistency.
Static analysis indicates a blank line is expected below line 93 per MD022 (blanks-around-headings).
📝 Proposed fix
below). Use `#[sinex_prop]` (or the block-style `sinex_proptest!`) so the
harness provides tracing, timeouts, context wiring, and seed persistence.
+
Add `cases = 256` (or any number) to lock the runner config. Capture any📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| harness provides tracing, timeouts, context wiring, and seed persistence. | |
| below). Use `#[sinex_prop]` (or the block-style `sinex_proptest!`) so the | |
| harness provides tracing, timeouts, context wiring, and seed persistence. | |
| Add `cases = 256` (or any number) to lock the runner config. Capture any |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 93-93: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TESTING.md` at line 93, Add a single blank line immediately after the
Markdown heading that precedes or contains the text "harness provides tracing,
timeouts, context wiring, and seed persistence." to satisfy MD022
(blanks-around-headings); edit the TESTING.md heading block so there is one
empty line between the heading and the following paragraph/line.
Owner
Author
Sinity
added a commit
that referenced
this pull request
Jul 13, 2026
Restore the macro-based property harness on a stable footing, execute async property cases on the active Tokio runtime, and persist structured failure artifacts so regressions are debuggable again. sinex-test-utils macros: - Re-export `sinex_prop` and `sinex_proptest` from `crate/lib/sinex-test-utils/src/lib.rs` and rebuild the procedural macro expansion in `macros/src/lib.rs`, which puts property tests back on a first-class harness instead of a mix of ad-hoc `proptest!` patterns. - Move property-runner setup behind shared helper logic so cases, seeds, shrink behavior, and failure persistence can be configured consistently across crates. runtime execution and failure handling: - Change the async property path in `macros/src/lib.rs` to use `tokio::runtime::Handle::current().block_on(...)`, which fixes the broken “run async property cases from a detached executor” behavior and keeps timeouts tied to the active test runtime. - Add structured failure persistence in `crate/lib/sinex-test-utils/src/snapshot_helper.rs`, writing JSON artifacts under `target/test-artifacts/` (or `SINEX_TEST_FAIL_DIR`) with error details, pool stats, and captured logs/context when available. - Make pool stats serializable and thread the failure snapshot hook through both `sinex_test` and `sinex_prop`, so ordinary async tests and property tests produce comparable failure artifacts. suite migration and docs: - Reapply the harness across the property suites in `sinex-core`, `sinex-schema`, and `sinex-satellite-sdk`, converting older inline `proptest!` blocks to `#[sinex_prop]`/`sinex_proptest!` where needed. - Update `TESTING.md`, `docs/TEST_PATTERNS.md`, `docs/TEST_PATTERNS_GUIDE.md`, and the `sinex-test-utils` README so the preferred property-test surface, seed env vars, and failure-artifact path are documented together. - Remove the short-lived logging/runlog detour and other branch churn so the final harness is centered on macro execution plus snapshot artifacts, not extra always-on telemetry files.
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
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.
Summary
I restored
sinex_propas the supported property-test surface, fixed the async execution model so property cases run on the current Tokio runtime, and added JSON failure artifacts so a failing case carries enough context to debug without rerunning the whole suite blind. The branch also reapplies the harness across the major property suites insinex-core,sinex-schema, andsinex-satellite-sdk, and updates the testing docs to point at the macro-based path again. In practice, this takes us from “property tests technically exist, but each crate has its own conventions and failures are opaque” to “property suites share one harness, one seed/persistence story, and one failure artifact shape.”Motivation
The earlier
sinex_proprollout had been backed out, and the repository regressed into an awkward middle state. Some suites were back on inlineproptest!usage, some async property bodies needed their own runtime tricks, and a failure often told you only that a case failed—not what the surrounding test context or pool state looked like at the time.I wanted to fix that before more test/runtime work piled on top of it. Property tests are only worth carrying if they are easy to write, deterministic to replay, and rich enough on failure that the seed is not your only clue. The branch therefore has a very specific shape: restore the macro surface, fix the runtime boundary, then make the failure output useful.
Changes grouped by concern
1. Put
sinex_propback at the center of the property-test APIcrate/lib/sinex-test-utils/src/lib.rsnow re-exportssinex_propandsinex_proptestagain, and the macro implementation incrate/lib/sinex-test-utils/macros/src/lib.rswas rebuilt so property tests can describe strategies declaratively while still getting the same context wiring, timeouts, and result handling as normalsinex_testcases.That lets tests read like this again:
The important part is not just the syntax. It is that seeds, case counts, shrink behavior, failure persistence, and context wiring are now routed through the same macro path instead of being reimplemented per suite.
2. Fix async property execution at the runtime boundary
The key bug fix in the branch is how async property cases are executed. The broken version tried to bridge async work from the property runner in a way that detached execution from the test runtime. The fixed version captures the active Tokio handle and runs each case against it:
This matters because it keeps timeouts, context setup, and async resource use inside the runtime the harness already owns. Without that, property tests can fail in weird, indirect ways that look like test flakiness rather than a harness bug.
3. Make failures carry useful context
I added structured failure persistence in
crate/lib/sinex-test-utils/src/snapshot_helper.rs. On failure, bothsinex_testandsinex_propnow write a JSON artifact undertarget/test-artifacts/by default, overridable withSINEX_TEST_FAIL_DIR.The snapshot includes:
TestContextsummary data,This is deliberately lighter than a full runlog stream but much more useful than a bare
❌line. It gives CI artifacts and local reruns enough information to see whether a failure came with connection pressure, log noise, or an obvious context mismatch.4. Reapply the harness across the actual property suites
The branch touches a wide set of tests because the harness is only valuable if the suites actually use it again. I reapplied it across the property-focused modules in
sinex-core,sinex-schema, andsinex-satellite-sdk, including files such as:event_property.rsevent_model_fuzzing_test.rsevent_validation_property_test.rsschema_property_test.rsulid_property_tests.rsproperty_checkpoint_state.rsIn several places that meant converting manual
proptest!blocks intosinex_proptest!or#[sinex_prop], and in a few places it meant retyping parameters so the generated macro surface stayed readable and compile-time checked.5. Update the docs to match the restored harness
TESTING.mdnow points property-test authors at#[sinex_prop]rather than the older#[sinex_test(proptest)]guidance.docs/TEST_PATTERNS.mdanddocs/TEST_PATTERNS_GUIDE.mdnow document the restored macro surface, the seed/case-count environment variables, and the new failure snapshot directory. Thesinex-test-utilsREADME also explains that failing tests emit JSON artifacts with logs and pool stats when a context is present.That documentation update matters because the last rollout failure left the repo with an API/documentation mismatch. This branch fixes the code and the instructions together.
Testing
The branch’s coverage is mostly self-hosting: it migrates the large property suites that exercise event creation, validation, schema behavior, ULID round-trips, checkpoint state, and boundary cases onto the restored macros. The macro implementation itself is covered by the fact that those suites now compile and execute through it again, and the runtime fix specifically addresses async property execution on the active Tokio handle.
Failure artifact persistence is also wired into ordinary
sinex_testfailures, so the snapshot path is not limited to property tests. That broadens confidence that the snapshot format and file-writing path will stay exercised even when a specific property suite is quiet.The main gap is that the failure artifact format is mostly validated through harness use rather than a standalone golden-file test of every JSON field. I am comfortable with that because the format is intentionally simple and the value here is the integration point.
Notes
One piece of branch churn is worth explaining: there was a short-lived runlog/logging detour in the middle of the work. I intentionally did not keep that as the center of the final change. The stable long-term value is the restored property harness plus failure snapshots, not another always-on telemetry stream.
This branch is also deliberately test-focused rather than runtime-focused. I wanted the harness repaired before more branches leaned on it, so the squash stays concentrated on the macros, the migrated suites, and the debugging experience when something fails.
Ref #101
Summary by CodeRabbit
Release Notes
New Features
#[sinex_prop]andsinex_proptest!macros for simplified property-based testing with built-in support for timeouts, seeding, and context injection.Refactor
Documentation