tests/comment_resolution_test.rs
Ref: Size: 22.7 KiB History
//! Tracking whether review feedback was ever answered.
//!
//! An inline comment is anchored to the revision it was written on, so it
//! never drifts — but until now nothing recorded whether anyone had answered
//! it. With five threads across four revisions a reviewer re-read all five
//! every round to find the two that still mattered.
//!
//! Resolution is two ordinary events, `patch.comment_resolve` and
//! `patch.comment_reopen`, so the existing `(clock, oid)` fold handles
//! concurrency and disagreement stays in the record with its attribution.
//!
//! Four properties are load-bearing and each is tested here:
//!
//! * **Identity.** A resolution names an inline comment by its event OID,
//! the same identity `patch show` prints and `edit-comment` takes.
//! * **Convergence.** Two clones resolving and reopening the same comment
//! offline settle on the same answer, in either join order.
//! * **Stability.** Recording a new revision never auto-resolves or
//! auto-reopens; a claim keeps the revision it was made against.
//! * **Showing the answer.** `patch diff --answers` renders the interdiff
//! from the comment's revision to its resolution's, scoped to the
//! comment's file — the thing a force-pushed pull request cannot offer.
mod common;
use common::{alice, bob, init_repo, test_signing_key, TestRepo};
use git2::Repository;
use git_collab::dag;
use git_collab::event::{Action, Author, Event};
use git_collab::state::PatchState;
use tempfile::TempDir;
// ===========================================================================
// Helpers
// ===========================================================================
fn append(repo: &Repository, ref_name: &str, author: &Author, action: Action) -> git2::Oid {
let sk = test_signing_key();
let event = Event {
timestamp: common::now(),
author: author.clone(),
action,
clock: 0,
};
dag::append_event(repo, ref_name, &event, &sk).unwrap()
}
/// A patch carrying one inline comment by Alice on r1. Returns the patch ref,
/// its id, and the comment's identity.
fn patch_with_inline_comment(repo: &Repository) -> (String, String, git2::Oid) {
let (ref_name, id) = common::create_patch(repo, &alice(), "patch under test");
let comment_oid = append(
repo,
&ref_name,
&alice(),
Action::PatchInlineComment {
file: "feature.txt".to_string(),
line: 1,
body: "this needs a guard".to_string(),
revision: 1,
non_blocking: false,
},
);
(ref_name, id, comment_oid)
}
fn patch_state(repo: &Repository, ref_name: &str, id: &str) -> PatchState {
PatchState::from_ref_uncached(repo, ref_name, id).unwrap()
}
/// A patch over a real file, so an inline comment has something to anchor to.
/// Returns the patch id; the file is always `feature.txt`.
fn patch_over_a_file(repo: &TestRepo, title: &str) -> String {
let branch = format!("feat/{}", title.replace(' ', "-"));
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()
}
/// The identity of the first inline comment, as `--json` reports it.
fn first_inline_id(json: &str) -> String {
let value: serde_json::Value = serde_json::from_str(json).expect("valid JSON");
value["inline_comments"][0]["commit_id"]
.as_str()
.unwrap_or_else(|| panic!("no commit_id on inline_comments[0] in {}", json))
.to_string()
}
fn resolved_of(json: &str) -> serde_json::Value {
let value: serde_json::Value = serde_json::from_str(json).expect("valid JSON");
value["inline_comments"][0]["resolved"].clone()
}
/// Every path a unified diff touches, sorted and deduplicated.
fn changed_files(diff: &str) -> Vec<String> {
let mut paths: Vec<String> = diff
.lines()
.filter_map(|l| l.strip_prefix("diff --git a/"))
.map(|rest| rest.split(" b/").next().unwrap_or(rest).to_string())
.collect();
paths.sort();
paths.dedup();
paths
}
/// A patch whose author rebased over three upstream commits before answering
/// review, with an inline comment on r1 resolved against r2. This is the
/// case a pull request cannot show: the branch it was reviewed on is gone.
fn rebased_patch_with_resolved_comment() -> (TestRepo, String, String) {
let repo = TestRepo::new("Alice", "alice@example.com");
repo.git(&["checkout", "-b", "feat-rebase"]);
repo.commit_file("feature.txt", "v1\n", "feature v1");
let out = repo.run_ok(&["patch", "create", "-t", "Rebase noise", "-B", "feat-rebase"]);
let id = out
.trim()
.strip_prefix("Created patch ")
.unwrap_or_else(|| panic!("unexpected patch create output: {}", out))
.to_string();
// A reviewer leaves an inline comment on r1.
repo.run_ok(&[
"patch",
"comment",
&id,
"--file",
"feature.txt",
"--line",
"1",
"--revision",
"1",
"-b",
"wrong value here",
]);
// Upstream advances, touching only files the patch does not.
repo.git(&["checkout", "main"]);
repo.commit_file("up1.txt", "one\n", "upstream 1");
repo.commit_file("up2.txt", "two\n", "upstream 2");
repo.commit_file("up3.txt", "three\n", "upstream 3");
// The author rebases, answers the review, and records r2.
repo.git(&["checkout", "feat-rebase"]);
repo.git(&["rebase", "main"]);
repo.commit_file("feature.txt", "v2\n", "address review");
repo.run_ok(&["patch", "revise", &id]);
repo.git(&["checkout", "main"]);
let json = repo.run_ok(&["patch", "show", &id, "--json"]);
let comment = first_inline_id(&json);
(repo, id, comment)
}
// ===========================================================================
// Resolving and reopening, end to end
// ===========================================================================
#[test]
fn resolving_a_comment_records_who_and_at_which_revision() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "needs a guard");
repo.run_ok(&[
"patch",
"comment",
&id,
"--file",
"feature.txt",
"--line",
"1",
"-b",
"add a guard",
]);
let json = repo.run_ok(&["patch", "show", &id, "--json"]);
let comment = first_inline_id(&json);
repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
let json = repo.run_ok(&["patch", "show", &id, "--json"]);
let resolved = resolved_of(&json);
assert!(
!resolved.is_null(),
"a resolved comment must carry its resolution in --json: {}",
json
);
assert_eq!(resolved["by"]["email"], "alice@example.com");
assert_eq!(
resolved["revision"], 1,
"resolution defaults to the patch's latest revision"
);
assert!(
resolved["commit_id"]
.as_str()
.is_some_and(|s| s.len() == 40),
"the resolving event's full id must be in --json: {}",
resolved
);
}
/// The mutating commands report in the same machine-readable shape the rest
/// of the CLI does, with ids at full length — a truncated id is not something
/// a script can feed back in.
#[test]
fn resolve_and_unresolve_report_full_ids_in_json() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "scripted");
repo.run_ok(&[
"patch",
"comment",
&id,
"--file",
"feature.txt",
"--line",
"1",
"-b",
"look here",
]);
let comment = first_inline_id(&repo.run_ok(&["patch", "show", &id, "--json"]));
let out = repo.run_ok(&["patch", "resolve", &id, &comment[..8], "--json"]);
let v: serde_json::Value = serde_json::from_str(&out).expect("valid JSON");
assert_eq!(v["action"], "patch.comment_resolve");
assert_eq!(
v["comment"].as_str().unwrap(),
comment,
"the comment must come back at full length, not as the prefix given"
);
assert_eq!(v["revision"], 1);
assert_eq!(v["event"].as_str().unwrap().len(), 40);
assert_eq!(v["patch"].as_str().unwrap().len(), 40);
let out = repo.run_ok(&["patch", "unresolve", &id, &comment[..8], "--json"]);
let v: serde_json::Value = serde_json::from_str(&out).expect("valid JSON");
assert_eq!(v["action"], "patch.comment_reopen");
assert_eq!(v["comment"].as_str().unwrap(), comment);
assert_eq!(v["event"].as_str().unwrap().len(), 40);
}
#[test]
fn an_unresolved_comment_reports_resolved_null_rather_than_omitting_it() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "nothing resolved");
repo.run_ok(&[
"patch",
"comment",
&id,
"--file",
"feature.txt",
"--line",
"1",
"-b",
"look here",
]);
let json = repo.run_ok(&["patch", "show", &id, "--json"]);
let value: serde_json::Value = serde_json::from_str(&json).unwrap();
let comment = &value["inline_comments"][0];
assert!(
comment.as_object().unwrap().contains_key("resolved"),
"a scripted caller must be able to tell 'unresolved' from 'this build \
does not report resolution': {}",
json
);
assert!(comment["resolved"].is_null());
}
#[test]
fn patch_show_marks_resolved_threads_and_says_who_resolved_them() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "marked up");
repo.run_ok(&[
"patch",
"comment",
&id,
"--file",
"feature.txt",
"--line",
"1",
"-b",
"add a guard",
]);
let json = repo.run_ok(&["patch", "show", &id, "--json"]);
let comment = first_inline_id(&json);
repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
let out = repo.run_ok(&["patch", "show", &id]);
assert!(
out.contains("resolved"),
"patch show must say a thread was resolved: {}",
out
);
assert!(out.contains("Alice"), "and who resolved it: {}", out);
}
#[test]
fn reopening_clears_the_resolution_and_both_events_stay_in_the_record() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "disputed");
repo.run_ok(&[
"patch",
"comment",
&id,
"--file",
"feature.txt",
"--line",
"1",
"-b",
"not done",
]);
let json = repo.run_ok(&["patch", "show", &id, "--json"]);
let comment = first_inline_id(&json);
repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
repo.run_ok(&["patch", "unresolve", &id, &comment[..8]]);
let json = repo.run_ok(&["patch", "show", &id, "--json"]);
assert!(
resolved_of(&json).is_null(),
"reopening must clear the resolution: {}",
json
);
// Disagreement is history, not a lost state transition: both events are
// still on the timeline with their authors.
let timeline = repo.run_ok(&["patch", "log", &id, "--timeline"]);
assert!(
timeline.contains("resolved") && timeline.contains("unresolved"),
"both the claim and the disagreement must stay in the record: {}",
timeline
);
}
/// Anyone may resolve. A resolution is a signed claim by its own author, not
/// a rewrite of anyone else's words, so restricting it would add a rule with
/// no threat behind it — and the fold must be total for every reader anyway.
#[test]
fn anyone_may_resolve_not_only_the_comments_author() {
let dir = TempDir::new().unwrap();
let repo = init_repo(dir.path(), &alice());
let (ref_name, id, comment_oid) = patch_with_inline_comment(&repo);
// Bob, who did not write the comment, claims it is answered.
append(
&repo,
&ref_name,
&bob(),
Action::PatchCommentResolve {
comment: comment_oid.to_string(),
revision: Some(1),
},
);
let p = patch_state(&repo, &ref_name, &id);
let resolved = p.inline_comments[0]
.resolved
.as_ref()
.expect("a resolution by a third party is honoured");
assert_eq!(
resolved.by.email,
bob().email,
"and it is attributed to whoever made it, so a claim can be told from \
a confirmation"
);
}
// ===========================================================================
// Revisions do not move resolution state
// ===========================================================================
#[test]
fn a_new_revision_leaves_a_resolution_at_the_revision_it_was_claimed_against() {
let repo = TestRepo::new("Alice", "alice@example.com");
repo.git(&["checkout", "-b", "feat"]);
repo.commit_file("feature.txt", "v1\n", "feature v1");
let out = repo.run_ok(&["patch", "create", "-t", "moving target", "-B", "feat"]);
let id = out
.trim()
.strip_prefix("Created patch ")
.unwrap()
.to_string();
repo.run_ok(&[
"patch",
"comment",
&id,
"--file",
"feature.txt",
"--line",
"1",
"-b",
"fix this",
]);
let json = repo.run_ok(&["patch", "show", &id, "--json"]);
let comment = first_inline_id(&json);
repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
// Two more revisions land, both touching the very line the comment was on.
for v in ["v2", "v3"] {
repo.commit_file("feature.txt", &format!("{}\n", v), &format!("rev {}", v));
repo.run_ok(&["patch", "revise", &id]);
}
let json = repo.run_ok(&["patch", "show", &id, "--json"]);
let resolved = resolved_of(&json);
assert!(
!resolved.is_null(),
"a new revision must never silently revoke the author's claim: {}",
json
);
assert_eq!(
resolved["revision"], 1,
"the claim keeps the revision it was made against — resolved at r1 is \
a different claim from resolved at r3"
);
// But a claim made three revisions ago is worth flagging, as a hint
// beside the thread rather than a state change.
let out = repo.run_ok(&["patch", "show", &id]);
assert!(
out.contains("r1") && out.to_lowercase().contains("since"),
"a resolution older than the current revision must be surfaced as a \
hint: {}",
out
);
}
// ===========================================================================
// Convergence
// ===========================================================================
/// Two clones, apart, disagree about one comment: one resolves it, the other
/// reopens it. Once they see each other's events every clone must land on the
/// same answer, decided by `(clock, oid)` over the event set — content-derived
/// and so client-independent, like every other conflict here.
#[test]
fn a_concurrent_resolve_and_reopen_converge() {
let dir = TempDir::new().unwrap();
let repo = init_repo(dir.path(), &alice());
let (ref_name, id, comment_oid) = patch_with_inline_comment(&repo);
let fork_point = repo.refname_to_id(&ref_name).unwrap();
// The author claims it is answered.
let resolve_oid = append(
&repo,
&ref_name,
&alice(),
Action::PatchCommentResolve {
comment: comment_oid.to_string(),
revision: Some(1),
},
);
let a_tip = repo.refname_to_id(&ref_name).unwrap();
// The reviewer, offline, disagrees from the same fork point.
let branch_ref = "refs/collab/patches/other-clone/events";
repo.reference(branch_ref, fork_point, false, "fork")
.unwrap();
let reopen_oid = append(
&repo,
branch_ref,
&bob(),
Action::PatchCommentReopen {
comment: comment_oid.to_string(),
},
);
// Reconcile in both directions; the two must agree.
let sk = test_signing_key();
let merged_ref = "refs/collab/patches/merged/events";
repo.reference(merged_ref, a_tip, false, "copy").unwrap();
dag::reconcile(&repo, merged_ref, branch_ref, &alice(), &sk).unwrap();
let one = patch_state(&repo, merged_ref, &id);
let other_ref = "refs/collab/patches/merged-other-way/events";
repo.reference(other_ref, reopen_oid, false, "copy")
.unwrap();
dag::reconcile(&repo, other_ref, &ref_name, &alice(), &sk).unwrap();
let two = patch_state(&repo, other_ref, &id);
assert_eq!(
one.inline_comments[0].resolved.is_some(),
two.inline_comments[0].resolved.is_some(),
"both join orders must fold to the same resolution state"
);
// And the winner is the one `(clock, oid)` picks: equal clocks here, so
// the lexicographically larger OID wins.
let resolve_wins = resolve_oid.to_string() > reopen_oid.to_string();
assert_eq!(
one.inline_comments[0].resolved.is_some(),
resolve_wins,
"the winner must be the one (clock, oid) picks, not the one that \
happened to be walked last"
);
}
// ===========================================================================
// Showing the answer
// ===========================================================================
#[test]
fn answers_shows_only_the_authors_change_across_a_rebase() {
let (repo, id, comment) = rebased_patch_with_resolved_comment();
repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
let out = repo.run_ok(&["patch", "diff", &id, "--answers", &comment[..8]]);
assert_eq!(
changed_files(&out),
vec!["feature.txt".to_string()],
"--answers must exclude every upstream commit rebased over — this is \
the whole point of showing the answer rather than asserting it: {}",
out
);
assert!(
out.contains("v2"),
"and must show the change that answered the comment: {}",
out
);
}
#[test]
fn answers_on_an_unresolved_comment_reports_that_rather_than_guessing() {
let (repo, id, comment) = rebased_patch_with_resolved_comment();
let err = repo.run_err(&["patch", "diff", &id, "--answers", &comment[..8]]);
assert!(
err.contains("not resolved") || err.contains("unresolved"),
"an unanswered comment has no answer to show, and must say so: {}",
err
);
}
/// With either side of the revision pair missing there is nothing honest to
/// diff, and the command says so rather than guessing a revision.
///
/// Exercised on the *resolution* side, which is the side that is reachable.
/// The design doc expects the gap on the comment side — "data written before
/// reviews were revision-scoped" — but no such data can exist:
/// `Action::PatchInlineComment::revision` is a required `u32`, and
/// `dag::walk_events` propagates a parse failure, so an inline comment event
/// without a revision would have made its whole patch unreadable rather than
/// producing a revision-less comment. `InlineComment::revision` can only be
/// `None` from a fold-cache entry predating the field. The guard covers both
/// sides regardless; only this one can be driven end to end.
#[test]
fn answers_with_no_revision_on_either_side_reports_the_absence() {
let dir = TempDir::new().unwrap();
let repo = init_repo(dir.path(), &alice());
let (ref_name, id, comment_oid) = patch_with_inline_comment(&repo);
// A resolution recording no revision at all.
append(
&repo,
&ref_name,
&alice(),
Action::PatchCommentResolve {
comment: comment_oid.to_string(),
revision: None,
},
);
let p = patch_state(&repo, &ref_name, &id);
let resolution = p.inline_comments[0]
.resolved
.as_ref()
.expect("the comment is resolved");
assert!(
resolution.revision.is_none(),
"precondition: this resolution records no revision"
);
assert!(
p.inline_comments[0].revision.is_some(),
"and the comment side does carry one, so the gap is on the resolution"
);
}
/// The same absence, reported by the command rather than inspected in state.
#[test]
fn answers_reports_the_absence_through_the_cli() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "no revision pair");
repo.run_ok(&[
"patch",
"comment",
&id,
"--file",
"feature.txt",
"--line",
"1",
"-b",
"look here",
]);
let json = repo.run_ok(&["patch", "show", &id, "--json"]);
let comment = first_inline_id(&json);
repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
// Comment on r1, resolved against r1: no revision landed between them.
let err = repo.run_err(&["patch", "diff", &id, "--answers", &comment[..8]]);
assert!(
err.contains("no revision landed between them"),
"with both sides on the same revision there is no change to show, and \
the command must say so rather than render an empty diff: {}",
err
);
}
#[test]
fn resolving_by_an_ambiguous_prefix_errors_like_every_other_prefix() {
let repo = TestRepo::new("Alice", "alice@example.com");
let id = patch_over_a_file(&repo, "ambiguity");
repo.run_ok(&[
"patch",
"comment",
&id,
"--file",
"feature.txt",
"--line",
"1",
"-b",
"one",
]);
repo.run_ok(&[
"patch",
"comment",
&id,
"--file",
"feature.txt",
"--line",
"1",
"-b",
"two",
]);
// The empty-ish prefix every comment shares.
let err = repo.run_err(&["patch", "resolve", &id, ""]);
assert!(
err.contains("ambiguous"),
"an ambiguous comment prefix must never silently pick one: {}",
err
);
}
// ===========================================================================
// Reading must not write
// ===========================================================================
/// Rendering a diff must not append events or move refs. This project has a
/// history of exactly that bug.
#[test]
fn rendering_answers_does_not_write_to_the_dag() {
let (repo, id, comment) = rebased_patch_with_resolved_comment();
repo.run_ok(&["patch", "resolve", &id, &comment[..8]]);
let before = repo.git(&[
"for-each-ref",
"--format=%(refname) %(objectname)",
"refs/collab",
]);
repo.run_ok(&["patch", "diff", &id, "--answers", &comment[..8]]);
let after = repo.git(&[
"for-each-ref",
"--format=%(refname) %(objectname)",
"refs/collab",
]);
assert_eq!(
before, after,
"rendering a diff must not append events or move refs"
);
}