Skip to content

test: insta snapshot tripwire for persisted-storage JSON shapes#975

Closed
anikettuli wants to merge 1 commit intoStremio:developmentfrom
anikettuli:claude/snapshot-tripwire
Closed

test: insta snapshot tripwire for persisted-storage JSON shapes#975
anikettuli wants to merge 1 commit intoStremio:developmentfrom
anikettuli:claude/snapshot-tripwire

Conversation

@anikettuli
Copy link
Copy Markdown

Summary

Adds a new src/unit_tests/snapshots.rs module that uses the insta crate to freeze the serialized JSON shape of every bucket that stremio-core writes to persistent storage. Each storage key listed in src/constants.rs now has a committed .snap fixture; any schema or field-name change to one of these types produces a visible cargo insta review diff that cannot accidentally land.

Why this change

Storage keys (profile, library, library_recent, streams, search_history, streaming_server_urls, notifications, calendar, dismissed_events) are installed on end-user devices. A silent rename of a serialized field — say a typo fix on a variant, or a #[serde(rename)] added during a refactor — breaks every existing install with no compiler warning and no user-visible cue until someone tries to load their saved state.

The existing coverage in src/unit_tests/serde/ tests round-trips for individual types, but has no complete tripwire for the composite bucket shapes. If someone adds a field to Profile that defaults to serde_json::Value::Null on deserialization but changes the serialized output, the round-trip tests still pass. The snapshot suite catches that kind of drift.

Effect

  • Any field rename, shape change, or default-value change on a storage-bound type produces an insta diff in CI. Contributors can't accidentally break existing user data.
  • Updating a snapshot intentionally is still fast: cargo insta review → inspect the diff → accept. The expected pairing with a migration step in src/runtime/env.rs is now a visible, enforceable reviewer checkbox rather than an unwritten rule.
  • Zero runtime behavior change. Tests-only addition.

Scope chosen deliberately

In scope (this PR): storage bucket types — the persistence contract.

  • Profile (default state, including the bundled official addons)
  • LibraryBucket (empty) — covers both library and library_recent
  • StreamsBucket, SearchHistoryBucket, ServerUrlsBucket, NotificationsBucket, DismissedEventsBucket (empty)

Out of scope (not this PR):

  • Msg / Action / Event JSON — these are dispatch-internal, not persisted. Breaking them affects live clients (where the on-the-wire contract matters for a running app session), but doesn't corrupt installed user data. The existing src/unit_tests/serde/ suite covers this class at the type level.
  • Deep-link URLs — already covered by existing src/unit_tests/deep_links/*.rs tests (now with a round-trip helper for the flate2-encoded player URL, see build: unblock cargo update + stremio-watched-bitfield base64 0.13 → 0.22 #974).

Files changed

  • src/unit_tests/snapshots.rs — new, 7 tests.
  • src/unit_tests/snapshots/*.snap — 7 committed fixtures.
  • src/unit_tests/mod.rs — register the module.
  • Cargo.toml — add insta = { version = "1", features = ["json"] } to [dev-dependencies].

How to review a snapshot change in a future PR

When a legitimate schema change lands, the author runs:

cargo insta review

which shows each new/changed snapshot side-by-side with the previous committed version. The author accepts the diff and commits the updated .snap file in the same PR as the code change. Reviewers see the .snap diff alongside the type change and can verify:

  1. Is the rename/addition intentional?
  2. Is there a migration step in src/runtime/env.rs for it?
  3. Does the migration have its own test fixture?

Test plan

Verified locally (Rust 1.85.1 stable-x86_64-pc-windows-gnu):

  • cargo test -p stremio-core --lib209 passed, 0 failed (202 original + 7 new snapshot tests)
  • cargo clippy --all --no-deps -- -D warnings — clean
  • cargo fmt --check — clean

Dependency on other PRs

Adds a new src/unit_tests/snapshots.rs module that uses `insta` to freeze
the JSON serialization of every bucket stremio-core writes to persistent
storage. Each storage key listed in src/constants.rs (profile, library,
library_recent, streams, search_history, streaming_server_urls,
notifications, calendar, dismissed_events) now has a corresponding
committed `.snap` file; any schema or field-name change produces a
visible `cargo insta review` diff that cannot accidentally land.

Why this matters: storage keys are installed on end-user devices. A
silent change to a serialized field name breaks every existing install
with no compiler hint. The snapshot suite converts that class of bug
from "manual audit required" to "CI-gated diff".

Scope chosen deliberately:
- In: storage bucket types (Profile, LibraryBucket, StreamsBucket,
  NotificationsBucket, SearchHistoryBucket, ServerUrlsBucket,
  DismissedEventsBucket). These are the persistence contract.
- Out: Msg/Action/Event JSON - not persisted to storage; covered
  implicitly by the existing serde unit tests in src/unit_tests/serde/.
- Out: deep-link URLs - already covered by the existing tests in
  src/unit_tests/deep_links/ (with the new round-trip helper).

Updating a snapshot intentionally: `cargo insta review`, inspect the
diff, accept it, and pair it with a migration step in runtime/env if
the wire format is not backward-compatible.

Files:
- src/unit_tests/snapshots.rs (new, 7 tests)
- src/unit_tests/snapshots/*.snap (7 committed fixtures)
- src/unit_tests/mod.rs: register the module
- Cargo.toml: add `insta = { version = "1", features = ["json"] }` dev-dep

Verified locally (Rust 1.85.1 stable-x86_64-pc-windows-gnu):
  cargo test -p stremio-core --lib -> 209 passed, 0 failed (202 + 7 new)
  cargo clippy --all --no-deps -D warnings -> clean
  cargo fmt --check                         -> clean
Copilot AI review requested due to automatic review settings April 20, 2026 05:43
@github-actions
Copy link
Copy Markdown

stremio-core-web prebuild

Prebuild artifact published by the Build workflow for branch claude/snapshot-tripwire.

Paste one of these into stremio-web's package.json and run pnpm install to wire this prebuild into a corresponding stremio-web PR:

Release

"@stremio/stremio-core-web": "https://stremio.github.io/stremio-core/stremio-core-web/claude/snapshot-tripwire/stremio-stremio-core-web-0.56.3.tgz"

Dev

"@stremio/stremio-core-web": "https://stremio.github.io/stremio-core/stremio-core-web/claude/snapshot-tripwire/dev/stremio-stremio-core-web-0.56.3.tgz"

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an insta-based snapshot test suite to lock down the JSON serialization “shape” of persisted storage buckets, so accidental serde/schema drift becomes a visible diff during review.

Changes:

  • Added src/unit_tests/snapshots.rs with snapshot tests for Profile::default() and several bucket defaults.
  • Committed .snap fixtures under src/unit_tests/snapshots/.
  • Registered the new unit-test module and added insta as a dev-dependency (with corresponding Cargo.lock updates).

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/unit_tests/snapshots.rs New insta snapshot tests + documentation for persistence-contract tripwire.
src/unit_tests/mod.rs Registers the new snapshots unit-test module.
src/unit_tests/snapshots/*.snap New committed JSON snapshot fixtures for each test.
Cargo.toml Adds insta under [dev-dependencies].
Cargo.lock Lockfile updates including new insta deps and additional resolved changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +21 to +22
//! `streaming_server_urls`, `notifications`, `calendar`,
//! `dismissed_events`.
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

The module docs claim the snapshot suite covers the calendar storage key, but this file doesn't define a calendar snapshot test and there is no .snap fixture for it. Also CALENDAR_STORAGE_KEY appears to be unused outside src/constants.rs, so either add the missing coverage (if something is persisted under this key) or remove calendar from the documented coverage list to avoid misleading future readers.

Suggested change
//! `streaming_server_urls`, `notifications`, `calendar`,
//! `dismissed_events`.
//! `streaming_server_urls`, `notifications`, `dismissed_events`.

Copilot uses AI. Check for mistakes.
Comment on lines +49 to +54
/// Empty `LibraryBucket` — covers both the `library` and `library_recent`
/// storage keys (same wire shape, split by recency of access).
#[test]
fn snapshot_library_bucket_empty() {
assert_json_snapshot!(LibraryBucket::default());
}
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

Snapshotting LibraryBucket::default() (and the other buckets) only verifies the top-level field names/container types; it won’t catch breaking JSON changes inside persisted item types (e.g., LibraryItem field rename/default-value changes) because items is empty. If the goal is a “shape tripwire” for persisted data, consider snapshotting a minimal non-empty bucket with one representative item so nested serialization changes also produce an insta diff.

Copilot uses AI. Check for mistakes.
/// [`crate::constants::NOTIFICATIONS_STORAGE_KEY`].
#[test]
fn snapshot_notifications_bucket_empty() {
assert_json_snapshot!(NotificationsBucket::default());
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

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

This test uses NotificationsBucket::default(), but NotificationsBucket’s default impl is only derived under cfg(test) and sets created to the Unix epoch, while production code initializes this bucket via NotificationsBucket::new::<E>(...) (which uses E::now()). To better reflect the real persisted wire format (and avoid test-only defaults masking changes), consider constructing the bucket via the production constructor with a deterministic test Env/time, or redact the timestamp field in the snapshot.

Suggested change
assert_json_snapshot!(NotificationsBucket::default());
assert_json_snapshot!(
NotificationsBucket::default(),
{
".created" => "[created]",
}
);

Copilot uses AI. Check for mistakes.
@kKaskak
Copy link
Copy Markdown
Member

kKaskak commented Apr 20, 2026

i think the real fix for this would be to ignore the fields that cannot be parsed etc instead of just guarding, example now users cannot downgrade core if they would like to right? the app would break adding tests here does absolutely nothing
Ui handles that by prompting to clear data for now

@kKaskak kKaskak closed this Apr 20, 2026
@anikettuli
Copy link
Copy Markdown
Author

Makes sense — if the UI-level "clear data" flow is the intended backstop, a CI-only tripwire is friction without matching value. Appreciate you taking the time to explain rather than just closing. I'll keep the reasoning in mind for future contributions.

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.

3 participants