diff --git a/src/bors/handlers/help.rs b/src/bors/handlers/help.rs index 21beb625..db32025a 100644 --- a/src/bors/handlers/help.rs +++ b/src/bors/handlers/help.rs @@ -85,7 +85,7 @@ mod tests { async fn help_command(pool: sqlx::PgPool) { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors help").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" You can use the following commands: ## PR management diff --git a/src/bors/handlers/info.rs b/src/bors/handlers/info.rs index ffa0dadc..697b82da 100644 --- a/src/bors/handlers/info.rs +++ b/src/bors/handlers/info.rs @@ -79,7 +79,7 @@ mod tests { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors info").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" ## Status of PR `1` - Not Approved @@ -100,7 +100,7 @@ mod tests { tester.post_comment("@bors info").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" ## Status of PR `1` - Approved by: `default-user` @@ -121,7 +121,7 @@ mod tests { tester.post_comment("@bors info").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" ## Status of PR `1` - Not Approved @@ -142,7 +142,7 @@ mod tests { tester.post_comment("@bors info").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" ## Status of PR `1` - Not Approved @@ -173,7 +173,7 @@ mod tests { tester.post_comment("@bors info").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" ## Status of PR `1` - Approved by: `default-user` diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index b806e320..4c6af35f 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -2,6 +2,7 @@ use std::sync::Arc; use super::mergeable_queue::MergeableQueueSender; use crate::bors::command::{BorsCommand, CommandParseError}; +use crate::bors::comment::CommentTag; use crate::bors::event::{BorsGlobalEvent, BorsRepositoryEvent, PullRequestComment}; use crate::bors::handlers::help::command_help; use crate::bors::handlers::info::command_info; @@ -22,6 +23,7 @@ use crate::bors::handlers::workflow::{handle_workflow_completed, handle_workflow use crate::bors::merge_queue::{AUTO_BRANCH_NAME, MergeQueueSender}; use crate::bors::{BorsContext, CommandPrefix, Comment, RepositoryState}; use crate::database::{DelegatedPermission, PullRequestModel}; +use crate::github::api::client::HideCommentReason; use crate::github::{GithubUser, LabelTrigger, PullRequest, PullRequestNumber}; use crate::permissions::PermissionType; use crate::{CommandParser, PgDbClient, TeamApiClient, load_repositories}; @@ -657,6 +659,24 @@ async fn unapprove_pr( handle_label_trigger(repo_state, pr.number, LabelTrigger::Unapproved).await } +/// Hide all previous "Try build started" comments on the given PR. +async fn hide_try_build_started_comments( + repo: &RepositoryState, + db: &PgDbClient, + pr: &PullRequestModel, +) -> anyhow::Result<()> { + let outdated = db + .get_tagged_bot_comments(repo.repository(), pr.number, CommentTag::TryBuildStarted) + .await?; + for comment in outdated { + repo.client + .hide_comment(&comment.node_id, HideCommentReason::Outdated) + .await?; + db.delete_tagged_bot_comment(&comment).await?; + } + Ok(()) +} + #[cfg(test)] mod tests { use crate::tests::{BorsTester, Comment, User, default_repo_name, run_test}; @@ -689,7 +709,7 @@ mod tests { async fn unknown_command(pool: sqlx::PgPool) { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment(Comment::from("@bors foo")).await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r#"Unknown command "foo". Run `@bors help` to see available commands."#); + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r#"Unknown command "foo". Run `@bors help` to see available commands."#); Ok(()) }) .await; diff --git a/src/bors/handlers/ping.rs b/src/bors/handlers/ping.rs index 06a6a6ff..431df8dc 100644 --- a/src/bors/handlers/ping.rs +++ b/src/bors/handlers/ping.rs @@ -22,7 +22,7 @@ mod tests { async fn ping_command(pool: sqlx::PgPool) { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors ping").await?; - assert_eq!(tester.get_comment(()).await?, "Pong 🏓!"); + assert_eq!(tester.get_comment_text(()).await?, "Pong 🏓!"); Ok(()) }) .await; diff --git a/src/bors/handlers/pr_events.rs b/src/bors/handlers/pr_events.rs index 3f3dc336..135f367d 100644 --- a/src/bors/handlers/pr_events.rs +++ b/src/bors/handlers/pr_events.rs @@ -317,13 +317,13 @@ mod tests { .await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" :warning: The base branch changed to `beta`, and the PR will need to be re-approved. " ); - tester.get_pr(()).await.expect_unapproved(); + tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) .await; @@ -337,7 +337,7 @@ mod tests { tester.edit_pr((), |_| {}).await?; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_approved_by(&User::default_pr_author().name); Ok(()) @@ -369,13 +369,13 @@ mod tests { tester.push_to_pr(()).await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" :warning: A new commit `pr-1-commit-1` was pushed to the branch, the PR will need to be re-approved. " ); - tester.get_pr(()).await.expect_unapproved(); + tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) .await; @@ -662,7 +662,7 @@ mod tests { tester.wait_for_pr((), |pr| pr.auto_build.is_some()).await?; tester.workflow_start(tester.auto_branch().await).await?; tester.push_to_pr(()).await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" :warning: A new commit `pr-1-commit-1` was pushed to the branch, the PR will need to be re-approved. @@ -690,7 +690,7 @@ mod tests { tester.workflow_start(tester.auto_branch().await).await?; tester.push_to_pr(()).await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" :warning: A new commit `pr-1-commit-1` was pushed to the branch, the PR will need to be re-approved. @@ -709,7 +709,7 @@ mod tests { tester.start_auto_build(()).await?; tester.workflow_start(tester.auto_branch().await).await?; - let prev_commit = &tester.get_pr(()).await.get_gh_pr().head_sha; + let prev_commit = &tester.get_pr_copy(()).await.get_gh_pr().head_sha; tester.push_to_pr(()).await?; tester.expect_comments((), 1).await; tester diff --git a/src/bors/handlers/refresh.rs b/src/bors/handlers/refresh.rs index f4500b4f..b4418cac 100644 --- a/src/bors/handlers/refresh.rs +++ b/src/bors/handlers/refresh.rs @@ -284,7 +284,7 @@ timeout = 3600 tester.cancel_timed_out_builds().await; }) .await; - insta::assert_snapshot!(tester.get_comment(()).await?, @":boom: Test timed out after `3600`s"); + insta::assert_snapshot!(tester.get_comment_text(()).await?, @":boom: Test timed out after `3600`s"); assert_eq!( tester .db() @@ -315,7 +315,7 @@ timeout = 3600 tester .expect_check_run( - &tester.get_pr(()).await.get_gh_pr().head_sha, + &tester.get_pr_copy(()).await.get_gh_pr().head_sha, TRY_BUILD_CHECK_RUN_NAME, "Bors try build", CheckRunStatus::Completed, @@ -381,7 +381,7 @@ timeout = 3600 .await?; tester.refresh_prs().await; tester - .get_pr(pr.number) + .get_pr_copy(pr.number) .await .expect_status(PullRequestStatus::Open); Ok(()) @@ -401,7 +401,7 @@ timeout = 3600 .await?; tester.refresh_prs().await; tester - .get_pr(pr.number) + .get_pr_copy(pr.number) .await .expect_status(PullRequestStatus::Closed); Ok(()) @@ -421,7 +421,7 @@ timeout = 3600 tester.refresh_prs().await; tester - .get_pr(pr.number) + .get_pr_copy(pr.number) .await .expect_status(PullRequestStatus::Draft); Ok(()) diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index ee561abc..a8418a73 100644 --- a/src/bors/handlers/review.rs +++ b/src/bors/handlers/review.rs @@ -388,7 +388,7 @@ mod tests { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors r+").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" :pushpin: Commit pr-1-sha has been approved by `default-user` @@ -397,7 +397,7 @@ mod tests { ); tester - .get_pr(()) + .get_pr_copy(()) .await .expect_rollup(None) .expect_approved_by(&User::default_pr_author().name); @@ -414,7 +414,7 @@ mod tests { .post_comment(format!(r#"@bors r={approve_user}"#).as_str()) .await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" :pushpin: Commit pr-1-sha has been approved by `user1` @@ -422,7 +422,10 @@ mod tests { " ); - tester.get_pr(()).await.expect_approved_by(approve_user); + tester + .get_pr_copy(()) + .await + .expect_approved_by(approve_user); Ok(()) }) .await; @@ -435,7 +438,7 @@ mod tests { .post_comment(Comment::from("@bors try").with_author(User::unprivileged())) .await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @"@unprivileged-user: :key: Insufficient privileges: not in try users" ); Ok(()) @@ -451,7 +454,7 @@ mod tests { .await?; tester.post_comment("@bors p=2").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @"@unprivileged-user: :key: Insufficient privileges: not in review users" ); Ok(()) @@ -464,7 +467,7 @@ mod tests { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors r+").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" :pushpin: Commit pr-1-sha has been approved by `default-user` @@ -472,16 +475,16 @@ mod tests { ", ); tester - .get_pr(()) + .get_pr_copy(()) .await .expect_approved_by(&User::default_pr_author().name); tester.post_comment("@bors r-").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @"Commit pr-1-sha has been unapproved." ); - tester.get_pr(()).await.expect_unapproved(); + tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) .await; @@ -496,12 +499,12 @@ mod tests { .post_comment(Comment::from("@bors r-").with_author(User::unprivileged())) .await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @"@unprivileged-user: :key: Insufficient privileges: not in review users" ); tester - .get_pr(()) + .get_pr_copy(()) .await .expect_approved_by(&User::default_pr_author().name); Ok(()) @@ -517,12 +520,12 @@ mod tests { tester.set_pr_status_closed(()).await?; tester.post_comment("@bors r-").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @":clipboard: Only unclosed PRs can be unapproved." ); tester - .get_pr(()) + .get_pr_copy(()) .await .expect_approved_by(&User::default_pr_author().name); Ok(()) @@ -537,7 +540,7 @@ mod tests { tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_priority(Some(10)) .expect_approved_by(&User::default_pr_author().name); @@ -553,7 +556,7 @@ mod tests { tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_priority(Some(10)) .expect_approved_by("user1"); @@ -581,7 +584,7 @@ mod tests { tester.post_comment("@bors r+").await?; tester.expect_comments((), 1).await; - tester.get_pr(()).await.expect_priority(Some(5)); + tester.get_pr_copy(()).await.expect_priority(Some(5)); Ok(()) }) @@ -597,7 +600,7 @@ mod tests { tester.post_comment("@bors r+ p=10").await?; tester.expect_comments((), 1).await; - tester.get_pr(()).await.expect_priority(Some(10)); + tester.get_pr_copy(()).await.expect_priority(Some(10)); Ok(()) }) @@ -609,7 +612,7 @@ mod tests { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors treeclosed=5").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @"Tree closed for PRs with priority less than 5" ); @@ -640,7 +643,7 @@ mod tests { .run_test(async |tester: &mut BorsTester| { tester.post_comment("@bors treeclosed=5").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @"@default-user: :key: Insufficient privileges: not in review users" ); Ok(()) @@ -659,7 +662,7 @@ mod tests { .run_test(async |tester: &mut BorsTester| { tester.post_comment("@bors r+").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @"@default-user: :key: Insufficient privileges: not in review users" ); Ok(()) @@ -676,7 +679,7 @@ mod tests { .post_comment(review_comment("@bors delegate+")) .await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r#" :v: @default-user, you can now approve this pull request! @@ -685,7 +688,7 @@ mod tests { ); tester - .get_pr(()) + .get_pr_copy(()) .await .expect_delegated(DelegatedPermission::Review); Ok(()) @@ -707,7 +710,7 @@ mod tests { tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_approved_by(&User::default_pr_author().name); Ok(()) @@ -763,7 +766,7 @@ mod tests { .run_test(async |tester: &mut BorsTester| { tester.post_comment("@bors delegate+").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @"@default-user: :key: Insufficient privileges: not in review users" ); Ok(()) @@ -781,7 +784,7 @@ mod tests { .await?; tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_delegated(DelegatedPermission::Review); @@ -830,14 +833,14 @@ mod tests { tester.post_comment("@bors r+").await?; tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_approved_by(&User::default_pr_author().name); tester.post_comment(review_comment("@bors r-")).await?; tester.expect_comments((), 1).await; - tester.get_pr(()).await.expect_unapproved(); + tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) @@ -859,7 +862,7 @@ mod tests { .await?; tester.expect_comments((), 1).await; - tester.get_pr(()).await.expect_unapproved(); + tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) @@ -874,7 +877,7 @@ mod tests { .post_comment(Comment::from("@bors delegate+").with_author(User::try_user())) .await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @"@user-with-try-privileges: :key: Insufficient privileges: not in review users" ); Ok(()) @@ -889,7 +892,7 @@ mod tests { tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_rollup(Some(RollupMode::Never)) .expect_approved_by(&User::default_pr_author().name); @@ -905,7 +908,7 @@ mod tests { tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_rollup(Some(RollupMode::Always)) .expect_approved_by(&User::default_pr_author().name); @@ -920,7 +923,7 @@ mod tests { tester.post_comment("@bors r+ rollup-").await?; tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_rollup(Some(RollupMode::Maybe)) .expect_approved_by(&User::default_pr_author().name); @@ -936,7 +939,7 @@ mod tests { tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_priority(Some(10)) .expect_rollup(Some(RollupMode::Never)) @@ -952,7 +955,7 @@ mod tests { tester.post_comment("@bors r=user1 rollup").await?; tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_rollup(Some(RollupMode::Always)) .expect_approved_by("user1"); @@ -967,7 +970,7 @@ mod tests { tester.post_comment("@bors r=user1 rollup=always").await?; tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_rollup(Some(RollupMode::Always)) .expect_approved_by("user1"); @@ -984,7 +987,7 @@ mod tests { .await?; tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_priority(Some(10)) .expect_rollup(Some(RollupMode::Always)) @@ -1030,7 +1033,7 @@ mod tests { tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_rollup(Some(RollupMode::Always)) .expect_approved_by(&User::default_pr_author().name); @@ -1052,7 +1055,7 @@ mod tests { tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_rollup(Some(RollupMode::Always)) .expect_approved_by(&User::default_pr_author().name); @@ -1065,11 +1068,14 @@ mod tests { #[sqlx::test] async fn approve_store_sha(pool: sqlx::PgPool) { run_test(pool, async |tester: &mut BorsTester| { - let pr = tester.get_pr(()).await.get_gh_pr(); + let pr = tester.get_pr_copy(()).await.get_gh_pr(); tester.post_comment("@bors r+").await?; tester.expect_comments((), 1).await; - tester.get_pr(()).await.expect_approved_sha(&pr.head_sha); + tester + .get_pr_copy(()) + .await + .expect_approved_sha(&pr.head_sha); Ok(()) }) @@ -1079,14 +1085,17 @@ mod tests { #[sqlx::test] async fn reapproved_pr_uses_latest_sha(pool: sqlx::PgPool) { run_test(pool, async |tester: &mut BorsTester| { - let pr = tester.get_pr(()).await.get_gh_pr(); + let pr = tester.get_pr_copy(()).await.get_gh_pr(); tester.post_comment("@bors r+").await?; tester.expect_comments((), 1).await; - tester.get_pr(()).await.expect_approved_sha(&pr.head_sha); + tester + .get_pr_copy(()) + .await + .expect_approved_sha(&pr.head_sha); tester.push_to_pr(()).await?; - let pr2 = tester.get_pr(()).await.get_gh_pr(); + let pr2 = tester.get_pr_copy(()).await.get_gh_pr(); assert_ne!(pr.head_sha, pr2.head_sha); tester.expect_comments((), 1).await; @@ -1094,7 +1103,10 @@ mod tests { tester.post_comment("@bors r+").await?; tester.expect_comments((), 1).await; - tester.get_pr(()).await.expect_approved_sha(&pr2.head_sha); + tester + .get_pr_copy(()) + .await + .expect_approved_sha(&pr2.head_sha); Ok(()) }) @@ -1110,7 +1122,7 @@ mod tests { .post_comment(review_comment("@bors delegate=try")) .await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" :v: @default-user, you can now perform try builds on this pull request! @@ -1118,7 +1130,7 @@ mod tests { " ); tester - .get_pr(()) + .get_pr_copy(()) .await .expect_delegated(DelegatedPermission::Try); Ok(()) @@ -1137,7 +1149,7 @@ mod tests { tester.expect_comments((), 1).await; tester.post_comment("@bors try").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… To cancel the try build, run the command `@bors try cancel`. @@ -1158,16 +1170,16 @@ mod tests { .await?; tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_delegated(DelegatedPermission::Try); tester.post_comment("@bors r+").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @"@default-user: :key: Insufficient privileges: not in review users" ); - tester.get_pr(()).await.expect_unapproved(); + tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) @@ -1204,8 +1216,8 @@ mod tests { .set_pr_status_draft(()) .await?; tester.post_comment("@bors r+").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @":clipboard: Only open, non-draft PRs can be approved."); - tester.get_pr(()).await.expect_unapproved(); + insta::assert_snapshot!(tester.get_comment_text(()).await?, @":clipboard: Only open, non-draft PRs can be approved."); + tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) .await; @@ -1218,8 +1230,8 @@ mod tests { .set_pr_status_closed(()) .await?; tester.post_comment("@bors r+").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @":clipboard: Only open, non-draft PRs can be approved."); - tester.get_pr(()).await.expect_unapproved(); + insta::assert_snapshot!(tester.get_comment_text(()).await?, @":clipboard: Only open, non-draft PRs can be approved."); + tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) .await; @@ -1232,8 +1244,8 @@ mod tests { .set_pr_status_merged(()) .await?; tester.post_comment("@bors r+").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @":clipboard: Only open, non-draft PRs can be approved."); - tester.get_pr(()).await.expect_unapproved(); + insta::assert_snapshot!(tester.get_comment_text(()).await?, @":clipboard: Only open, non-draft PRs can be approved."); + tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) .await; @@ -1248,12 +1260,12 @@ mod tests { }) .await?; tester.post_comment("@bors r+").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" :clipboard: Looks like this PR is still in progress, ignoring approval. Hint: Remove **[do not merge]** from this PR's title when it is ready for review. "); - tester.get_pr(()).await.expect_unapproved(); + tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) .await; @@ -1277,8 +1289,8 @@ labels_blocking_approval = ["proposed-final-comment-period"] }) .await?; tester.post_comment("@bors r+").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @":clipboard: This PR cannot be approved because it currently has the following label: `proposed-final-comment-period`."); - tester.get_pr(()).await.expect_unapproved(); + insta::assert_snapshot!(tester.get_comment_text(()).await?, @":clipboard: This PR cannot be approved because it currently has the following label: `proposed-final-comment-period`."); + tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) .await; @@ -1304,8 +1316,8 @@ labels_blocking_approval = ["proposed-final-comment-period", "final-comment-peri }) .await?; tester.post_comment("@bors r+").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @":clipboard: This PR cannot be approved because it currently has the following labels: `proposed-final-comment-period`, `final-comment-period`."); - tester.get_pr(()).await.expect_unapproved(); + insta::assert_snapshot!(tester.get_comment_text(()).await?, @":clipboard: This PR cannot be approved because it currently has the following labels: `proposed-final-comment-period`, `final-comment-period`."); + tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) .await; @@ -1324,7 +1336,7 @@ merge_queue_enabled = true tester.wait_for_pr((), |pr| pr.auto_build.is_some()).await?; tester.workflow_start(tester.auto_branch().await).await?; tester.post_comment("@bors r-").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" Commit pr-1-sha has been unapproved. Auto build cancelled due to unapproval. Cancelled workflows: @@ -1352,7 +1364,7 @@ merge_queue_enabled = true .await?; tester.workflow_start(tester.auto_branch().await).await?; tester.post_comment("@bors r-").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" Commit pr-1-sha has been unapproved. Auto build cancelled due to unapproval. It was not possible to cancel some workflows. @@ -1378,7 +1390,7 @@ merge_queue_enabled = true tester.expect_comments((), 1).await; tester .expect_check_run( - &tester.get_pr(()).await.get_gh_pr().head_sha, + &tester.get_pr_copy(()).await.get_gh_pr().head_sha, AUTO_BUILD_CHECK_RUN_NAME, AUTO_BUILD_CHECK_RUN_NAME, CheckRunStatus::Completed, diff --git a/src/bors/handlers/trybuild.rs b/src/bors/handlers/trybuild.rs index 11e80a74..b48df594 100644 --- a/src/bors/handlers/trybuild.rs +++ b/src/bors/handlers/trybuild.rs @@ -1,7 +1,7 @@ use std::sync::Arc; -use super::has_permission; use super::{PullRequestData, deny_request}; +use super::{has_permission, hide_try_build_started_comments}; use crate::PgDbClient; use crate::bors::command::{CommandPrefix, Parent}; use crate::bors::comment::try_build_cancelled_comment; @@ -75,7 +75,13 @@ pub(super) async fn command_try_build( // Try to cancel any previously running try build workflows let cancelled_workflow_urls = if let Some(build) = get_pending_build(pr.db) { - cancel_previous_try_build(repo, &db, build).await? + let res = cancel_previous_try_build(repo, &db, build).await?; + // Also try to hide previous "Try build started" comments that weren't hidden yet + if let Err(error) = hide_try_build_started_comments(repo, &db, pr.db).await { + tracing::error!("Failed to hide previous try build started comment(s): {error:?}"); + } + + res } else { vec![] }; @@ -284,7 +290,7 @@ mod tests { use crate::database::operations::get_all_workflows; use crate::database::{BuildStatus, WorkflowStatus}; use crate::github::CommitSha; - use crate::github::api::client::MinimizeCommentReason; + use crate::github::api::client::HideCommentReason; use crate::tests::BorsTester; use crate::tests::{ BorsBuilder, Comment, GitHubState, User, WorkflowEvent, WorkflowJob, WorkflowRunData, @@ -300,7 +306,7 @@ mod tests { tester.expect_comments((), 1).await; tester.workflow_full_success(tester.try_branch().await).await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r#" :sunny: Try build successful ([Workflow1](https://github.com/rust-lang/borstest/actions/runs/1)) Build commit: merge-0-pr-1 (`merge-0-pr-1`, parent: `main-sha1`) @@ -320,7 +326,7 @@ mod tests { tester.expect_comments((), 1).await; tester.workflow_full_failure(tester.try_branch().await).await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @":broken_heart: Test for merge-0-pr-1 failed: [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1)" ); Ok(()) @@ -351,7 +357,7 @@ mod tests { tester.modify_repo(&default_repo_name(), |repo| repo.update_workflow_run(workflow.clone(), WorkflowStatus::Pending)).await; tester.workflow_full_failure(workflow).await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" :broken_heart: Test for merge-0-pr-1 failed: [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1). Failed jobs: @@ -371,7 +377,7 @@ mod tests { .post_comment(Comment::from("@bors try").with_author(User::unprivileged())) .await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @"@unprivileged-user: :key: Insufficient privileges: not in try users" ); Ok(()) @@ -386,7 +392,7 @@ mod tests { .post_comment(Comment::from("@bors try").with_author(User::try_user())) .await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… @@ -403,7 +409,7 @@ mod tests { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors try").await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… @@ -546,7 +552,7 @@ try-job: Bar .await?; tester.expect_comments((), 1).await; tester.post_comment("@bors try parent=last").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-1-pr-1… To cancel the try build, run the command `@bors try cancel`. @@ -572,7 +578,7 @@ try-job: Bar tester .post_comment("@bors try parent=last") .await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @":exclamation: There was no previous build. Please set an explicit parent or remove the `parent=last` argument to use the default parent."); + insta::assert_snapshot!(tester.get_comment_text(()).await?, @":exclamation: There was no previous build. Please set an explicit parent or remove the `parent=last` argument to use the default parent."); Ok(()) }) .await; @@ -586,7 +592,7 @@ try-job: Bar tester .post_comment("@bors try") .await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r###" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r###" :lock: Merge conflict This pull request and the master branch diverged in a way that cannot @@ -648,7 +654,7 @@ try-job: Bar tester.post_comment("@bors try").await?; tester.expect_comments((), 1).await; tester.post_comment("@bors try").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-1-pr-1… To cancel the try build, run the command `@bors try cancel`. @@ -670,12 +676,12 @@ try-job: Bar "#, ) .await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… To cancel the try build, run the command `@bors try cancel`. "); - insta::assert_snapshot!(tester.get_comment(()).await?, @"Try build cancelled. Cancelled workflows:"); + insta::assert_snapshot!(tester.get_comment_text(()).await?, @"Try build cancelled. Cancelled workflows:"); Ok(()) }) .await; @@ -691,7 +697,7 @@ try-job: Bar .await?; tester.post_comment("@bors try").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-1-pr-1… (The previously running try build was automatically cancelled.) @@ -715,7 +721,7 @@ try-job: Bar tester.expect_comments((), 1).await; tester.post_comment("@bors try").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-1-pr-1… To cancel the try build, run the command `@bors try cancel`. @@ -727,7 +733,7 @@ try-job: Bar .with_check_suite_id(2), ) .await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r#" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r#" :sunny: Try build successful ([Workflow1](https://github.com/rust-lang/borstest/actions/runs/2)) Build commit: merge-1-pr-1 (`merge-1-pr-1`, parent: `main-sha1`) @@ -742,7 +748,7 @@ try-job: Bar async fn try_cancel_no_running_build(pool: sqlx::PgPool) { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors try cancel").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @":exclamation: There is currently no try build in progress."); + insta::assert_snapshot!(tester.get_comment_text(()).await?, @":exclamation: There is currently no try build in progress."); Ok(()) }) .await; @@ -766,7 +772,7 @@ try-job: Bar )) .await?; tester.post_comment("@bors try cancel").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" Try build cancelled. Cancelled workflows: - https://github.com/rust-lang/borstest/actions/runs/123 - https://github.com/rust-lang/borstest/actions/runs/124 @@ -787,7 +793,7 @@ try-job: Bar .workflow_event(WorkflowEvent::started(tester.try_branch().await)) .await?; tester.post_comment("@bors try cancel").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @"Try build was cancelled. It was not possible to cancel some workflows."); + insta::assert_snapshot!(tester.get_comment_text(()).await?, @"Try build was cancelled. It was not possible to cancel some workflows."); let build = tester.db().find_build( &default_repo_name(), tester.try_branch().await.get_name().to_string(), @@ -813,7 +819,7 @@ try-job: Bar tester.post_comment("@bors try cancel").await?; tester.expect_comments((), 1).await; - tester.get_pr(()).await.expect_try_build_cancelled(); + tester.get_pr_copy(()).await.expect_try_build_cancelled(); Ok(()) }) .await; @@ -841,7 +847,7 @@ try-job: Bar tester.workflow_full_success(w2).await?; tester.workflow_start(w3).await?; tester.post_comment("@bors try cancel").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" Try build cancelled. Cancelled workflows: - https://github.com/rust-lang/borstest/actions/runs/3 "); @@ -879,13 +885,13 @@ try = ["+foo", "+bar", "-baz"] )) .run_test(async |tester: &mut BorsTester| { tester.post_comment("@bors try").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… To cancel the try build, run the command `@bors try cancel`. "); tester - .get_pr(()) + .get_pr_copy(()) .await .expect_added_labels(&["foo", "bar"]) .expect_removed_labels(&["baz"]); @@ -905,18 +911,18 @@ try_succeeded = ["+foo", "+bar", "-baz"] )) .run_test(async |tester: &mut BorsTester| { tester.post_comment("@bors try").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… To cancel the try build, run the command `@bors try cancel`. "); - tester.get_pr(()).await.expect_added_labels(&[]); + tester.get_pr_copy(()).await.expect_added_labels(&[]); tester .workflow_full_success(tester.try_branch().await) .await?; tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_added_labels(&["foo", "bar"]) .expect_removed_labels(&["baz"]); @@ -936,18 +942,18 @@ try_failed = ["+foo", "+bar", "-baz"] )) .run_test(async |tester: &mut BorsTester| { tester.post_comment("@bors try").await?; - insta::assert_snapshot!(tester.get_comment(()).await?, @r" + insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… To cancel the try build, run the command `@bors try cancel`. "); - tester.get_pr(()).await.expect_added_labels(&[]); + tester.get_pr_copy(()).await.expect_added_labels(&[]); tester .workflow_full_failure(tester.try_branch().await) .await?; tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_added_labels(&["foo", "bar"]) .expect_removed_labels(&["baz"]); @@ -964,7 +970,7 @@ try_failed = ["+foo", "+bar", "-baz"] tester .expect_check_run( - &tester.get_pr(()).await.get_gh_pr().head_sha, + &tester.get_pr_copy(()).await.get_gh_pr().head_sha, TRY_BUILD_CHECK_RUN_NAME, "Bors try build", CheckRunStatus::InProgress, @@ -990,7 +996,7 @@ try_failed = ["+foo", "+bar", "-baz"] tester .expect_check_run( - &tester.get_pr(()).await.get_gh_pr().head_sha, + &tester.get_pr_copy(()).await.get_gh_pr().head_sha, TRY_BUILD_CHECK_RUN_NAME, "Bors try build", CheckRunStatus::Completed, @@ -1016,7 +1022,7 @@ try_failed = ["+foo", "+bar", "-baz"] tester .expect_check_run( - &tester.get_pr(()).await.get_gh_pr().head_sha, + &tester.get_pr_copy(()).await.get_gh_pr().head_sha, TRY_BUILD_CHECK_RUN_NAME, "Bors try build", CheckRunStatus::Completed, @@ -1040,7 +1046,7 @@ try_failed = ["+foo", "+bar", "-baz"] tester .expect_check_run( - &tester.get_pr(()).await.get_gh_pr().head_sha, + &tester.get_pr_copy(()).await.get_gh_pr().head_sha, TRY_BUILD_CHECK_RUN_NAME, "Bors try build", CheckRunStatus::Completed, @@ -1059,7 +1065,7 @@ try_failed = ["+foo", "+bar", "-baz"] tester.post_comment("@bors try").await?; tester.expect_comments((), 1).await; - let prev_sha = tester.get_pr(()).await.get_gh_pr().head_sha; + let prev_sha = tester.get_pr_copy(()).await.get_gh_pr().head_sha; tester.push_to_pr(()).await?; tester.post_comment("@bors try").await?; tester.expect_comments((), 1).await; @@ -1075,7 +1081,7 @@ try_failed = ["+foo", "+bar", "-baz"] .await; tester .expect_check_run( - &tester.get_pr(()).await.get_gh_pr().head_sha, + &tester.get_pr_copy(()).await.get_gh_pr().head_sha, TRY_BUILD_CHECK_RUN_NAME, "Bors try build", CheckRunStatus::InProgress, @@ -1088,16 +1094,16 @@ try_failed = ["+foo", "+bar", "-baz"] } #[sqlx::test] - async fn minimize_try_build_started_comment_after_success(pool: sqlx::PgPool) { + async fn hide_try_build_started_comment_after_success(pool: sqlx::PgPool) { run_test(pool, async |tester: &mut BorsTester| { - let comment = tester.post_comment("@bors try").await?; - tester.expect_comments((), 1).await; + tester.post_comment("@bors try").await?; + let comment = tester.get_comment(()).await?; tester .workflow_full_success(tester.try_branch().await) .await?; tester.expect_comments((), 1).await; tester - .expect_minimized_comment(&comment, MinimizeCommentReason::Outdated) + .expect_hidden_comment(&comment, HideCommentReason::Outdated) .await; Ok(()) @@ -1106,16 +1112,34 @@ try_failed = ["+foo", "+bar", "-baz"] } #[sqlx::test] - async fn minimize_try_build_started_comment_after_failure(pool: sqlx::PgPool) { + async fn hide_try_build_started_comment_after_failure(pool: sqlx::PgPool) { run_test(pool, async |tester: &mut BorsTester| { - let comment = tester.post_comment("@bors try").await?; - tester.expect_comments((), 1).await; + tester.post_comment("@bors try").await?; + let comment = tester.get_comment(()).await?; tester .workflow_full_failure(tester.try_branch().await) .await?; tester.expect_comments((), 1).await; tester - .expect_minimized_comment(&comment, MinimizeCommentReason::Outdated) + .expect_hidden_comment(&comment, HideCommentReason::Outdated) + .await; + + Ok(()) + }) + .await; + } + + #[sqlx::test] + async fn hide_try_build_started_comment_after_restart(pool: sqlx::PgPool) { + run_test(pool, async |tester: &mut BorsTester| { + tester.post_comment("@bors try").await?; + let comment = tester.get_comment(()).await?; + + // Hide the previous "Try build started" comment when we restart the build + tester.post_comment("@bors try").await?; + tester.expect_comments((), 1).await; + tester + .expect_hidden_comment(&comment, HideCommentReason::Outdated) .await; Ok(()) diff --git a/src/bors/handlers/workflow.rs b/src/bors/handlers/workflow.rs index 6fa89acc..e692a75a 100644 --- a/src/bors/handlers/workflow.rs +++ b/src/bors/handlers/workflow.rs @@ -1,13 +1,13 @@ use crate::PgDbClient; -use crate::bors::comment::{CommentTag, build_failed_comment, try_build_succeeded_comment}; +use crate::bors::comment::{build_failed_comment, try_build_succeeded_comment}; use crate::bors::event::{WorkflowRunCompleted, WorkflowRunStarted}; -use crate::bors::handlers::get_build_type; use crate::bors::handlers::labels::handle_label_trigger; use crate::bors::handlers::{BuildType, is_bors_observed_branch}; +use crate::bors::handlers::{get_build_type, hide_try_build_started_comments}; use crate::bors::merge_queue::MergeQueueSender; use crate::bors::{FailedWorkflowRun, RepositoryState, WorkflowRun}; use crate::database::{BuildModel, BuildStatus, PullRequestModel, WorkflowModel, WorkflowStatus}; -use crate::github::api::client::{GithubRepositoryClient, MinimizeCommentReason}; +use crate::github::api::client::GithubRepositoryClient; use crate::github::{CommitSha, LabelTrigger}; use octocrab::models::CheckRunId; use octocrab::models::workflows::{Conclusion, Job, Status}; @@ -279,16 +279,7 @@ async fn maybe_complete_build( }; if build_type == BuildType::Try { - // Hide "Try build started" comments that are now outdated - let outdated = db - .get_tagged_bot_comments(repo.repository(), pr.number, CommentTag::TryBuildStarted) - .await?; - for comment in outdated { - repo.client - .minimize_comment(&comment.node_id, MinimizeCommentReason::Outdated) - .await?; - db.delete_tagged_bot_comment(&comment).await?; - } + hide_try_build_started_comments(repo, db, &pr).await?; } if let Some(comment) = comment_opt { @@ -564,7 +555,7 @@ mod tests { tester.workflow_full_success(w2).await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r#" :sunny: Try build successful - [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1) :white_check_mark: @@ -594,7 +585,7 @@ mod tests { tester.workflow_event(WorkflowEvent::success(w1)).await?; tester.workflow_event(WorkflowEvent::success(w2)).await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r#" :sunny: Try build successful - [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1) :white_check_mark: @@ -623,7 +614,7 @@ mod tests { tester.workflow_event(WorkflowEvent::failure(w2)).await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @":broken_heart: Test for merge-0-pr-1 failed: [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1), [Workflow1](https://github.com/rust-lang/borstest/actions/runs/2)" ); Ok(()) @@ -644,7 +635,7 @@ mod tests { tester.expect_comments((), 1).await; tester .expect_check_run( - &tester.get_pr(()).await.get_gh_pr().head_sha, + &tester.get_pr_copy(()).await.get_gh_pr().head_sha, AUTO_BUILD_CHECK_RUN_NAME, AUTO_BUILD_CHECK_RUN_NAME, CheckRunStatus::Completed, @@ -672,14 +663,14 @@ auto_build_succeeded = ["+foo", "+bar", "-baz"] .run_test(async |tester: &mut BorsTester| { tester.start_auto_build(()).await?; - tester.get_pr(()).await.expect_added_labels(&[]); + tester.get_pr_copy(()).await.expect_added_labels(&[]); tester .workflow_full_success(tester.auto_branch().await) .await?; tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_added_labels(&["foo", "bar"]) .expect_removed_labels(&["baz"]); @@ -704,14 +695,14 @@ auto_build_failed = ["+foo", "+bar", "-baz"] .run_test(async |tester: &mut BorsTester| { tester.start_auto_build(()).await?; - tester.get_pr(()).await.expect_added_labels(&[]); + tester.get_pr_copy(()).await.expect_added_labels(&[]); tester .workflow_full_failure(tester.auto_branch().await) .await?; tester.expect_comments((), 1).await; tester - .get_pr(()) + .get_pr_copy(()) .await .expect_added_labels(&["foo", "bar"]) .expect_removed_labels(&["baz"]); @@ -745,14 +736,14 @@ auto_build_failed = ["+foo", "+bar", "-baz"] .run_test(async |tester: &mut BorsTester| { tester.start_auto_build(()).await?; - tester.get_pr(()).await.expect_added_labels(&[]); + tester.get_pr_copy(()).await.expect_added_labels(&[]); tester.workflow_full_failure(tester.auto_branch().await).await?; tester.wait_for_pr((), |pr| { pr.auto_build.as_ref().unwrap().status == BuildStatus::Failure }).await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @":broken_heart: Test for merge-0-pr-1 failed: [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1)" ); @@ -774,7 +765,7 @@ auto_build_failed = ["+foo", "+bar", "-baz"] tester.expect_comments((), 1).await; tester .expect_check_run( - &tester.get_pr(()).await.get_gh_pr().head_sha, + &tester.get_pr_copy(()).await.get_gh_pr().head_sha, AUTO_BUILD_CHECK_RUN_NAME, AUTO_BUILD_CHECK_RUN_NAME, CheckRunStatus::Completed, diff --git a/src/bors/merge_queue.rs b/src/bors/merge_queue.rs index 24ea64dd..f75efbc1 100644 --- a/src/bors/merge_queue.rs +++ b/src/bors/merge_queue.rs @@ -436,7 +436,7 @@ mod tests { tester.start_auto_build(()).await?; tester .expect_check_run( - &tester.get_pr(()).await.get_gh_pr().head_sha, + &tester.get_pr_copy(()).await.get_gh_pr().head_sha, AUTO_BUILD_CHECK_RUN_NAME, AUTO_BUILD_CHECK_RUN_NAME, CheckRunStatus::InProgress, @@ -455,7 +455,7 @@ mod tests { tester.expect_comments((), 1).await; tester.process_merge_queue().await; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @":hourglass: Testing commit pr-1-sha with merge merge-0-pr-1..." ); Ok(()) @@ -493,7 +493,7 @@ mod tests { tester.start_auto_build(()).await?; tester.workflow_full_success(tester.auto_branch().await).await?; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r" :sunny: Test successful - [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1) Approved by: `default-user` @@ -647,7 +647,7 @@ mod tests { .await; tester.process_merge_queue().await; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @":eyes: Test was successful, but fast-forwarding failed: this PR has conflicts with the `main` branch" ); Ok(()) @@ -671,7 +671,7 @@ mod tests { .await; tester.process_merge_queue().await; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @":eyes: Test was successful, but fast-forwarding failed: this PR is behind the `main` branch" ); Ok(()) @@ -695,7 +695,7 @@ mod tests { .await; tester.process_merge_queue().await; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @":eyes: Test was successful, but fast-forwarding failed: IO error" ); Ok(()) @@ -770,7 +770,7 @@ mod tests { tester.expect_comments((), 1).await; tester.process_merge_queue().await; insta::assert_snapshot!( - tester.get_comment(()).await?, + tester.get_comment_text(()).await?, @r#" :lock: Merge conflict diff --git a/src/database/mod.rs b/src/database/mod.rs index 9e18251d..643b1521 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -511,7 +511,7 @@ pub struct RepoModel { pub created_at: DateTime, } -/// Represents a tagged comment made by the bors GitHub app that can be later minimized. +/// Represents a tagged comment made by the bors GitHub app that can be later hidden. pub struct CommentModel { pub id: PrimaryKey, /// The GitHub repository this comment belongs to. diff --git a/src/github/api/client.rs b/src/github/api/client.rs index 2ce8699a..d8f8f483 100644 --- a/src/github/api/client.rs +++ b/src/github/api/client.rs @@ -465,13 +465,13 @@ impl GithubRepositoryClient { Ok(response) } - /// Minimizes a comment on an Issue, Commit, Pull Request, or Gist. + /// Hides a comment on an Issue, Commit, Pull Request, or Gist. /// /// GitHub Docs: - pub async fn minimize_comment( + pub async fn hide_comment( &self, node_id: &str, - reason: MinimizeCommentReason, + reason: HideCommentReason, ) -> anyhow::Result<()> { const QUERY: &str = "mutation($node_id: ID!, $reason: ReportedContentClassifiers!) { minimizeComment(input: {subjectId: $node_id, classifier: $reason}) { @@ -482,17 +482,17 @@ impl GithubRepositoryClient { #[derive(Serialize)] struct Variables<'a> { node_id: &'a str, - reason: MinimizeCommentReason, + reason: HideCommentReason, } #[derive(Deserialize)] struct Output {} - tracing::debug!(node_id, ?reason, "Minimizing comment"); - perform_retryable("minimize_comment", RetryMethod::default(), || async { + tracing::debug!(node_id, ?reason, "Hiding comment"); + perform_retryable("hide_comment", RetryMethod::default(), || async { self.graphql::(QUERY, Variables { node_id, reason }) .await - .context("Failed to minimize comment")?; + .context("Failed to hide comment")?; anyhow::Ok(()) }) .await?; @@ -500,12 +500,12 @@ impl GithubRepositoryClient { } } -/// The reasons a piece of content can be reported or minimized. +/// The reasons a piece of content can be reported or hidden. /// /// GitHub Docs: #[derive(Debug, Copy, Clone, Deserialize, Serialize, PartialEq, Eq)] #[serde(rename_all = "SCREAMING_SNAKE_CASE")] -pub enum MinimizeCommentReason { +pub enum HideCommentReason { /// An abusive or harassing piece of content. Abuse, /// A duplicated piece of content. diff --git a/src/tests/mocks/github.rs b/src/tests/mocks/github.rs index f87bca30..fd00c02b 100644 --- a/src/tests/mocks/github.rs +++ b/src/tests/mocks/github.rs @@ -5,12 +5,12 @@ use std::sync::Arc; use wiremock::{MockServer, Request, ResponseTemplate}; use crate::create_github_client; -use crate::github::api::client::MinimizeCommentReason; +use crate::github::api::client::HideCommentReason; use crate::tests::GitHubState; use crate::tests::mocks::app::{AppHandler, default_app_id}; use crate::tests::mocks::pull_request::CommentMsg; use crate::tests::mocks::repository::{mock_repo, mock_repo_list}; -use crate::tests::mocks::{MinimizedComment, dynamic_mock_req}; +use crate::tests::mocks::{HiddenComment, dynamic_mock_req}; pub struct GitHubMockServer { mock_server: MockServer, @@ -121,7 +121,7 @@ async fn mock_graphql(github: Arc>, mock_server: #[derive(serde::Deserialize)] struct Variables { node_id: String, - reason: MinimizeCommentReason, + reason: HideCommentReason, } let github = github.clone(); @@ -130,12 +130,10 @@ async fn mock_graphql(github: Arc>, mock_server: // We have to use e.g. `blocking_lock` to lock from a sync function. // It has to happen in a separate thread though. std::thread::spawn(move || { - github - .blocking_lock() - .add_minimized_comment(MinimizedComment { - node_id: data.node_id, - reason: data.reason, - }); + github.blocking_lock().add_hidden_comment(HiddenComment { + node_id: data.node_id, + reason: data.reason, + }); }) .join() .unwrap(); diff --git a/src/tests/mocks/mod.rs b/src/tests/mocks/mod.rs index d88f5ec2..e17fa246 100644 --- a/src/tests/mocks/mod.rs +++ b/src/tests/mocks/mod.rs @@ -4,7 +4,7 @@ use std::sync::Arc; use crate::TeamApiClient; use crate::github::GithubRepoName; -use crate::github::api::client::MinimizeCommentReason; +use crate::github::api::client::HideCommentReason; use crate::tests::Comment; use crate::tests::mocks::github::GitHubMockServer; use crate::tests::mocks::permissions::TeamApiMockServer; @@ -27,15 +27,15 @@ pub mod user; pub mod workflow; #[derive(Debug)] -pub struct MinimizedComment { +pub struct HiddenComment { pub node_id: String, - pub reason: MinimizeCommentReason, + pub reason: HideCommentReason, } /// Represents the state of GitHub. pub struct GitHubState { pub(super) repos: HashMap>>, - minimized_comments: Vec, + hidden_comments: Vec, } impl GitHubState { @@ -70,8 +70,8 @@ impl GitHubState { self } - pub fn add_minimized_comment(&mut self, comment: MinimizedComment) { - self.minimized_comments.push(comment); + pub fn add_hidden_comment(&mut self, comment: HiddenComment) { + self.hidden_comments.push(comment); } pub fn check_sha_history(&self, repo: GithubRepoName, branch: &str, expected_shas: &[&str]) { @@ -141,13 +141,13 @@ impl GitHubState { Ok(comment) } - pub fn check_minimized_comment(&self, comment: &Comment, reason: MinimizeCommentReason) { + pub fn check_hidden_comment(&self, comment: &Comment, reason: HideCommentReason) { assert!( - self.minimized_comments + self.hidden_comments .iter() .any(|c| &c.node_id == comment.node_id.as_ref().unwrap() && c.reason == reason), - "Comment {comment:?} was not minimized with reason {reason:?}.\nMinized comments: {:?}", - self.minimized_comments + "Comment {comment:?} was not hidden with reason {reason:?}.\nHidden comments: {:?}", + self.hidden_comments ); } } @@ -157,7 +157,7 @@ impl Default for GitHubState { let repo = Repo::default(); Self { repos: HashMap::from([(repo.name.clone(), Arc::new(Mutex::new(repo)))]), - minimized_comments: Default::default(), + hidden_comments: Default::default(), } } } diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 6c09c5b5..8d74d374 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -43,7 +43,7 @@ use crate::tests::mocks::workflow::{ }; // Public re-exports for use in tests -use crate::github::api::client::MinimizeCommentReason; +use crate::github::api::client::HideCommentReason; pub use io::load_test_file; pub use mocks::ExternalHttpMock; pub use mocks::GitHubState; @@ -230,7 +230,7 @@ impl BorsTester { } /// Get a PR proxy that can be used to assert various things about the PR. - pub async fn get_pr>(&self, id: Id) -> PullRequestProxy { + pub async fn get_pr_copy>(&self, id: Id) -> PullRequestProxy { let id = id.into(); let pr = self .get_repo(&id.repo) @@ -347,22 +347,34 @@ impl BorsTester { self.get_branch("automation/bors/auto").await } - /// Wait until the next bot comment is received on the specified repo and PR. - pub async fn get_comment>(&mut self, id: Id) -> anyhow::Result { + /// Wait until the next bot comment is received on the specified repo and PR, and return its + /// text. + pub async fn get_comment_text>( + &mut self, + id: Id, + ) -> anyhow::Result { Ok(GitHubState::get_comment(self.github.clone(), id) .await? .content) } + /// Wait until the next bot comment is received on the specified repo and PR, and return it. + pub async fn get_comment>(&mut self, id: Id) -> anyhow::Result { + GitHubState::get_comment(self.github.clone(), id).await + } + //-- Generation of GitHub events --// pub async fn post_comment>(&mut self, comment: C) -> anyhow::Result { let comment = comment.into(); // Allocate comment IDs let (id, node_id) = self - .get_pr(comment.pr_ident.clone()) + .github + .lock() .await - .gh_pr + .get_repo(&comment.pr_ident.repo) + .lock() + .get_pr_mut(comment.pr_ident.number) .next_comment_ids(); let comment = comment.with_ids(id, node_id); @@ -720,18 +732,18 @@ impl BorsTester { let id = id.into(); for i in 0..count { let id = id.clone(); - self.get_comment(id) + self.get_comment_text(id) .await .unwrap_or_else(|_| panic!("Failed to get comment #{i}")); } } - /// Assert that the given comment has been minimized. - pub async fn expect_minimized_comment(&self, comment: &Comment, reason: MinimizeCommentReason) { + /// Assert that the given comment has been hidden. + pub async fn expect_hidden_comment(&self, comment: &Comment, reason: HideCommentReason) { self.github .lock() .await - .check_minimized_comment(comment, reason); + .check_hidden_comment(comment, reason); } pub async fn expect_check_run( @@ -797,7 +809,7 @@ impl BorsTester { /// /// This method is useful if you execute a command that produces no comment as an output /// and you need to wait until it has been processed by bors. - /// Prefer using [BorsTester::expect_comments] or [BorsTester::get_comment] to synchronize + /// Prefer using [BorsTester::expect_comments] or [BorsTester::get_comment_text] to synchronize /// if you are waiting for a comment to be posted to a PR. pub async fn wait_for(&self, condition: F) -> anyhow::Result<()> where