Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions src/cli/commands/pr/merge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,11 @@ pub async fn run_pr_merge(

let mut prs_to_merge: Vec<PRToMerge> = Vec::new();
let mut json_skipped: Vec<String> = Vec::new();
// A repo whose PR lookup failed belongs in neither of the two collections
// above: it is not a merge candidate and it was not skipped. Without a
// third one, "we looked and found nothing" and "we could not look" arrive
// at the summary as the same state.
let mut lookup_failures: Vec<String> = Vec::new();

for repo in &all_repos {
if !path_exists(&repo.absolute_path) {
Expand Down Expand Up @@ -426,11 +431,24 @@ pub async fn run_pr_merge(
if !opts.json {
Output::error(&format!("{}: {}", repo.name, e));
}
lookup_failures.push(repo.name.clone());
}
}
}

if prs_to_merge.is_empty() {
// An empty candidate list has two causes that read identically here.
// Only one of them is an absence of PRs; the other is an absence of
// knowledge, and reporting it as the first is a false statement the
// exit code then endorses.
if !lookup_failures.is_empty() {
anyhow::bail!(
"could not determine PR state for {} of {} repositories: {}",
lookup_failures.len(),
all_repos.len(),
lookup_failures.join(", ")
);
}
println!("No open PRs found for any repository.");
println!("Repositories checked: {}", all_repos.len());
return Ok(());
Expand Down Expand Up @@ -730,6 +748,18 @@ pub async fn run_pr_merge(
));
}

// Case 3: any per-repo failure makes the run a failure, including a
// mixed run. The warning above is read by a human; the exit code is
// the only part a script sees, and it reported this as done.
if error_count > 0 || !lookup_failures.is_empty() {
anyhow::bail!(
"{} of {} auto-merge attempts failed{}",
error_count,
success_count + error_count,
describe_lookup_failures(&lookup_failures)
);
}

return Ok(());
}

Expand Down Expand Up @@ -1112,9 +1142,35 @@ pub async fn run_pr_merge(
}
}

// Case 3, on the path that matters most: a run that failed to merge some
// of the PRs it selected has already emitted its JSON document and its
// human summary by this point. Both are truthful. The exit code was not.
if error_count > 0 || !lookup_failures.is_empty() {
anyhow::bail!(
"{} of {} PR merges failed{}",
error_count,
success_count + error_count,
describe_lookup_failures(&lookup_failures)
);
}

Ok(())
}

/// Render the lookup-failure tail of a summary, or nothing when every repo
/// was successfully inspected. Kept separate so the three case-3 exits phrase
/// the same fact identically.
fn describe_lookup_failures(failures: &[String]) -> String {
if failures.is_empty() {
String::new()
} else {
format!(
"; PR state could not be determined for {}",
failures.join(", ")
)
}
}

/// Check if a repo has changes ahead of its default branch
/// Returns Ok(true) if there are changes, Ok(false) if no changes or on default branch
fn check_repo_for_changes(repo: &RepoInfo) -> anyhow::Result<bool> {
Expand Down
100 changes: 99 additions & 1 deletion tests/common/mock_platform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@
//! testing of platform adapter methods.

use serde_json::{json, Map, Value};
use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::Arc;
use wiremock::matchers::{header, method, path};
use wiremock::{Mock, MockServer, ResponseTemplate};
use wiremock::{Mock, MockServer, Request, Respond, ResponseTemplate};

/// Start a wiremock server and configure GITHUB_TOKEN env var.
/// Returns the server and a GitHubAdapter pointed at it.
Expand Down Expand Up @@ -376,6 +378,76 @@ pub async fn mock_merge_pr(server: &MockServer, number: u64, merged: bool) {
.await;
}

/// GitHub API: a PR whose GET response reflects whether its merge PUT has fired.
///
/// `mock_get_pr` mounts one invariant response, so a GET issued *after* a
/// successful merge PUT still reports `merged: false`. That is a state the real
/// API cannot produce, and any command that verifies its own merge by reading
/// the PR back sees a contradiction that belongs to the fixture rather than to
/// the code under test. This helper couples the two endpoints through shared
/// state so the sequence GET -> PUT -> GET behaves as the live API does.
///
/// Returns the merged flag so a test can assert the PUT actually fired. That
/// matters: without it, a command that never attempted the merge and a command
/// that merged successfully both leave the fixture reporting `merged: false`
/// for different reasons.
pub async fn mock_pr_lifecycle(server: &MockServer, number: u64) -> Arc<AtomicBool> {
let merged = Arc::new(AtomicBool::new(false));

struct ReadPr {
merged: Arc<AtomicBool>,
number: u64,
}

impl Respond for ReadPr {
fn respond(&self, _request: &Request) -> ResponseTemplate {
let is_merged = self.merged.load(Ordering::SeqCst);
ResponseTemplate::new(200).set_body_json(github_pr_json(
self.number,
if is_merged { "closed" } else { "open" },
"feat/test",
"main",
is_merged,
"PR description\n<!-- gitgrip-linked-prs\nfrontend:42\n-->",
))
}
}

struct MergePr {
merged: Arc<AtomicBool>,
}

impl Respond for MergePr {
fn respond(&self, _request: &Request) -> ResponseTemplate {
self.merged.store(true, Ordering::SeqCst);
ResponseTemplate::new(200).set_body_json(json!({
"sha": "merge123",
"merged": true,
"message": "Pull Request successfully merged"
}))
}
}

Mock::given(method("GET"))
.and(path(format!("/repos/owner/repo/pulls/{}", number)))
.respond_with(ReadPr {
merged: Arc::clone(&merged),
number,
})
.mount(server)
.await;

Mock::given(method("PUT"))
.and(path(format!("/repos/owner/repo/pulls/{}/merge", number)))
.respond_with(MergePr {
merged: Arc::clone(&merged),
})
.mount(server)
.await;

merged
}

/// GitHub API: merge PR returns 405 with "branch behind" message.
pub async fn mock_merge_pr_behind(server: &MockServer, number: u64) {
let body = json!({
Expand Down Expand Up @@ -666,6 +738,32 @@ pub fn point_repo_at_mock(
}

/// Mock a GitHub repo info response (GET /repos/:owner/:repo).
/// GitHub API: repo info with explicit merge-method permissions.
///
/// `mock_repo_info` always reports every method as allowed, so no test could
/// reach the branch where a command refuses a method the repository forbids.
pub async fn mock_repo_info_with_methods(
server: &MockServer,
owner: &str,
repo: &str,
allow_squash: bool,
allow_merge_commit: bool,
allow_rebase: bool,
) {
let mut body = github_repo_json(owner, repo);
if let Value::Object(ref mut m) = body {
m.insert("allow_squash_merge".into(), json!(allow_squash));
m.insert("allow_merge_commit".into(), json!(allow_merge_commit));
m.insert("allow_rebase_merge".into(), json!(allow_rebase));
}

Mock::given(method("GET"))
.and(path(format!("/repos/{}/{}", owner, repo)))
.respond_with(ResponseTemplate::new(200).set_body_json(body))
.mount(server)
.await;
}

pub async fn mock_repo_info(server: &MockServer, owner: &str, repo: &str) {
let body = github_repo_json(owner, repo);

Expand Down
Loading