tests/unresolved_counts_test.rs
Ref: Size: 17.9 KiB History
//! Surfacing *how much* feedback is still unanswered, in the views a reviewer
//! scans rather than the one they open.
//!
//! `28fc6751` made a single thread's answered-ness visible on `patch show`.
//! That answers "is this thread done" one patch at a time, which is the wrong
//! shape for the question a reviewer actually starts with: *which* patches
//! have unanswered feedback. Answering that by opening every patch is the tax
//! this removes.
//!
//! The count is one number with one definition, computed once from the folded
//! `PatchState` and rendered by `patch list`, the dashboard and the web list.
//! Three properties are load-bearing and each is pinned here:
//!
//! * **The definition.** An unresolved comment is an inline comment carrying
//! no standing resolution and not withdrawn. The two edge cases that forced
//! a decision — a *stale* claim and a *tombstoned* comment — are tested
//! directly, because a count cannot carry the caveat `patch show` prints.
//! * **Agreement.** The CLI, the JSON and the web list report the same number
//! for the same repository. A number that means one thing in one view and
//! something else in another is worse than no number.
//! * **Reading is not writing.** Rendering a list must not append events or
//! move refs. This project has a history of exactly that bug.
mod common;
use common::{ServerHarness, TestRepo};
// ===========================================================================
// Helpers
// ===========================================================================
/// A patch over `feature.txt`, left checked out on `main`. Returns the
/// abbreviated id `patch create` prints, which is what a user would then type;
/// the full id only ever appears in `--json`.
fn patch_over_a_file(repo: &TestRepo, title: &str, branch: &str) -> String {
repo.git(&["checkout", "-b", branch]);
repo.commit_file("feature.txt", "v1\n", &format!("commit for {}", title));
let out = repo.run_ok(&["patch", "create", "-t", title, "-B", branch]);
repo.git(&["checkout", "main"]);
out.trim()
.strip_prefix("Created patch ")
.unwrap_or_else(|| panic!("unexpected patch create output: {}", out))
.to_string()
}
/// Leave an inline comment on `feature.txt:1` and return its full id.
fn inline_comment(repo: &TestRepo, id: &str, body: &str) -> String {
repo.run_ok(&[
"patch",
"comment",
id,
"--file",
"feature.txt",
"--line",
"1",
"-b",
body,
]);
let json = repo.run_ok(&["patch", "show", id, "--json"]);
let value: serde_json::Value = serde_json::from_str(&json).expect("valid JSON");
let comments = value["inline_comments"]
.as_array()
.expect("inline_comments is an array");
comments
.last()
.expect("at least one inline comment")
.get("commit_id")
.and_then(|v| v.as_str())
.expect("commit_id on the inline comment")
.to_string()
}
/// The `unresolved_comments` field `patch list --json` reports for `id`.
fn json_count(repo: &TestRepo, id: &str) -> u64 {
let json = repo.run_ok(&["patch", "list", "--json", "-a", "--archived"]);
let value: serde_json::Value = serde_json::from_str(&json).expect("valid JSON");
let entry = value
.as_array()
.expect("a JSON array")
.iter()
.find(|p| p["id"].as_str().is_some_and(|full| full.starts_with(id)))
.unwrap_or_else(|| panic!("patch {} missing from `patch list --json`: {}", id, json));
entry
.get("unresolved_comments")
.unwrap_or_else(|| {
panic!(
"`patch list --json` carries no `unresolved_comments` key: {}",
json
)
})
.as_u64()
.unwrap_or_else(|| panic!("`unresolved_comments` is not a number: {}", json))
}
/// The row `patch list` prints for `id`.
fn list_row(repo: &TestRepo, id: &str) -> String {
let listing = repo.run_ok(&["patch", "list", "-a", "--archived"]);
listing
.lines()
.find(|l| l.split_whitespace().next().is_some_and(|first| first == id))
.unwrap_or_else(|| panic!("patch {} missing from `patch list`:\n{}", id, listing))
.to_string()
}
// ===========================================================================
// The count, on the CLI
// ===========================================================================
#[test]
fn patch_list_says_how_much_feedback_is_still_unanswered() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "needs answers", "feat-count");
let first = inline_comment(&repo, &id, "this needs a guard");
inline_comment(&repo, &id, "and this name is wrong");
let row = list_row(&repo, &id);
assert!(
row.contains("2 unresolved"),
"`patch list` does not say how much feedback is outstanding:\n{}",
row
);
repo.run_ok(&["patch", "resolve", &id, &first[..8]]);
let row = list_row(&repo, &id);
assert!(
row.contains("1 unresolved"),
"answering a thread did not move the count:\n{}",
row
);
}
/// Zero is not printed, for the same reason `(N new)` is not: a list is
/// scanned, and a column of `(0 unresolved)` is noise that trains the eye to
/// skip the very thing the column exists to show. `--json` still carries the
/// zero; see below.
#[test]
fn a_patch_with_nothing_outstanding_says_nothing() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "all answered", "feat-quiet");
let comment = inline_comment(&repo, &id, "one thing");
repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
let row = list_row(&repo, &id);
assert!(
!row.contains("unresolved"),
"a fully answered patch should not carry a count:\n{}",
row
);
}
/// The scripted surface has the opposite requirement to the text one. A caller
/// parsing JSON cannot tell an absent key from a zero, so the key is always
/// present — the same rule `resolved` and `non_blocking` already follow.
#[test]
fn json_always_carries_the_count_even_when_it_is_zero() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "scripted", "feat-json");
assert_eq!(
json_count(&repo, &id),
0,
"a patch with no feedback must report 0, not omit the key"
);
let comment = inline_comment(&repo, &id, "one thing");
assert_eq!(json_count(&repo, &id), 1);
repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
assert_eq!(json_count(&repo, &id), 0);
}
/// `--json` grew a field; it did not lose one. The ids stay full.
#[test]
fn json_still_reports_full_ids_beside_the_new_field() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "ids intact", "feat-ids");
inline_comment(&repo, &id, "something");
let json = repo.run_ok(&["patch", "list", "--json", "-a", "--archived"]);
let value: serde_json::Value = serde_json::from_str(&json).expect("valid JSON");
let entry = &value.as_array().expect("array")[0];
let full = entry["id"].as_str().expect("an id");
assert_eq!(
full.len(),
40,
"`--json` must carry the full patch id, not the abbreviation the \
prose surface prints: {}",
json
);
assert!(
full.starts_with(&id),
"the id in `--json` is not the patch that was created: {}",
json
);
assert!(
entry["inline_comments"][0]["commit_id"]
.as_str()
.is_some_and(|s| s.len() == 40),
"inline comment ids must stay full: {}",
json
);
}
// ===========================================================================
// The two decisions a count forces
// ===========================================================================
/// **A stale claim counts as answered.**
///
/// `patch show` prints a resolution older than the tip as
/// `resolved by Alice at r1 — 2 revisions landed since`: a hint beside the
/// thread, deliberately never a state change. A count cannot carry that
/// caveat, so it has to pick a side, and counting the claim as *unanswered*
/// would make in every list view precisely the state change `28fc6751`
/// refused to make.
///
/// It would also be a number nobody could act on. Staleness is not a property
/// of the thread but of the pair (claim revision, current tip), so a revision
/// landing on an unrelated file would silently re-raise feedback that nobody
/// touched — and the only way to drive the count back down would be to
/// re-resolve every thread on every revision. That converts "unanswered
/// feedback" into "feedback not re-confirmed against the tip", which is a
/// different and much noisier question than the one a review queue asks.
///
/// So the count counts recorded facts, and the caveat stays where it can be
/// spelled out. This test pins both halves: the count says answered, and
/// `patch show` still says how old the claim is.
#[test]
fn a_claim_older_than_the_tip_still_counts_as_answered() {
let repo = TestRepo::new("Alice", "alice@example.com");
repo.git(&["checkout", "-b", "feat-stale"]);
repo.commit_file("feature.txt", "v1\n", "feature v1");
let out = repo.run_ok(&["patch", "create", "-t", "moving target", "-B", "feat-stale"]);
let id = out
.trim()
.strip_prefix("Created patch ")
.unwrap()
.to_string();
let comment = inline_comment(&repo, &id, "fix this");
repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
// Two more revisions land after the claim was made.
for v in ["v2", "v3"] {
repo.commit_file("feature.txt", &format!("{}\n", v), &format!("rev {}", v));
repo.run_ok(&["patch", "revise", &id]);
}
assert_eq!(
json_count(&repo, &id),
0,
"a revision landing elsewhere must not silently re-raise a thread \
nobody reopened"
);
let row = list_row(&repo, &id);
assert!(
!row.contains("unresolved"),
"the list disagrees with the recorded resolution:\n{}",
row
);
// And the caveat the count cannot carry is still where it can be read.
let show = repo.run_ok(&["patch", "show", &id]);
assert!(
show.contains("r1") && show.to_lowercase().contains("since"),
"the staleness hint must survive on the surface that has room for it:\n{}",
show
);
}
/// **A withdrawn comment is not outstanding feedback.**
///
/// Deleting an inline comment leaves a tombstone: the words are gone and there
/// is nothing left to answer. Counting it would send a reviewer to a patch to
/// read `[deleted]`, and the only way to clear the row would be to "resolve" a
/// comment that no longer says anything. `patch show` still prints the
/// tombstone, so a reader who opens the patch sees at once why the number is
/// what it is.
#[test]
fn a_withdrawn_comment_is_not_outstanding_feedback() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "withdrawn", "feat-tombstone");
let comment = inline_comment(&repo, &id, "actually never mind");
assert_eq!(json_count(&repo, &id), 1);
repo.run_ok(&["patch", "delete-comment", &id, &comment[..8]]);
assert_eq!(
json_count(&repo, &id),
0,
"a tombstone carries no words to answer, so it is not outstanding"
);
}
/// A reopened thread is outstanding again — the count follows the fold, not
/// the first event it saw.
#[test]
fn reopening_a_thread_puts_it_back_in_the_count() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "reopened", "feat-reopen");
let comment = inline_comment(&repo, &id, "not actually fixed");
repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
assert_eq!(json_count(&repo, &id), 0);
repo.run_ok(&["patch", "unresolve", &id, &comment[..8]]);
assert_eq!(
json_count(&repo, &id),
1,
"withdrawing a resolution must put the thread back in the count"
);
}
// ===========================================================================
// Filtering
// ===========================================================================
#[test]
fn patch_list_can_narrow_to_patches_with_unanswered_feedback() {
let repo = TestRepo::new("Alice", "alice@example.com");
patch_over_a_file(&repo, "nothing outstanding", "feat-a");
let noisy = patch_over_a_file(&repo, "needs attention", "feat-b");
inline_comment(&repo, &noisy, "look at this");
let listing = repo.run_ok(&["patch", "list", "--unresolved"]);
assert!(
listing.contains("needs attention"),
"the filter dropped a patch with unanswered feedback:\n{}",
listing
);
assert!(
!listing.contains("nothing outstanding"),
"the filter kept a patch with nothing outstanding:\n{}",
listing
);
}
/// The filter is a flag, not a fourth value of the status filter, so it
/// composes with the axis that already exists rather than replacing it.
#[test]
fn the_unresolved_filter_composes_with_the_status_filter() {
let repo = TestRepo::new("Alice", "alice@example.com");
let open = patch_over_a_file(&repo, "open with feedback", "feat-open");
inline_comment(&repo, &open, "a thing");
let closed = patch_over_a_file(&repo, "closed with feedback", "feat-closed");
inline_comment(&repo, &closed, "another thing");
repo.run_ok(&["patch", "close", &closed]);
let default = repo.run_ok(&["patch", "list", "--unresolved"]);
assert!(default.contains("open with feedback"));
assert!(
!default.contains("closed with feedback"),
"`--unresolved` alone must still mean open patches only:\n{}",
default
);
let all = repo.run_ok(&["patch", "list", "--unresolved", "-a", "--archived"]);
assert!(
all.contains("closed with feedback"),
"`--unresolved -a` must widen to closed patches too:\n{}",
all
);
}
/// An empty result says which filter emptied it, rather than claiming the
/// repository has no patches — the same rule the web list already follows.
#[test]
fn an_empty_unresolved_list_names_the_filter() {
let repo = TestRepo::new("Alice", "alice@example.com");
patch_over_a_file(&repo, "nothing outstanding", "feat-empty");
let listing = repo.run_ok(&["patch", "list", "--unresolved"]);
assert!(
listing.to_lowercase().contains("unanswered")
|| listing.to_lowercase().contains("unresolved"),
"an empty `--unresolved` list must say what it was filtering for:\n{}",
listing
);
}
// ===========================================================================
// The web list agrees
// ===========================================================================
#[test]
fn the_web_patch_list_reports_the_same_count_as_the_cli() {
let harness = ServerHarness::new("unresolved-web-count");
let repo = harness.work_repo();
let id = patch_over_a_file(repo, "web counted", "feat-web");
inline_comment(repo, &id, "first thing");
inline_comment(repo, &id, "second thing");
let third = inline_comment(repo, &id, "third thing");
repo.run_ok(&["patch", "resolve", &id, &third[..8]]);
assert_eq!(json_count(repo, &id), 2, "sanity: the CLI count");
harness.push_head();
harness.push_collab_refs();
let body = harness
.get_ok(&format!("/{}/patches", harness.repo_name()))
.body;
assert!(
body.contains("<th>Unresolved</th>"),
"the web patch list has no Unresolved column:\n{body}"
);
assert!(
body.contains(">2<"),
"the web patch list does not report the count the CLI reports:\n{body}"
);
}
#[test]
fn the_web_patch_list_can_narrow_to_patches_with_unanswered_feedback() {
let harness = ServerHarness::new("unresolved-web-filter");
let repo = harness.work_repo();
patch_over_a_file(repo, "Nothing outstanding here", "feat-web-quiet");
let noisy = patch_over_a_file(repo, "Needs attention here", "feat-web-noisy");
inline_comment(repo, &noisy, "look at this");
harness.push_head();
harness.push_collab_refs();
let body = harness
.get_ok(&format!(
"/{}/patches?filter=unresolved",
harness.repo_name()
))
.body;
assert!(
body.contains("Needs attention here"),
"the web filter dropped a patch with unanswered feedback:\n{body}"
);
assert!(
!body.contains("Nothing outstanding here"),
"the web filter kept a patch with nothing outstanding:\n{body}"
);
assert!(
body.contains("patches?filter=unresolved"),
"the filter bar does not offer the unresolved filter:\n{body}"
);
}
// ===========================================================================
// Reading is not writing
// ===========================================================================
/// Counting is a fold over state already in hand. It must not append an event
/// or move a ref — not on the CLI, not through the server.
#[test]
fn counting_unanswered_feedback_writes_nothing() {
let harness = ServerHarness::new("unresolved-readonly");
let repo = harness.work_repo();
let id = patch_over_a_file(repo, "read only", "feat-readonly");
inline_comment(repo, &id, "a thing");
harness.push_head();
harness.push_collab_refs();
let snapshot = |r: &TestRepo| {
r.git(&[
"for-each-ref",
"--format=%(refname) %(objectname)",
"refs/collab",
])
};
let before = snapshot(repo);
repo.run_ok(&["patch", "list"]);
repo.run_ok(&["patch", "list", "--unresolved"]);
repo.run_ok(&["patch", "list", "--json", "-a", "--archived"]);
harness.get_ok(&format!("/{}/patches", harness.repo_name()));
harness.get_ok(&format!(
"/{}/patches?filter=unresolved",
harness.repo_name()
));
let after = snapshot(repo);
assert_eq!(
before, after,
"rendering a list must not append events or move refs"
);
}