diff --git a/docs/tools/forge.md b/docs/tools/forge.md index 3e2d8cae7..aa87d6980 100644 --- a/docs/tools/forge.md +++ b/docs/tools/forge.md @@ -82,7 +82,7 @@ hive-forge pr reviews 42 --approve # submit APPROVED review hive-forge pr reviews 42 --request-changes -m "msg" # submit REQUEST_CHANGES review hive-forge pr reviews 42 --comment -m "msg" # submit COMMENT review hive-forge pr assign-reviewer 42 argus # set argus as a requested reviewer on the PR -hive-forge pr assign-reviewer 42 argus --remove # withdraw the request; fails if argus is still listed afterwards (a reviewer who already reviewed stays listed) +hive-forge pr assign-reviewer 42 argus --remove # withdraw the request hive-forge pr assign-committer 42 damocles # add damocles to the PR's assignee list hive-forge pr diff 42 # unified diff (lockfile hunks collapsed by default) hive-forge pr diff 42 --full # include unfiltered lockfile hunks diff --git a/hive-forge/src/verbs/pr_assign_reviewer.rs b/hive-forge/src/verbs/pr_assign_reviewer.rs index a3a70eb6d..b36c8760c 100644 --- a/hive-forge/src/verbs/pr_assign_reviewer.rs +++ b/hive-forge/src/verbs/pr_assign_reviewer.rs @@ -46,27 +46,6 @@ pub fn run(client: &Client, args: Args) -> Result<()> { .api() .repo_delete_pull_review_requests(owner, name, idx, body) .send()?; - // The forge answers success even when there was no pending request - // to remove (the reviewer already submitted a review), so read the - // list back instead of claiming a withdrawal that did not happen. - let pull = client - .api() - .repo_get_pull_request(owner, name, idx) - .send()?; - let still_listed = pull - .requested_reviewers - .as_deref() - .unwrap_or_default() - .iter() - .any(|u| u.login.as_deref() == Some(args.user.as_str())); - if still_listed { - anyhow::bail!( - "{} is still listed as a requested reviewer on #{} after the withdraw call \ - (the forge keeps a reviewer who already reviewed in that list)", - args.user, - args.number - ); - } println!( "review request withdrawn: {} on #{}", args.user, args.number diff --git a/hive-forge/tests/pr_assign_reviewer.rs b/hive-forge/tests/pr_assign_reviewer.rs deleted file mode 100644 index 69d7ef6f7..000000000 --- a/hive-forge/tests/pr_assign_reviewer.rs +++ /dev/null @@ -1,117 +0,0 @@ -//! `pr assign-reviewer --remove` against a mock forge: the withdraw call -//! answers success either way, so the verb must read the requested list back -//! and refuse to claim a withdrawal the forge did not perform. - -use std::io::{BufRead as _, BufReader, Read as _, Write as _}; -use std::net::{TcpListener, TcpStream}; -use std::path::PathBuf; -use std::process::Command; - -/// A unique scratch `HYPERHIVE_STATE_DIR` holding a `forge-token` file, -/// same manual-tmpdir pattern as `pr_edit.rs`. -fn scratch_state_dir(tag: &str) -> PathBuf { - let ts = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .map_or(0, |d| d.as_nanos()); - let dir = std::env::temp_dir().join(format!("hive-forge-assign-reviewer-test-{tag}-{ts}")); - std::fs::create_dir_all(&dir).expect("create scratch state dir"); - std::fs::write(dir.join("forge-token"), "test-token\n").expect("seed forge-token"); - dir -} - -/// Answer one request: `DELETE` succeeds with an empty body, `GET` returns -/// PR #7 listing `requested` as its requested reviewers. -fn serve(mut stream: TcpStream, requested: &[&str]) { - let mut reader = BufReader::new(stream.try_clone().expect("clone stream")); - let mut request_line = String::new(); - reader - .read_line(&mut request_line) - .expect("read request line"); - let mut content_length = 0usize; - loop { - let mut line = String::new(); - if reader.read_line(&mut line).expect("read header") == 0 || line == "\r\n" { - break; - } - if let Some((name, value)) = line.split_once(':') - && name.eq_ignore_ascii_case("content-length") - { - content_length = value.trim().parse().expect("numeric content-length"); - } - } - let mut raw = vec![0u8; content_length]; - reader.read_exact(&mut raw).expect("read body"); - let response = if request_line.starts_with("DELETE") { - "HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n".to_owned() - } else { - let reviewers: Vec<_> = requested - .iter() - .map(|login| serde_json::json!({ "login": login })) - .collect(); - let body = serde_json::json!({ - "number": 7, - "title": "t", - "state": "open", - "requested_reviewers": reviewers, - }) - .to_string(); - format!( - "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", - body.len() - ) - }; - stream - .write_all(response.as_bytes()) - .expect("write response"); -} - -/// Run `hive-forge -r o/r pr assign-reviewer 7 argus --remove` against a -/// forge whose PR lists `requested` afterwards. Returns `(success, stdout, stderr)`. -fn remove_argus(tag: &str, requested: &'static [&'static str]) -> (bool, String, String) { - let listener = TcpListener::bind("127.0.0.1:0").expect("bind mock forge"); - let addr = listener.local_addr().expect("mock forge addr"); - std::thread::spawn(move || { - for stream in listener.incoming() { - serve(stream.expect("accept"), requested); - } - }); - let out = Command::new(env!("CARGO_BIN_EXE_hive-forge")) - .args([ - "-r", - "o/r", - "pr", - "assign-reviewer", - "7", - "argus", - "--remove", - ]) - .env("HIVE_FORGE_URL", format!("http://{addr}")) - .env("HYPERHIVE_STATE_DIR", scratch_state_dir(tag)) - .env_remove("HIVE_FORGE_TOKEN_FILE") - .env_remove("HIVE_LABEL") - .output() - .expect("run hive-forge"); - ( - out.status.success(), - String::from_utf8_lossy(&out.stdout).into_owned(), - String::from_utf8_lossy(&out.stderr).into_owned(), - ) -} - -#[test] -fn withdrawal_that_left_the_reviewer_listed_is_reported_as_failed() { - let (ok, stdout, stderr) = remove_argus("still-listed", &["argus"]); - assert!(!ok, "{stdout}"); - assert!(!stdout.contains("withdrawn"), "{stdout}"); - assert!(stderr.contains("still listed"), "{stderr}"); -} - -#[test] -fn withdrawal_that_removed_the_reviewer_is_reported_as_done() { - let (ok, stdout, stderr) = remove_argus("removed", &["review-classifier"]); - assert!(ok, "{stderr}"); - assert!( - stdout.contains("review request withdrawn: argus on #7"), - "{stdout}" - ); -}