merge queue: checking #1842 on main (c50a1cc), stacked on #1851

crates/mergify-auth/Cargo.toml

@@ -25,7 +25,6 @@ url = { workspace = true }
 mergify-core = { path = "../mergify-core", features = ["test-support"] }
 mergify-test-support = { path = "../mergify-test-support" }
 serde_json = { workspace = true }
-temp-env = { workspace = true }
 tempfile = { workspace = true }
 tokio = { workspace = true, features = ["rt-multi-thread"] }
 wiremock = { workspace = true }

crates/mergify-auth/src/browser.rs

@@ -231,7 +231,7 @@ mod tests {
     #[cfg(target_os = "macos")]
     #[test]
     fn macos_opens_the_url_with_open() {
-        let command = temp_env::with_vars(
+        let command = mergify_core::env::testing::with_vars(
             [("SSH_CONNECTION", None::<&str>), ("SSH_TTY", None::<&str>)],
             || command_for("https://dashboard.mergify.com/device"),
         )
@@ -246,7 +246,7 @@ mod tests {
     #[cfg(all(unix, not(target_os = "macos")))]
     #[test]
     fn a_graphical_session_gets_xdg_open() {
-        let command = temp_env::with_vars(
+        let command = mergify_core::env::testing::with_vars(
             [("DISPLAY", Some(":0")), ("WAYLAND_DISPLAY", None::<&str>)],
             || command_for("https://dashboard.mergify.com/device"),
         )
@@ -263,7 +263,7 @@ mod tests {
     #[cfg(target_os = "macos")]
     #[test]
     fn an_ssh_session_to_a_mac_opens_nothing() {
-        let opened = temp_env::with_vars(
+        let opened = mergify_core::env::testing::with_vars(
             [
                 ("SSH_CONNECTION", Some("10.0.0.1 52000 10.0.0.2 22")),
                 ("SSH_TTY", None),
@@ -281,7 +281,7 @@ mod tests {
     #[cfg(all(unix, not(target_os = "macos")))]
     #[test]
     fn a_headless_session_opens_nothing() {
-        let opened = temp_env::with_vars(
+        let opened = mergify_core::env::testing::with_vars(
             [("DISPLAY", None::<&str>), ("WAYLAND_DISPLAY", None::<&str>)],
             || command_for("https://dashboard.mergify.com/device").is_ok(),
         );

crates/mergify-auth/src/lib.rs

@@ -41,17 +41,18 @@ mod testing {
     /// Run `body` to completion with `MERGIFY_TOKEN` forced to
     /// `value`.
     ///
-    /// `temp_env` cannot wrap an `.await`, so the future is driven
-    /// inside the closure instead. Without this the wiring that
-    /// reads the variable is untestable, and untestable wiring is
-    /// wiring a future edit can delete with the suite still green:
-    /// asserting on the renderer alone proves only that the renderer
-    /// can print a note, never that anything asks it to.
+    /// The overlay is installed on this thread and the future is
+    /// driven on it, by a `current_thread` runtime built here.
+    /// Without this the wiring that reads the variable is
+    /// untestable, and untestable wiring is wiring a future edit can
+    /// delete with the suite still green: asserting on the renderer
+    /// alone proves only that the renderer can print a note, never
+    /// that anything asks it to.
     pub fn with_mergify_token<F: std::future::Future>(value: Option<&str>, body: F) -> F::Output {
         let runtime = tokio::runtime::Builder::new_current_thread()
             .enable_all()
             .build()
             .unwrap();
-        temp_env::with_var("MERGIFY_TOKEN", value, || runtime.block_on(body))
+        mergify_core::env::testing::with_var("MERGIFY_TOKEN", value, || runtime.block_on(body))
     }
 }

crates/mergify-auth/src/machine.rs

@@ -111,7 +111,7 @@ mod tests {
     // the variable instead.
     #[test]
     fn the_variables_are_the_fallback() {
-        let from_windows = temp_env::with_vars(
+        let from_windows = mergify_core::env::testing::with_vars(
             [
                 ("COMPUTERNAME", Some("WIN-BOX")),
                 ("HOSTNAME", Some("ignored")),
@@ -120,13 +120,13 @@ mod tests {
         );
         assert_eq!(from_windows.as_deref(), Some("WIN-BOX"));
 
-        let from_shell = temp_env::with_vars(
+        let from_shell = mergify_core::env::testing::with_vars(
             [("COMPUTERNAME", None), ("HOSTNAME", Some("build-42"))],
             from_env,
         );
         assert_eq!(from_shell.as_deref(), Some("build-42"));
 
-        let from_nothing = temp_env::with_vars(
+        let from_nothing = mergify_core::env::testing::with_vars(
             [("COMPUTERNAME", None::<&str>), ("HOSTNAME", None)],
             from_env,
         );

crates/mergify-cli/tests/stack_push_github_native.rs

@@ -17,7 +17,10 @@
 //!   the first PR mutation and `POST /stacks` **after** the last one.
 //!   Getting that order wrong is what permanently closes a surviving
 //!   pull request (see `mergify_stack::native_stack`), and no unit
-//!   test on the module in isolation can catch it.
+//!   test on the module in isolation can catch it;
+//! - the two surfaces keyed off the registration's outcome — the
+//!   `Depends-On:` header and the sticky stack comment — must follow
+//!   the outcome and not the flag, in both directions.
 
 use std::path::{Path, PathBuf};
 use std::process::Command;
@@ -447,6 +450,221 @@ async fn depends_on_restored_when_native_registration_does_not_happen() {
     );
 }
 
+/// The `url` GitHub puts on an issue comment, as an absolute URL —
+/// which is what the CLI has to turn back into a path before it can
+/// call the API with it.
+fn comment_url(server: &MockServer, id: u64) -> String {
+    format!("{}/repos/myorg/myrepo/issues/comments/{id}", server.uri())
+}
+
+/// Every `METHOD /path` the server saw against the issue-comments
+/// endpoints, which is where both stack-comment surfaces live.
+async fn comment_requests(server: &MockServer) -> Vec<String> {
+    request_log(server)
+        .await
+        .into_iter()
+        .filter(|r| r.contains("/issues/") && r.contains("comments"))
+        .collect()
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn no_stack_comment_is_posted_when_native_registration_succeeds() {
+    // GitHub's own Stacks UI lists the members on the pull request
+    // page, so the sticky table is a second copy of what the reader is
+    // already looking at — the same argument as the `Depends-On:`
+    // header one surface up.
+    let (work, _) = build_stack_repo(2);
+    let local = work.path().join("local");
+    let server = mock_github_creating(&[101, 102]).await;
+    Mock::given(method("POST"))
+        .and(wm_path("/repos/myorg/myrepo/stacks"))
+        .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"number": 12})))
+        .mount(&server)
+        .await;
+
+    assert_success(&run_push(&local, &server.uri(), &[]));
+
+    let comments = comment_requests(&server).await;
+    assert!(
+        comments.iter().all(|r| r.starts_with("GET ")),
+        "a registered stack must have nothing written to its comments, got: {comments:#?}",
+    );
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn a_stack_comment_from_an_older_push_is_deleted_once_github_holds_the_stack() {
+    // The migration half. Every stack pushed before this behaviour
+    // shipped carries the table, and nothing refreshes it any more, so
+    // it would sit frozen at that push's membership under a live list
+    // that keeps moving. Take it down on the next push instead.
+    let (work, _) = build_stack_repo(2);
+    let local = work.path().join("local");
+    let server = mock_github_creating(&[101, 102]).await;
+    // Outranks `mock_github_creating`'s empty listing: wiremock breaks
+    // a tie between two matching mocks by mount order, and 1 beats the
+    // default 5 regardless of it.
+    Mock::given(method("GET"))
+        .and(path_regex(r"^/repos/myorg/myrepo/issues/\d+/comments$"))
+        .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([
+            {"url": comment_url(&server, 900), "body": "looks good to me"},
+            {
+                "url": comment_url(&server, 901),
+                "body": "This pull request is part of a [Mergify stack](https://docs.mergify.com/stacks/):\n| # | Pull Request | Link | |\n",
+            },
+        ])))
+        .with_priority(1)
+        .mount(&server)
+        .await;
+    // Only ours is deletable: a DELETE aimed at comment 900 would 404
+    // here and fail the push.
+    Mock::given(method("DELETE"))
+        .and(wm_path("/repos/myorg/myrepo/issues/comments/901"))
+        .respond_with(ResponseTemplate::new(204))
+        .expect(2)
+        .mount(&server)
+        .await;
+    Mock::given(method("POST"))
+        .and(wm_path("/repos/myorg/myrepo/stacks"))
+        .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"number": 12})))
+        .mount(&server)
+        .await;
+
+    let output = run_push(&local, &server.uri(), &[]);
+    assert_success(&output);
+
+    let comments = comment_requests(&server).await;
+    assert!(
+        comments.iter().all(|r| !r.starts_with("POST ")),
+        "the comment is taken down, never rewritten, got: {comments:#?}",
+    );
+    let out = String::from_utf8_lossy(&output.stdout) + String::from_utf8_lossy(&output.stderr);
+    assert!(
+        out.contains("removed 2 stack comments"),
+        "the cleanup should be stated, got: {out}",
+    );
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn a_registered_stack_down_to_one_live_pull_request_still_loses_its_comment() {
+    // The stale-comment case the member count would hide. A stack
+    // whose members merge down to one open pull request keeps its
+    // registration — `appendable_tail` finds nothing to add, so GitHub
+    // goes on holding stack #7 — and that last pull request is still
+    // carrying the table it was given while the stack had several
+    // members. Counting pull requests before deciding to look would
+    // skip exactly this one, leaving a frozen list under GitHub's live
+    // one for ever.
+    //
+    // The count still gates the fallback upsert, and that costs
+    // nothing here: GitHub rejects a stack below two members, so a
+    // genuinely one-change stack is never registered and never reaches
+    // the removal at all.
+    let (_work, local, change_ids, remote_head) = repo_with_pushed_branches(1, 1);
+    let server = MockServer::start().await;
+    Mock::given(method("GET"))
+        .and(wm_path("/search/issues"))
+        .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({
+            "items": [{"number": 101}],
+        })))
+        .mount(&server)
+        .await;
+    Mock::given(method("GET"))
+        .and(wm_path("/repos/myorg/myrepo/pulls/101"))
+        .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({
+            "number": 101,
+            "state": "open",
+            "merged_at": null,
+            "draft": false,
+            "title": "existing",
+            "body": "existing",
+            "head": {"ref": head_ref(&change_ids, 0), "sha": remote_head},
+            "base": {"ref": "main"},
+            "html_url": "https://github.com/myorg/myrepo/pull/101",
+            // What is left of a stack that was bigger: GitHub keeps
+            // the registration and infers the merged prefix itself.
+            "stack": {"id": 162_170, "number": 7, "position": 1, "size": 1},
+        })))
+        .mount(&server)
+        .await;
+    Mock::given(method("GET"))
+        .and(wm_path("/repos/myorg/myrepo/pulls/101/reviews"))
+        .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([])))
+        .mount(&server)
+        .await;
+    Mock::given(method("PATCH"))
+        .and(wm_path("/repos/myorg/myrepo/pulls/101"))
+        .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({})))
+        .mount(&server)
+        .await;
+    Mock::given(method("GET"))
+        .and(path_regex(r"^/repos/myorg/myrepo/issues/\d+/comments$"))
+        .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([{
+            "url": comment_url(&server, 901),
+            "body": "This pull request is part of a [Mergify stack](https://docs.mergify.com/stacks/):\n| # | Pull Request | Link | |\n",
+        }])))
+        .mount(&server)
+        .await;
+    Mock::given(method("DELETE"))
+        .and(wm_path("/repos/myorg/myrepo/issues/comments/901"))
+        .respond_with(ResponseTemplate::new(204))
+        .expect(1)
+        .mount(&server)
+        .await;
+
+    let output = run_push(&local, &server.uri(), &[]);
+    assert_success(&output);
+
+    let out = String::from_utf8_lossy(&output.stdout) + String::from_utf8_lossy(&output.stderr);
+    assert!(
+        out.contains("GitHub stack #7 unchanged"),
+        "the registration must survive the shrink, or this is not the \
+         case under test, got: {out}",
+    );
+    assert!(
+        out.contains("removed 1 stack comment"),
+        "the last member's stale table is taken down, got: {out}",
+    );
+    // A POST here would be the opposite failure: writing the table
+    // back onto a pull request GitHub is already listing.
+    let comments = comment_requests(&server).await;
+    assert!(
+        comments.iter().all(|r| !r.starts_with("POST ")),
+        "nothing is written back, got: {comments:#?}",
+    );
+}
+
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn the_stack_comment_stays_when_native_registration_does_not_happen() {
+    // The other direction, and the reason this is keyed off the
+    // outcome rather than the flag: a push that degraded has GitHub
+    // rendering nothing, so dropping the comment would leave the stack
+    // with no member list anywhere.
+    let (work, _) = build_stack_repo(2);
+    let local = work.path().join("local");
+    let server = mock_github_creating(&[101, 102]).await;
+    Mock::given(method("POST"))
+        .and(wm_path("/repos/myorg/myrepo/stacks"))
+        .respond_with(ResponseTemplate::new(404).set_body_string("Not Found"))
+        .mount(&server)
+        .await;
+
+    assert_success(&run_push(&local, &server.uri(), &[]));
+
+    let comments = comment_requests(&server).await;
+    assert_eq!(
+        comments
+            .iter()
+            .filter(|r| r.starts_with("POST ") && r.ends_with("/comments"))
+            .count(),
+        2,
+        "both pull requests keep the table, got: {comments:#?}",
+    );
+    assert!(
+        comments.iter().all(|r| !r.starts_with("DELETE ")),
+        "nothing is taken down when nothing replaced it, got: {comments:#?}",
+    );
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn by_default_the_stack_is_registered_after_every_pull_request_is_upserted() {
     let (work, _) = build_stack_repo(2);