crates/cli/git-ents/tests/review.rs
review.rshistorycomment on this file
| 1 | //! Integration coverage for `git ents review` against a real local |
| 2 | //! composition root (`roots.local`): reviewing a commit writes both refs |
| 3 | //! `model.review` requires — the entity ref and its retention pin |
| 4 | //! (`model.review-pin`), the pin's parents including the reviewed commit |
| 5 | //! and its tree the empty tree — a review's discussion thread surfaces |
| 6 | //! comments naming it as their context (`model.comment-context`), and |
| 7 | //! re-reviewing a descendant advances the same composite-keyed ref |
| 8 | //! fast-forward rather than minting a new one (`model.review-pin`). |
| 9 | |
| 10 | #![allow( |
| 11 | clippy::expect_used, |
| 12 | clippy::indexing_slicing, |
| 13 | reason = "integration test" |
| 14 | )] |
| 15 | |
| 16 | mod common; |
| 17 | |
| 18 | use std::path::Path; |
| 19 | use std::process::Command; |
| 20 | |
| 21 | use ents_forge::comment::NewComment; |
| 22 | use ents_forge::review::NewReview; |
| 23 | use ents_forge::review::{ReviewState, Verdict}; |
| 24 | use git_ents::commands::{comment, members, review}; |
| 25 | use git_ents::root::LocalRoot; |
| 26 | use gix_object::{CommitRef, Find, Write as _}; |
| 27 | use gix_ref_store::RefStoreRead as _; |
| 28 | |
| 29 | /// Seed `dir`'s working tree with `path` and commit it under a fixed test |
| 30 | /// identity, returning the new commit's id. |
| 31 | fn commit_file(dir: &Path, path: &str, contents: &str) -> gix_hash::ObjectId { |
| 32 | std::fs::write(dir.join(path), contents).expect("write"); |
| 33 | let status = Command::new("git") |
| 34 | .arg("-C") |
| 35 | .arg(dir) |
| 36 | .args(["add", "-A"]) |
| 37 | .status() |
| 38 | .expect("git add"); |
| 39 | assert!(status.success()); |
| 40 | let status = Command::new("git") |
| 41 | .arg("-C") |
| 42 | .arg(dir) |
| 43 | .args([ |
| 44 | "-c", |
| 45 | "user.name=test", |
| 46 | "-c", |
| 47 | "user.email=test@example.com", |
| 48 | "commit", |
| 49 | "-q", |
| 50 | "-m", |
| 51 | "seed", |
| 52 | ]) |
| 53 | .status() |
| 54 | .expect("git commit"); |
| 55 | assert!(status.success()); |
| 56 | let output = Command::new("git") |
| 57 | .arg("-C") |
| 58 | .arg(dir) |
| 59 | .args(["rev-parse", "HEAD"]) |
| 60 | .output() |
| 61 | .expect("rev-parse"); |
| 62 | let hex = String::from_utf8(output.stdout).expect("utf8"); |
| 63 | hex.trim().parse().expect("valid oid") |
| 64 | } |
| 65 | |
| 66 | /// `model.review`, `model.review-pin`: `git ents review new` writes both |
| 67 | /// the review's own entity ref (keyed `reviews/<target>/<member>`) and its |
| 68 | /// retention pin, and the pin's tip commit is a merge-shaped, empty-tree |
| 69 | /// commit whose parents include the reviewed commit — the reachability |
| 70 | /// edge `model.review-pin` requires. |
| 71 | // @relation(model.review, model.review-pin, meta-ref.identity-binding, roots.local, scope=function, role=Verifies) |
| 72 | #[test] |
| 73 | fn review_new_writes_both_refs_with_the_pin_parented_on_the_reviewed_commit() { |
| 74 | let fixture = common::Fixture::new(1); |
| 75 | let reviewed = commit_file(fixture.path(), "file.txt", "line one\n"); |
| 76 | let root = LocalRoot::open(fixture.path()).expect("opens"); |
| 77 | members::add(&root, "reviewer", None, Some(fixture.key_path.clone())).expect("enrolls"); |
| 78 | |
| 79 | let new = NewReview { |
| 80 | target: "HEAD".to_owned(), |
| 81 | verdict: Verdict::Approve, |
| 82 | body: "looks good".to_owned(), |
| 83 | }; |
| 84 | let target = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews"); |
| 85 | |
| 86 | // The entity ref exists and reads back verdict, body, and the |
| 87 | // reviewed commit as a plain data field — no pin read required. |
| 88 | let (found, _thread) = review::show(&root, &target, "reviewer").expect("shows"); |
| 89 | assert_eq!(found.verdict, Verdict::Approve); |
| 90 | assert_eq!(found.body, "looks good"); |
| 91 | assert_eq!(found.target(), reviewed); |
| 92 | |
| 93 | // The pin ref exists; its tip's parents include the reviewed commit, |
| 94 | // and its tree is the empty tree — the sole exception to |
| 95 | // `meta-ref.namespace`'s tree-is-the-entity shape. |
| 96 | let pin_ref = |
| 97 | ents_model::namespace::review_pin_ref(&target, &ents_model::MemberId::new("reviewer")) |
| 98 | .expect("valid"); |
| 99 | let pin_tip = root |
| 100 | .refs |
| 101 | .get(pin_ref.as_ref()) |
| 102 | .expect("reads") |
| 103 | .expect("pin ref exists"); |
| 104 | let mut buf = Vec::new(); |
| 105 | let data = root |
| 106 | .objects |
| 107 | .try_find(&pin_tip, &mut buf) |
| 108 | .expect("reads") |
| 109 | .expect("pin commit exists"); |
| 110 | let commit = CommitRef::from_bytes(data.data, pin_tip.kind()).expect("parses"); |
| 111 | assert!( |
| 112 | commit.parents().any(|parent| parent == reviewed), |
| 113 | "pin's parents must include the reviewed commit" |
| 114 | ); |
| 115 | let empty_tree = root |
| 116 | .objects |
| 117 | .write(&gix_object::Tree { entries: vec![] }) |
| 118 | .expect("writes empty tree"); |
| 119 | assert_eq!(commit.tree(), empty_tree); |
| 120 | } |
| 121 | |
| 122 | /// `model.comment-context`, `model.review`: a comment naming |
| 123 | /// `reviews/<target>/<member>` as its context surfaces in `git ents review |
| 124 | /// show`'s thread — the review itself stores no list of its comments. |
| 125 | // @relation(model.review, model.comment-context, roots.local, scope=function, role=Verifies) |
| 126 | #[test] |
| 127 | fn review_show_surfaces_a_context_comment() { |
| 128 | let fixture = common::Fixture::new(1); |
| 129 | commit_file(fixture.path(), "file.txt", "line one\n"); |
| 130 | let root = LocalRoot::open(fixture.path()).expect("opens"); |
| 131 | members::add(&root, "reviewer", None, Some(fixture.key_path.clone())).expect("enrolls"); |
| 132 | |
| 133 | let new = NewReview { |
| 134 | target: "HEAD".to_owned(), |
| 135 | verdict: Verdict::RequestChanges, |
| 136 | body: "one nit".to_owned(), |
| 137 | }; |
| 138 | let target = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews"); |
| 139 | |
| 140 | let draft = NewComment { |
| 141 | body: "please rename this".to_owned(), |
| 142 | path: None, |
| 143 | lines: None, |
| 144 | rev: "HEAD".to_owned(), |
| 145 | worktree: false, |
| 146 | context: Some(format!("reviews/{target}/reviewer")), |
| 147 | parent: None, |
| 148 | }; |
| 149 | comment::add(&root, draft, Some(fixture.key_path.clone())).expect("comments"); |
| 150 | |
| 151 | let (_review, thread) = review::show(&root, &target, "reviewer").expect("shows"); |
| 152 | assert_eq!(thread.len(), 1); |
| 153 | assert_eq!(thread[0].1.body, "please rename this"); |
| 154 | } |
| 155 | |
| 156 | /// `git ents review list [--target rev]`: filtering by target keeps only |
| 157 | /// reviews of that commit — exercised across two different reviewers |
| 158 | /// (rather than one reviewer reviewing two commits) since, per |
| 159 | /// `model.review-pin`, one member reviewing a descendant commit advances |
| 160 | /// their existing thread rather than opening a second one; two distinct |
| 161 | /// review entities need two distinct reviewers. |
| 162 | // @relation(model.review, roots.local, scope=function, role=Verifies) |
| 163 | #[test] |
| 164 | fn review_list_filters_by_target() { |
| 165 | let fixture = common::Fixture::new(1); |
| 166 | let other_key = fixture.path().join(".id_ed25519_bob"); |
| 167 | common::write_key(&other_key, 2); |
| 168 | let first = commit_file(fixture.path(), "file.txt", "line one\n"); |
| 169 | let second = commit_file(fixture.path(), "file.txt", "line one\nline two\n"); |
| 170 | let root = LocalRoot::open(fixture.path()).expect("opens"); |
| 171 | members::add(&root, "alice", None, Some(fixture.key_path.clone())).expect("enrolls alice"); |
| 172 | members::add(&root, "bob", None, Some(other_key.clone())).expect("enrolls bob"); |
| 173 | |
| 174 | let review_of_first = NewReview { |
| 175 | target: first.to_string(), |
| 176 | verdict: Verdict::Approve, |
| 177 | body: String::new(), |
| 178 | }; |
| 179 | let first_target = |
| 180 | review::new(&root, review_of_first, Some(fixture.key_path.clone())).expect("reviews"); |
| 181 | let review_of_second = NewReview { |
| 182 | target: second.to_string(), |
| 183 | verdict: Verdict::Approve, |
| 184 | body: String::new(), |
| 185 | }; |
| 186 | review::new(&root, review_of_second, Some(other_key)).expect("reviews"); |
| 187 | |
| 188 | let all = review::list(&root, None).expect("lists"); |
| 189 | assert_eq!(all.len(), 2); |
| 190 | |
| 191 | let filtered = review::list(&root, Some(first.to_string())).expect("lists"); |
| 192 | assert_eq!(filtered.len(), 1); |
| 193 | assert_eq!(filtered[0].0.0, first_target); |
| 194 | assert_eq!(filtered[0].0.1, ents_model::MemberId::new("alice")); |
| 195 | } |
| 196 | |
| 197 | /// `model.review-pin`: re-reviewing a descendant of a commit this member |
| 198 | /// already reviewed advances the *same* two refs fast-forward — the |
| 199 | /// composite key stays anchored at the original genesis target, and |
| 200 | /// [`ents_forge::review::Review::target`] moves to the newly reviewed |
| 201 | /// commit — rather than minting a second, unrelated review. |
| 202 | // @relation(model.review, model.review-pin, meta-ref.identity-binding, roots.local, scope=function, role=Verifies) |
| 203 | #[test] |
| 204 | fn re_reviewing_a_descendant_advances_the_same_ref_fast_forward() { |
| 205 | let fixture = common::Fixture::new(1); |
| 206 | let first = commit_file(fixture.path(), "file.txt", "line one\n"); |
| 207 | let second = commit_file(fixture.path(), "file.txt", "line one\nline two\n"); |
| 208 | let root = LocalRoot::open(fixture.path()).expect("opens"); |
| 209 | members::add(&root, "reviewer", None, Some(fixture.key_path.clone())).expect("enrolls"); |
| 210 | |
| 211 | let initial = NewReview { |
| 212 | target: first.to_string(), |
| 213 | verdict: Verdict::RequestChanges, |
| 214 | body: "please address this".to_owned(), |
| 215 | }; |
| 216 | let first_target = |
| 217 | review::new(&root, initial, Some(fixture.key_path.clone())).expect("reviews"); |
| 218 | assert_eq!(first_target, first.to_string()); |
| 219 | |
| 220 | // Re-review the descendant: the CLI's own signer/member resolution |
| 221 | // finds the existing "reviewer" review of `first`, an ancestor of |
| 222 | // `second`, and advances it in place. |
| 223 | let follow_up = NewReview { |
| 224 | target: second.to_string(), |
| 225 | verdict: Verdict::Approve, |
| 226 | body: "looks good now".to_owned(), |
| 227 | }; |
| 228 | let advanced_target = |
| 229 | review::new(&root, follow_up, Some(fixture.key_path.clone())).expect("re-reviews"); |
| 230 | |
| 231 | // The composite key's target segment is unchanged (still genesis-keyed |
| 232 | // at `first`), but the entity's own recorded target has moved to |
| 233 | // `second`, and there is still exactly one review by this reviewer. |
| 234 | assert_eq!(advanced_target, first_target); |
| 235 | let (review, _thread) = review::show(&root, &first_target, "reviewer").expect("shows"); |
| 236 | assert_eq!(review.target(), second); |
| 237 | assert_eq!(review.verdict, Verdict::Approve); |
| 238 | assert_eq!(review.body, "looks good now"); |
| 239 | |
| 240 | let all = review::list(&root, None).expect("lists"); |
| 241 | assert_eq!( |
| 242 | all.len(), |
| 243 | 1, |
| 244 | "re-review advances in place, not a second row" |
| 245 | ); |
| 246 | } |
| 247 | |
| 248 | /// `model.review`: `git ents review withdraw` writes a new `Withdrawn` |
| 249 | /// entity onto the reviewer's own existing ref, preserving the verdict and |
| 250 | /// body untouched — append-only, so the prior `Active` commit stays in the |
| 251 | /// ref's history rather than being replaced by a different shape. |
| 252 | // @relation(model.review, model.review-pin, roots.local, scope=function, role=Verifies) |
| 253 | #[test] |
| 254 | fn withdraw_preserves_verdict_and_body_and_flips_only_state() { |
| 255 | let fixture = common::Fixture::new(1); |
| 256 | let reviewed = commit_file(fixture.path(), "file.txt", "line one\n"); |
| 257 | let root = LocalRoot::open(fixture.path()).expect("opens"); |
| 258 | members::add(&root, "reviewer", None, Some(fixture.key_path.clone())).expect("enrolls"); |
| 259 | |
| 260 | let new = NewReview { |
| 261 | target: "HEAD".to_owned(), |
| 262 | verdict: Verdict::RequestChanges, |
| 263 | body: "please fix this".to_owned(), |
| 264 | }; |
| 265 | let target = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews"); |
| 266 | |
| 267 | let withdrawn_target = |
| 268 | review::withdraw(&root, reviewed.to_string(), Some(fixture.key_path.clone())) |
| 269 | .expect("withdraws"); |
| 270 | assert_eq!(withdrawn_target, target, "withdraw advances the same ref"); |
| 271 | |
| 272 | let (review, _thread) = review::show(&root, &target, "reviewer").expect("shows"); |
| 273 | assert_eq!(review.state, ReviewState::Withdrawn); |
| 274 | assert_eq!(review.verdict, Verdict::RequestChanges); |
| 275 | assert_eq!(review.body, "please fix this"); |
| 276 | assert_eq!(review.target(), reviewed); |
| 277 | |
| 278 | // The chain is the audit trail: the review still enumerates from |
| 279 | // `list` (only the web listings hide a withdrawn row), and there is |
| 280 | // still exactly one review row, not a second one. |
| 281 | let all = review::list(&root, None).expect("lists"); |
| 282 | assert_eq!(all.len(), 1); |
| 283 | assert_eq!(all[0].1.state, ReviewState::Withdrawn); |
| 284 | } |
| 285 | |
| 286 | /// `git ents review list --porcelain` emits the stable record grammar |
| 287 | /// (`lens.parity`, `model.review`): a head line of full target segment, |
| 288 | /// member, full reviewed oid, verdict, and state, then the body |
| 289 | /// tab-prefixed — and a withdrawal shows up as the state token. |
| 290 | // @relation(lens.porcelain, lens.parity, model.review, roots.local, scope=function, role=Verifies) |
| 291 | #[test] |
| 292 | fn review_list_porcelain_emits_full_id_records() { |
| 293 | let fixture = common::Fixture::new(6); |
| 294 | let reviewed = commit_file(fixture.path(), "file.txt", "line one\n"); |
| 295 | let root = LocalRoot::open(fixture.path()).expect("opens"); |
| 296 | members::add(&root, "reviewer", None, Some(fixture.key_path.clone())).expect("enrolls"); |
| 297 | |
| 298 | let new = NewReview { |
| 299 | target: "HEAD".to_owned(), |
| 300 | verdict: Verdict::Approve, |
| 301 | body: "looks good\n\nsecond paragraph".to_owned(), |
| 302 | }; |
| 303 | let target = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews"); |
| 304 | |
| 305 | let porcelain = |fixture: &common::Fixture| { |
| 306 | let output = Command::new(common::bin_path()) |
| 307 | .current_dir(fixture.path()) |
| 308 | .args(["review", "list", "--porcelain"]) |
| 309 | .output() |
| 310 | .expect("runs"); |
| 311 | assert!(output.status.success(), "{output:?}"); |
| 312 | String::from_utf8(output.stdout).expect("utf8") |
| 313 | }; |
| 314 | |
| 315 | let expected = format!( |
| 316 | "{target} reviewer {reviewed} approve active\n\tlooks good\n\t\n\tsecond paragraph\n" |
| 317 | ); |
| 318 | assert_eq!(porcelain(&fixture), expected); |
| 319 | |
| 320 | review::withdraw(&root, reviewed.to_string(), Some(fixture.key_path.clone())) |
| 321 | .expect("withdraws"); |
| 322 | let expected = format!( |
| 323 | "{target} reviewer {reviewed} approve withdrawn\n\tlooks good\n\t\n\tsecond paragraph\n" |
| 324 | ); |
| 325 | assert_eq!(porcelain(&fixture), expected); |
| 326 | } |
| 327 | |
| 328 | /// `git ents review new` with no `--body`, run as the real binary with a |
| 329 | /// fake `$EDITOR`: the body composes from the scratch file, `#` lines |
| 330 | /// stripped — the same editor fallback `issue new` has. |
| 331 | // @relation(model.review, roots.local, scope=function, role=Verifies) |
| 332 | #[test] |
| 333 | fn review_new_composes_body_from_a_fake_editor() { |
| 334 | let fixture = common::Fixture::new(7); |
| 335 | commit_file(fixture.path(), "file.txt", "line one\n"); |
| 336 | let root = LocalRoot::open(fixture.path()).expect("opens"); |
| 337 | members::add(&root, "reviewer", None, Some(fixture.key_path.clone())).expect("enrolls"); |
| 338 | |
| 339 | let editor_path = fixture.path().join("fake-editor.sh"); |
| 340 | common::write_fake_editor( |
| 341 | &editor_path, |
| 342 | "composed review body\n# a stray comment line", |
| 343 | ); |
| 344 | |
| 345 | let output = Command::new(common::bin_path()) |
| 346 | .current_dir(fixture.path()) |
| 347 | .args(["review", "new", "--verdict", "approve", "--key"]) |
| 348 | .arg(&fixture.key_path) |
| 349 | .env("GIT_EDITOR", &editor_path) |
| 350 | .env("EDITOR", &editor_path) |
| 351 | .output() |
| 352 | .expect("runs"); |
| 353 | assert!(output.status.success(), "{output:?}"); |
| 354 | |
| 355 | let all = review::list(&root, None).expect("lists"); |
| 356 | assert_eq!(all.len(), 1); |
| 357 | assert_eq!(all[0].1.body, "composed review body"); |
| 358 | } |
| 359 | |
| 360 | /// An empty composed review body aborts with a failing exit status, |
| 361 | /// mirroring `git commit`'s own empty-message abort. |
| 362 | // @relation(model.review, roots.local, scope=function, role=Verifies) |
| 363 | #[test] |
| 364 | fn review_new_aborts_on_an_empty_editor_body() { |
| 365 | let fixture = common::Fixture::new(8); |
| 366 | commit_file(fixture.path(), "file.txt", "line one\n"); |
| 367 | let root = LocalRoot::open(fixture.path()).expect("opens"); |
| 368 | members::add(&root, "reviewer", None, Some(fixture.key_path.clone())).expect("enrolls"); |
| 369 | |
| 370 | let editor_path = fixture.path().join("fake-editor.sh"); |
| 371 | common::write_fake_editor(&editor_path, "# only a comment, no body"); |
| 372 | |
| 373 | let output = Command::new(common::bin_path()) |
| 374 | .current_dir(fixture.path()) |
| 375 | .args(["review", "new", "--verdict", "approve", "--key"]) |
| 376 | .arg(&fixture.key_path) |
| 377 | .env("GIT_EDITOR", &editor_path) |
| 378 | .env("EDITOR", &editor_path) |
| 379 | .output() |
| 380 | .expect("runs"); |
| 381 | assert!( |
| 382 | !output.status.success(), |
| 383 | "an empty body must abort review creation: {output:?}" |
| 384 | ); |
| 385 | assert_eq!(review::list(&root, None).expect("lists").len(), 0); |
| 386 | } |
| 387 | |
| 388 | /// `model.review`: withdrawing when this member has never reviewed |
| 389 | /// `target` (or an ancestor of it) is a clear refusal — there is nothing to |
| 390 | /// withdraw. |
| 391 | // @relation(model.review, roots.local, scope=function, role=Verifies) |
| 392 | #[test] |
| 393 | fn withdraw_refuses_when_no_review_exists() { |
| 394 | let fixture = common::Fixture::new(1); |
| 395 | commit_file(fixture.path(), "file.txt", "line one\n"); |
| 396 | let root = LocalRoot::open(fixture.path()).expect("opens"); |
| 397 | members::add(&root, "reviewer", None, Some(fixture.key_path.clone())).expect("enrolls"); |
| 398 | |
| 399 | let err = review::withdraw(&root, "HEAD".to_owned(), Some(fixture.key_path.clone())) |
| 400 | .expect_err("nothing to withdraw"); |
| 401 | assert!( |
| 402 | matches!(err, git_ents::error::Error::Forge(_)), |
| 403 | "expected Error::Forge, got {err:?}" |
| 404 | ); |
| 405 | assert!( |
| 406 | err.to_string().contains("not found"), |
| 407 | "expected a NotFound refusal, got {err}" |
| 408 | ); |
| 409 | } |
| 410 | |
| 411 | /// `model.review`: withdrawing an already-withdrawn review is a |
| 412 | /// no-op-ish re-write, not an error — the same ref simply advances again |
| 413 | /// with the same `Withdrawn` state. |
| 414 | // @relation(model.review, roots.local, scope=function, role=Verifies) |
| 415 | #[test] |
| 416 | fn withdrawing_an_already_withdrawn_review_is_not_an_error() { |
| 417 | let fixture = common::Fixture::new(1); |
| 418 | commit_file(fixture.path(), "file.txt", "line one\n"); |
| 419 | let root = LocalRoot::open(fixture.path()).expect("opens"); |
| 420 | members::add(&root, "reviewer", None, Some(fixture.key_path.clone())).expect("enrolls"); |
| 421 | |
| 422 | let new = NewReview { |
| 423 | target: "HEAD".to_owned(), |
| 424 | verdict: Verdict::Approve, |
| 425 | body: "looks good".to_owned(), |
| 426 | }; |
| 427 | let target = review::new(&root, new, Some(fixture.key_path.clone())).expect("reviews"); |
| 428 | |
| 429 | review::withdraw(&root, "HEAD".to_owned(), Some(fixture.key_path.clone())) |
| 430 | .expect("withdraws"); |
| 431 | review::withdraw(&root, "HEAD".to_owned(), Some(fixture.key_path.clone())) |
| 432 | .expect("withdrawing again is not an error"); |
| 433 | |
| 434 | let (review, _thread) = review::show(&root, &target, "reviewer").expect("shows"); |
| 435 | assert_eq!(review.state, ReviewState::Withdrawn); |
| 436 | assert_eq!(review.verdict, Verdict::Approve); |
| 437 | } |