Skip to content

test(rocm-core): give the JSONL store tests a record type of their own - #920

Closed
jussielo-amd wants to merge 1 commit into
mainfrom
test-jsonl-store-own-fixtures
Closed

jussielo-amd wants to merge 1 commit into
mainfrom
test-jsonl-store-own-fixtures

Conversation

@jussielo-amd

Copy link
Copy Markdown
Collaborator

Summary

This delivers ROCMAI-886, which came out of the review of the rocm-core split. The tests in crates/rocm-core/src/jsonl_store.rs drove the generic JSONL store engine through the automation audit store: they imported AuditEventRecord, append_audit_event and load_recent_audit_events and seeded files through helpers shaped around that record. That is a test-only edge from the generic engine back to one of its users, and it meant the engine's own tests could not be read, run or moved without the first customer. It did no harm to the build, which is why it was left as a follow-up, but the dependency pointed the wrong way.

The store tests now carry their own small record type (Entry, a tag plus padding so a serialized line is about as long as a real store's) and a handful of local helpers, and the engine's tests no longer name anything from automation.rs. The automation tests are untouched and keep covering the automation-specific stores through the shared audit fixtures in test_support.rs.

This is a test-only change: no production code changes, every leaf test name is kept, and each test makes the same assertions about the same behaviour.

What moved

Before After
AuditEventRecord, audit_record, audit_line, audit_messages from test_support Entry, entry, entry_line, entry_tags, local to jsonl_store::tests
seed_audit_store, paths.audit_events_path(), paths.ensure() seed_store, temp_store (a store path under a throwaway root, directory created)
append_audit_event, load_recent_audit_events append_entry (no rotation), load_recent_entries (the same four generations the audit reader used), over append_jsonl_record and load_recent_jsonl_records
repair_audit_store_with repair_store_with, taking the store path
the_store_lock_is_a_sibling_of_the_store built paths through AppPaths accessors the same assertion over two plain paths in different directories

test_support.rs keeps its audit helpers, since automation.rs still uses them; only its module comment changes to say so. docs/architecture.md no longer describes the test-only edge, and docs/testing.md notes that the engine's tests use a record type of their own.

Verification

Ran locally:

  • cargo fmt --all --check and cargo clippy -p rocm-core --all-targets -- -D warnings: clean.
  • cargo test -p rocm-core --lib --no-fail-fast: 1004 passed, 3 ignored, before and after. The sorted --list of test names is byte-identical before and after (1007 entries), so no leaf name was added, dropped or renamed.
  • cargo xtask manifest --check and cargo xtask check-crate-edges: both pass. Lychee over docs/architecture.md and docs/testing.md passes, and the repository's pre-commit hooks passed on the commit.
  • Mutation checks, each reverted afterwards. The target of each edit was broken and the covering tests went red: skipping the rotation step (5 tests fail, including the two concurrency tests and load_recent_spans_a_rollover); dropping the store lock (rotation_under_concurrent_threads_loses_no_records and rotation_keeps_the_live_file_and_the_configured_generations fail); changing the lock file's name (the_store_lock_is_a_sibling_of_the_store); disabling the shrink retry (load_recent_retries_past_a_shrink_mid_scan_and_succeeds); dropping the generation shift (7 rotation tests fail); making a successful replace fall through to the truncate (a_torn_tail_is_not_truncated_when_replacing_the_store_works); and cutting the torn tail one byte too far (three torn-tail tests fail).
  • Coverage: cargo llvm-cov -p rocm-core --summary-only reports 89.58% of 32797 lines before and 89.57% of 32793 lines after (jsonl_store.rs itself 96.21% to 95.91%: three more )?; error-propagation lines of multi-line test calls are unexecuted, as they were for the equivalent calls before). That is about 0.01 points, well inside the 0.2 point tolerance of cargo xtask coverage, and coverage-floors.toml is not touched. I did not run the full cargo xtask coverage across the workspace.

Not run locally: the Windows cross-check of the cfg(windows) test (append_repairs_a_torn_tail_while_another_handle_blocks_replacing_the_store), because ring's build script needs lib.exe, which this Linux host lacks. That test was edited the same way as its Unix sibling append_repairs_a_torn_tail_in_a_directory_that_takes_no_new_files (a store path and append_entry in place of the audit accessors), and the Windows CI job is what exercises it. The full workspace gate, cargo xtask e2e and scripts/smoke_local.py are left to CI.

User-visible behaviour

None, so no Gherkin scenario applies: the change is confined to a #[cfg(test)] module and two documentation sentences, and no command output, exit code, file or engine selection changes.

Notes for reviewers

It is a single commit. The interesting part is the fixture block at the top of jsonl_store::tests; the rest of the diff is mechanical substitution of the audit helpers with those. The padding in Entry is deliberate: the rotation tests size their byte caps against a realistic line length (TEST_ROTATION rotates after about 14 records, and rotation_under_concurrent_threads_loses_no_records asserts it rotates more than once), and the torn-tail tests cut the first 20 bytes of a line, so a minimal one-field record would have changed what they exercise. The store is named events.jsonl because a_failed_rotation_prints_its_warning_once_to_stderr asserts the warning names that file.

The tests in jsonl_store.rs drove the generic store engine through the automation audit store: they imported AuditEventRecord, append_audit_event and load_recent_audit_events and seeded files with helpers shaped around that record. That is a test-only edge from the generic engine back to one of its users, so the engine's own tests could not be read or run without its first customer.

They now use a small local record (Entry, padded so a serialized line is about as long as a real store's, which the rotation caps and the torn-tail cuts are sized against) and local helpers over append_jsonl_record and load_recent_jsonl_records. automation.rs keeps its audit fixtures in test_support.rs and its own tests are unchanged. Test-only: every leaf test name and assertion is kept, and no production code changes. docs/architecture.md and docs/testing.md no longer describe the test-only edge.

Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues block approval of the test-fixture decoupling change.

No open issues 🎉

What changed in this PR

Decouples the generic JSONL store tests from automation-specific records and helpers, keeping test fixtures aligned with the engine’s role in rocm-core. Production behavior is unchanged.

Changes:

  • Introduces a local Entry record and helpers for seeding, appending, reading, and repairing test stores.
  • Migrates existing tests to those fixtures while preserving test names and assertions.
  • Updates documentation and fixture comments to describe the removed test dependency.
File Description
docs/​testing.md Documents the engine tests’ independent record type.
docs/​architecture.md Removes the description of the test-only dependency on automation.
crates/​rocm-core/​src/​test_support.rs Clarifies fixture usage in the module comment.
crates/​rocm-core/​src/​jsonl_store.rs Replaces audit-specific test fixtures with local records and store helpers.

Review effort: Balanced

@jussielo-amd
jussielo-amd marked this pull request as ready for review November 5, 2026 17:12
@jussielo-amd
jussielo-amd requested a review from a team as a code owner November 5, 2026 17:12
@jussielo-amd
jussielo-amd requested a review from rominf November 5, 2026 17:12
@jussielo-amd

Copy link
Copy Markdown
Collaborator Author

Superseded by #932: this pull request's commits are included in that branch (head 02e2bf5), and #932's description covers this change. Closing so the review happens in one place.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants