test: corrupt team cache is non-fatal for issue view (#194)#244
Merged
test: corrupt team cache is non-fatal for issue view (#194)#244
Conversation
Add integration test that writes truncated JSON to teams.json, runs
`jr issue view`, and asserts the command succeeds with an actionable
hint ("name not cached — run 'jr team list --refresh'") inline in the
Team row.
Scope note: the issue's original proposal called for "stderr contains a
warning about the cache", but that's the I/O-error path
(src/cli/issue/list.rs:951). Corrupt JSON takes a different path —
src/cache.rs:23-26 maps serde_json::from_str failures to Ok(None), and
the handler at list.rs:947 surfaces the same inline hint used for a
cold cache. That hint is equally actionable (user runs the refresh to
overwrite the corrupt file), so the test pins the real behavior rather
than asserting the stderr warning that doesn't exist for this path.
Refs #194
There was a problem hiding this comment.
Pull request overview
Adds an integration test to ensure jr issue view gracefully falls back (non-fatal) when the team cache file (teams.json) is corrupt, surfacing the UUID plus the existing “name not cached — run 'jr team list --refresh'” hint inline.
Changes:
- Add
issue_view_corrupt_team_cache_falls_back_gracefullyintegration test that writes invalid JSON toteams.json. - Assert
jr issue viewsucceeds and stdout includes both the team UUID and the cache-refresh hint.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Per Copilot review: the comment said "Truncated JSON" but the previous
content ("{ not json") was malformed tokens rather than a truncation.
Use `{"teams": [` — valid JSON opening abruptly cut off — so the test
fixture matches its stated partial-write scenario.
Both inputs trigger the same Ok(None) fall-through (verified by the
unit test at src/cache.rs:661), so coverage is unchanged.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Adds an integration test that writes truncated JSON to
teams.json, runsjr issue view, and asserts the command succeeds with an actionable hint surfaced inline in the Team row. Locks the graceful-degradation guarantee against regressions.Divergence from issue's original proposal
The issue proposed "assert stderr contains a warning about the cache". That's the I/O-error path (
src/cli/issue/list.rs:951→eprintln!("warning: failed to read team cache: {e}")). But corrupt JSON takes a different path:src/cache.rs:23-26mapsserde_json::from_strerrors toOk(None)(notErr)list.rs:947then uses the same fall-through as a cold cache:"{UUID} (name not cached — run 'jr team list --refresh')"inline in the output tableThat fall-through is equally actionable — running the refresh overwrites the corrupt file, fixing the problem. So the test pins the real behavior (stdout has the UUID + inline hint) rather than asserting the stderr warning that doesn't fire for this path.
If we later decide corrupt caches warrant their own user-facing signal (distinct from "cache empty"), that's a production change to
read_cache, not a test change — worth a separate issue.Test plan
cargo fmt --all -- --checkcleancargo clippy --all-targets -- -D warningscleancargo test— 814 passed, 0 failed (baseline 813 + 1 new)Scope
Single test, no production changes. Uses existing
common::fixtures::issue_response_with_teamfixture.Refs #194