Back to feed

sledtools/pika branch #72

pika-orch-incus-cleanup-14

Simplify pika-news structured event fixtures

Target branch: master

Merge Commit: 994722dc101b6ae759052c47ced327e9cf730916

branch: merged tutorial: ready ci: success
Open CI Details

Continuous Integration

CI: success

Compact status on the review page, with full logs on the CI page.

Open CI Details

Latest run #91 success

2 passed

head cf1964ed07231b0e8c277b544be471f30f2d4010 · queued 2026-03-25 21:14:12 · 2 lane(s)

queued 7s · ran 23s

check-notifications · success check-agent-contracts · success

Summary

This branch replaces hand-written, inline JSON string literals in the pika-news structured event test fixtures with type-safe Rust constructors that serialise via serde. Three new helper functions—fixture_job_event, fixture_run_event, and event_line—are introduced in the test module and then used across three test sites: the default structured CI wrapper script, the custom-lane structured wrapper script, and the failure-message rendering unit test. The result is dramatically more readable test code that is validated at compile time against the pikaci domain types, eliminating a class of silent breakage when event schemas evolve.

Tutorial Steps

Import pikaci domain types into the test module

Intent: Bring `JobRecord`, `RunLifecycleEvent`, `RunRecord`, and `RunStatus` into scope so the test helpers can construct real domain objects instead of raw JSON strings.

Affected files: crates/pika-news/src/forge.rs

Evidence
@@ -1228,6 +1228,7 @@ mod tests {
     use std::time::Duration;
 
     use chrono::Utc;
+    use pikaci::{JobRecord, RunLifecycleEvent, RunRecord, RunStatus};

A single use statement is added at the top of mod tests to import the four key types from the pikaci crate. These are the canonical types that the production code deserialises from the CI runner's JSONL output, so constructing them directly in tests guarantees schema compatibility at compile time.

Add fixture_job_event helper

Intent: Provide a reusable constructor for `JobRecord` test instances, parameterised only by the fields that vary across tests (status, exit_code, message) while hard-coding sensible defaults for everything else.

Affected files: crates/pika-news/src/forge.rs

Evidence
@@ -1253,6 +1254,74 @@ mod tests {
+    fn fixture_job_event(
+        status: RunStatus,
+        exit_code: Option<i32>,
+        message: Option<&str>,
+    ) -> JobRecord {
+        JobRecord {
+            id: "job-one".to_string(),
+            description: "job one".to_string(),
+            status,
+            executor: "remote_linux_vm".to_string(),
+            ...
+            finished_at: match status {
+                RunStatus::Running => None,
+                _ => Some("2026-03-19T00:00:02Z".to_string()),
+            },
+            exit_code,
+            message: message.map(ToOwned::to_owned),
+            ...
+        }
+    }

The fixture_job_event function accepts only the three fields that actually differ between test scenarios—status, exit_code, and message—and fills every other field with deterministic defaults. A match on status conditionally sets finished_at to None when the job is still Running, mirroring real runtime behaviour. This eliminates duplicated, fragile JSON blobs that previously encoded the same information across multiple tests.

Add fixture_run_event helper

Intent: Provide a reusable constructor for `RunRecord` test instances, parameterised by run_id, status, message, and the list of embedded jobs.

Affected files: crates/pika-news/src/forge.rs

Evidence
@@ -1253,6 +1254,74 @@ mod tests {
+    fn fixture_run_event(
+        run_id: &str,
+        status: RunStatus,
+        message: Option<&str>,
+        jobs: Vec<JobRecord>,
+    ) -> RunRecord {
+        RunRecord {
+            run_id: run_id.to_string(),
+            status,
+            ...
+            changed_files: Vec::new(),
+            filters: Vec::new(),
+            message: message.map(ToOwned::to_owned),
+            prepare_timings: Vec::new(),
+            jobs,
+        }
+    }

fixture_run_event follows the same pattern as fixture_job_event: the four fields that vary across tests are parameters, while the many prepared_output_* and other stable fields are hard-coded. This is where the biggest readability win occurs—RunRecord has over 20 fields, and the old inline JSON encoded every one of them as escaped string fragments inside a Rust concat! macro.

Add event_line serialisation helper

Intent: Provide a one-liner that serialises any `RunLifecycleEvent` variant to a JSON string suitable for embedding in test wrapper scripts or passing directly to the rendering function under test.

Affected files: crates/pika-news/src/forge.rs

Evidence
@@ -1253,6 +1254,74 @@ mod tests {
+    fn event_line(event: RunLifecycleEvent) -> String {
+        serde_json::to_string(&event).expect("serialize test event")
+    }

event_line is a thin wrapper around serde_json::to_string that panics with a clear message on serialisation failure. By delegating serialisation to serde, the tests automatically stay in sync with any #[serde(rename)] or structural changes on RunLifecycleEvent. This replaces manually-constructed JSON strings that could silently drift from the real schema.

Rewrite the default structured CI wrapper test to use fixture helpers

Intent: Replace the inline `concat!` blob of escaped JSON with structured `RunLifecycleEvent` construction and `format!`-based script templating.

Affected files: crates/pika-news/src/forge.rs

Evidence
@@ -1539,21 +1608,44 @@ mod tests {
+        let run_started = event_line(RunLifecycleEvent::RunStarted {
+            run_id: "pikaci-run-123".to_string(),
+            ...
+        });
+        let job_started = event_line(RunLifecycleEvent::JobStarted {
+            run_id: "pikaci-run-123".to_string(),
+            job: Box::new(fixture_job_event(RunStatus::Running, None, None)),
+        });
+        let job_finished = event_line(RunLifecycleEvent::JobFinished {
+            run_id: "pikaci-run-123".to_string(),
+            job: Box::new(fixture_job_event(RunStatus::Passed, Some(0), None)),
+        });
+        let run_finished = event_line(RunLifecycleEvent::RunFinished {
+            run: Box::new(fixture_run_event(
+                "pikaci-run-123",
+                RunStatus::Passed,
+                None,
+                Vec::new(),
+            )),
+        });
         fs::write(
             &wrapper_path,
-            concat!(
+            format!(
                 ...
+{run_started}\n\
+{job_started}\n\
+{job_finished}\n\
+{run_finished}\n\
             ),

The test that exercises the default pikaci wrapper script path previously contained four lines of hand-escaped JSON embedded in a concat! macro. Each line is now built from a RunLifecycleEvent variant, serialised with event_line, and interpolated into the bash heredoc via format!. The bash argument-checking preamble is unchanged—only the data payload lines are replaced.

This gives two concrete benefits:

  1. Compile-time validation — if a field is added to JobRecord or RunRecord, the test will fail to compile until updated.
  2. Readability — the intent of each event (e.g. RunStatus::Running vs RunStatus::Passed) is immediately visible rather than buried in escaped JSON.

Rewrite the custom-lane structured wrapper test to use fixture helpers

Intent: Apply the same fixture-based approach to the second integration test, which uses a custom lane script name and a simpler event sequence (run_started + run_finished only, no jobs).

Affected files: crates/pika-news/src/forge.rs

Evidence
@@ -1624,19 +1716,34 @@ mod tests {
+        let run_started = event_line(RunLifecycleEvent::RunStarted {
+            run_id: "pikaci-run-456".to_string(),
+            ...
+        });
+        let run_finished = event_line(RunLifecycleEvent::RunFinished {
+            run: Box::new(fixture_run_event(
+                "pikaci-run-456",
+                RunStatus::Passed,
+                None,
+                Vec::new(),
+            )),
+        });
         fs::write(
             &wrapper_path,
-            concat!(
+            format!(
                 ...
+{run_started}\n\
+{run_finished}\n\
             ),

The custom-lane test mirrors the previous refactor but with a different run_id (pikaci-run-456) and only two events. The pattern is identical: construct events with the helpers, serialise, and interpolate into the bash script template with format!.

Rewrite the failure-message rendering unit test to use fixture helpers

Intent: Replace the raw JSON string literals passed to `render_pikaci_stdout_line` with structured event construction, making the failure-specific fields (status, exit_code, message) explicit.

Affected files: crates/pika-news/src/forge.rs

Evidence
@@ -1708,14 +1815,28 @@ mod tests {
     fn structured_pikaci_failure_log_keeps_job_and_run_messages() {
         let job_line = render_pikaci_stdout_line(
-            r#"{"event":"job_finished",...}"#,
+            &event_line(RunLifecycleEvent::JobFinished {
+                run_id: "pikaci-run-123".to_string(),
+                job: Box::new(fixture_job_event(
+                    RunStatus::Failed,
+                    Some(1),
+                    Some("cargo test failed in crate pika_core"),
+                )),
+            }),
             ...
         let run_line = render_pikaci_stdout_line(
-            r#"{"event":"run_finished",...}"#,
+            &event_line(RunLifecycleEvent::RunFinished {
+                run: Box::new(fixture_run_event(
+                    "pikaci-run-123",
+                    RunStatus::Failed,
+                    Some("prepared-output helper exited 1"),
+                    Vec::new(),
+                )),
+            }),

This unit test verifies that render_pikaci_stdout_line preserves failure messages in its output. The old version passed raw JSON r#"..."# strings that were difficult to read and impossible to validate at compile time. The new version makes the failure scenario explicit: RunStatus::Failed, exit code 1, and a descriptive message string. The assertions remain unchanged, confirming the refactor is behaviour-preserving.

Diff