From 5a76e5e09fb1989d76010578cc9f06a880332184 Mon Sep 17 00:00:00 2001 From: mostafa Date: Sun, 21 Sep 2025 23:48:09 +0300 Subject: [PATCH 1/5] Add workflow links to try build started comments --- ...9247855b9712d6633c1dc3683751941749b1.json} | 4 +- src/bors/comment.rs | 8 ++ src/bors/handlers/mod.rs | 2 +- src/bors/handlers/workflow.rs | 52 ++++++++- src/database/mod.rs | 2 +- src/database/operations.rs | 1 + src/github/api/client.rs | 103 ++++++++++++++++++ src/tests/mocks/github.rs | 85 +++++++++++---- 8 files changed, 225 insertions(+), 32 deletions(-) rename .sqlx/{query-94ce8ab6ef6cb76aa62aaf81c9f4b1751318faf2009b411b6cd616f47a82abf0.json => query-070fd5babee0d508900290c872bd9247855b9712d6633c1dc3683751941749b1.json} (88%) diff --git a/.sqlx/query-94ce8ab6ef6cb76aa62aaf81c9f4b1751318faf2009b411b6cd616f47a82abf0.json b/.sqlx/query-070fd5babee0d508900290c872bd9247855b9712d6633c1dc3683751941749b1.json similarity index 88% rename from .sqlx/query-94ce8ab6ef6cb76aa62aaf81c9f4b1751318faf2009b411b6cd616f47a82abf0.json rename to .sqlx/query-070fd5babee0d508900290c872bd9247855b9712d6633c1dc3683751941749b1.json index 98b40198..cda6106b 100644 --- a/.sqlx/query-94ce8ab6ef6cb76aa62aaf81c9f4b1751318faf2009b411b6cd616f47a82abf0.json +++ b/.sqlx/query-070fd5babee0d508900290c872bd9247855b9712d6633c1dc3683751941749b1.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "\n SELECT\n id,\n repository as \"repository: GithubRepoName\",\n pr_number as \"pr_number: i64\",\n tag as \"tag: CommentTag\",\n node_id,\n created_at as \"created_at: DateTime\"\n FROM comment\n WHERE repository = $1\n AND pr_number = $2\n AND tag = $3\n ", + "query": "\n SELECT\n id,\n repository as \"repository: GithubRepoName\",\n pr_number as \"pr_number: i64\",\n tag as \"tag: CommentTag\",\n node_id,\n created_at as \"created_at: DateTime\"\n FROM comment\n WHERE repository = $1\n AND pr_number = $2\n AND tag = $3\n ORDER BY created_at\n ", "describe": { "columns": [ { @@ -50,5 +50,5 @@ false ] }, - "hash": "94ce8ab6ef6cb76aa62aaf81c9f4b1751318faf2009b411b6cd616f47a82abf0" + "hash": "070fd5babee0d508900290c872bd9247855b9712d6633c1dc3683751941749b1" } diff --git a/src/bors/comment.rs b/src/bors/comment.rs index 5eeac044..4fba8fd2 100644 --- a/src/bors/comment.rs +++ b/src/bors/comment.rs @@ -198,6 +198,14 @@ pub fn try_build_started_comment( Comment::new(msg) } +pub fn append_workflow_links_to_comment(comment_content: &mut String, workflow_links: Vec) { + comment_content.push_str("\n\n**Workflows**:\n\n"); + + for link in workflow_links { + comment_content.push_str(&format!("- {link}\n")); + } +} + pub fn merge_conflict_comment(branch: &str) -> Comment { let message = format!( r#":lock: Merge conflict diff --git a/src/bors/handlers/mod.rs b/src/bors/handlers/mod.rs index 3ac68178..73010049 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -108,7 +108,7 @@ pub async fn handle_bors_repository_event( repo = payload.repository.to_string(), id = payload.run_id.into_inner() ); - handle_workflow_started(db, payload) + handle_workflow_started(repo, db, payload) .instrument(span.clone()) .await?; diff --git a/src/bors/handlers/workflow.rs b/src/bors/handlers/workflow.rs index 11229f6f..a53783ad 100644 --- a/src/bors/handlers/workflow.rs +++ b/src/bors/handlers/workflow.rs @@ -1,5 +1,8 @@ +use super::trybuild::TRY_BRANCH_NAME; use crate::PgDbClient; -use crate::bors::comment::{build_failed_comment, try_build_succeeded_comment}; +use crate::bors::comment::{ + CommentTag, append_workflow_links_to_comment, build_failed_comment, try_build_succeeded_comment, +}; use crate::bors::event::{WorkflowRunCompleted, WorkflowRunStarted}; use crate::bors::handlers::labels::handle_label_trigger; use crate::bors::handlers::{BuildType, is_bors_observed_branch}; @@ -17,6 +20,7 @@ use std::sync::Arc; use std::time::Duration; pub(super) async fn handle_workflow_started( + repo: Arc, db: Arc, payload: WorkflowRunStarted, ) -> anyhow::Result<()> { @@ -53,14 +57,53 @@ pub(super) async fn handle_workflow_started( tracing::info!("Storing workflow started into DB"); db.create_workflow( &build, - payload.name, - payload.url, + payload.name.clone(), + payload.url.clone(), payload.run_id.into(), - payload.workflow_type, + payload.workflow_type.clone(), WorkflowStatus::Pending, ) .await?; + if build.branch == TRY_BRANCH_NAME { + add_workflow_links_to_try_build_start_comment(repo, db, &build, payload).await?; + } + + Ok(()) +} + +async fn add_workflow_links_to_try_build_start_comment( + repo: Arc, + db: Arc, + build: &BuildModel, + payload: WorkflowRunStarted, +) -> anyhow::Result<()> { + let Some(pr) = db.find_pr_by_build(build).await? else { + tracing::warn!("PR for build not found"); + return Ok(()); + }; + let comments = db + .get_tagged_bot_comments(&payload.repository, pr.number, CommentTag::TryBuildStarted) + .await?; + + let Some(try_build_comment) = comments.last() else { + tracing::warn!("No try build comment found for PR"); + return Ok(()); + }; + + let workflows = db.get_workflow_urls_for_build(build).await?; + + let mut comment_content = repo + .client + .get_comment_content(&try_build_comment.node_id) + .await?; + + append_workflow_links_to_comment(&mut comment_content, workflows); + + repo.client + .update_comment_content(&try_build_comment.node_id, &comment_content) + .await?; + Ok(()) } @@ -108,7 +151,6 @@ pub(super) async fn handle_workflow_completed( ) .await } - /// Attempt to complete a pending build after a workflow run has been completed. /// We assume that the status of the completed workflow run has already been updated in the /// database. diff --git a/src/database/mod.rs b/src/database/mod.rs index ee99acec..4f6fd553 100644 --- a/src/database/mod.rs +++ b/src/database/mod.rs @@ -394,7 +394,7 @@ impl PullRequestModel { /// Describes whether a workflow is a Github Actions workflow or if it's a job from some external /// CI. -#[derive(Debug, PartialEq, sqlx::Type)] +#[derive(Debug, Clone, PartialEq, sqlx::Type)] #[sqlx(type_name = "TEXT")] #[sqlx(rename_all = "lowercase")] pub enum WorkflowType { diff --git a/src/database/operations.rs b/src/database/operations.rs index a1b60014..b8826531 100644 --- a/src/database/operations.rs +++ b/src/database/operations.rs @@ -1043,6 +1043,7 @@ pub(crate) async fn get_tagged_bot_comments( WHERE repository = $1 AND pr_number = $2 AND tag = $3 + ORDER BY created_at "#, repo as &GithubRepoName, pr_number.0 as i32, diff --git a/src/github/api/client.rs b/src/github/api/client.rs index d8f8f483..0b9c85b4 100644 --- a/src/github/api/client.rs +++ b/src/github/api/client.rs @@ -498,6 +498,109 @@ impl GithubRepositoryClient { .await?; Ok(()) } + + pub async fn get_comment_content(&self, node_id: &str) -> anyhow::Result { + const QUERY: &str = r#" + query($node_id: ID!) { + node(id: $node_id) { + ... on IssueComment { + body + } + } + } + "#; + + #[derive(Serialize)] + struct Variables<'a> { + node_id: &'a str, + } + + #[derive(Deserialize)] + struct Output { + node: Option, + } + + #[derive(Deserialize)] + struct IssueCommentNode { + body: String, + } + + tracing::debug!(node_id, "Fetching comment content"); + + let output: Output = + perform_retryable("get_comment_content", RetryMethod::default(), || async { + self.graphql::(QUERY, Variables { node_id }) + .await + .context("Failed to fetch comment content") + }) + .await?; + + match output.node { + Some(comment) => Ok(comment.body), + None => anyhow::bail!("No comment found for node_id: {}", node_id), + } + } + pub async fn update_comment_content( + &self, + node_id: &str, + new_body: &str, + ) -> anyhow::Result<()> { + const QUERY: &str = r#" + mutation($id: ID!, $body: String!) { + updateComment(input: {id: $id, body: $body}) { + comment { + id + body + } + } + } + "#; + + #[derive(Serialize)] + struct Variables<'a> { + id: &'a str, + body: &'a str, + } + + #[derive(Deserialize)] + struct Output { + update_comment: Option, + } + + #[derive(Deserialize)] + struct UpdatedComment { + comment: CommentNode, + } + + #[derive(Deserialize)] + struct CommentNode { + body: String, + } + + tracing::debug!(node_id, "Updating comment content"); + + let output: Output = + perform_retryable("update_comment_content", RetryMethod::default(), || async { + self.graphql::( + QUERY, + Variables { + id: node_id, + body: new_body, + }, + ) + .await + .context("Failed to update comment") + }) + .await?; + + match output.update_comment { + Some(updated_comment) => { + tracing::debug!("Updated comment content: {}", updated_comment.comment.body); + Ok(()) + } + None => anyhow::bail!("Failed to update comment: {node_id}"), + } + } } /// The reasons a piece of content can be reported or hidden. diff --git a/src/tests/mocks/github.rs b/src/tests/mocks/github.rs index fd00c02b..f4f33648 100644 --- a/src/tests/mocks/github.rs +++ b/src/tests/mocks/github.rs @@ -108,40 +108,79 @@ async fn mock_graphql(github: Arc>, mock_server: let query: Document<&str> = graphql_parser::parse_query(&body.query).expect("Could not parse GraphQL query"); let definition = query.definitions.into_iter().next().unwrap(); - let mutation = match definition { - Definition::Operation(OperationDefinition::Mutation(mutation)) => mutation, + let selection_set = match definition { + Definition::Operation(OperationDefinition::Mutation(mutation)) => { + mutation.selection_set + } + Definition::Operation(OperationDefinition::Query(query)) => query.selection_set, _ => panic!("Unexpected GraphQL query: {}", body.query), }; - let selection = mutation.selection_set.items.into_iter().next().unwrap(); + let selection = selection_set.items.into_iter().next().unwrap(); let operation = match selection { Selection::Field(field) => field, _ => panic!("Unexpected GraphQL selection"), }; - if operation.name == "minimizeComment" { - #[derive(serde::Deserialize)] - struct Variables { - node_id: String, - reason: HideCommentReason, + + match operation.name { + "minimizeComment" => { + #[derive(serde::Deserialize)] + struct Variables { + node_id: String, + reason: HideCommentReason, + } + + let github = github.clone(); + let data: Variables = serde_json::from_value(body.variables).unwrap(); + + // 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_hidden_comment(HiddenComment { + node_id: data.node_id, + reason: data.reason, + }); + }) + .join() + .unwrap(); + ResponseTemplate::new(200).set_body_json(HashMap::::new()) + } + "updateComment" => { + #[derive(serde::Deserialize)] + struct Variables { + id: String, + body: String, + } + + let data: Variables = serde_json::from_value(body.variables).unwrap(); + let response = serde_json::json!({ + "update_comment": { + "comment": { + "id": data.id, + "body": data.body + } + } + }); + + ResponseTemplate::new(200).set_body_json(response) } + "node" => { + #[derive(serde::Deserialize)] + struct Variables { + node_id: String, + } - let github = github.clone(); - let data: Variables = serde_json::from_value(body.variables).unwrap(); + let data: Variables = serde_json::from_value(body.variables).unwrap(); - // 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_hidden_comment(HiddenComment { - node_id: data.node_id, - reason: data.reason, + let response = serde_json::json!({ + "node": { + "__typename": "IssueComment", + "body":format!(":hourglass: Trying commit {} \n To cancel the try build, run the command @bors try cancel`.", data.node_id) + } }); - }) - .join() - .unwrap(); - } else { - panic!("Unexpected operation {}", operation.name); + ResponseTemplate::new(200).set_body_json(response) + } + _ => panic!("Unexpected GraphQL operation {}", operation.name), } - - ResponseTemplate::new(200).set_body_json(HashMap::::new()) }, "POST", "^/graphql$".to_string(), From dca3ea3dc40dba1755fae026c5a91238fd4b5908 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Mon, 22 Sep 2025 21:01:23 +0200 Subject: [PATCH 2/5] Rename `get_comment` to `get_next_comment` --- src/bors/handlers/help.rs | 2 +- src/bors/handlers/info.rs | 10 +++---- src/bors/handlers/mod.rs | 2 +- src/bors/handlers/ping.rs | 2 +- src/bors/handlers/pr_events.rs | 8 +++--- src/bors/handlers/refresh.rs | 2 +- src/bors/handlers/retry.rs | 6 ++-- src/bors/handlers/review.rs | 50 +++++++++++++++++----------------- src/bors/handlers/trybuild.rs | 46 +++++++++++++++---------------- src/bors/handlers/workflow.rs | 10 +++---- src/bors/merge_queue.rs | 14 +++++----- src/tests/mocks/mod.rs | 2 +- src/tests/mod.rs | 19 +++++++------ 13 files changed, 88 insertions(+), 85 deletions(-) diff --git a/src/bors/handlers/help.rs b/src/bors/handlers/help.rs index 3b27534d..d2250a29 100644 --- a/src/bors/handlers/help.rs +++ b/src/bors/handlers/help.rs @@ -87,7 +87,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_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_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 b4a4760d..ae1a420f 100644 --- a/src/bors/handlers/info.rs +++ b/src/bors/handlers/info.rs @@ -93,7 +93,7 @@ mod tests { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors info").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r" ## Status of PR `1` - Not Approved @@ -113,7 +113,7 @@ mod tests { tester.post_comment("@bors info").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r" ## Status of PR `1` - Approved by: `default-user` @@ -134,7 +134,7 @@ mod tests { tester.post_comment("@bors info").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r" ## Status of PR `1` - Not Approved @@ -155,7 +155,7 @@ mod tests { tester.post_comment("@bors info").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r" ## Status of PR `1` - Not Approved @@ -186,7 +186,7 @@ mod tests { tester.post_comment("@bors info").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_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 73010049..7ac622a8 100644 --- a/src/bors/handlers/mod.rs +++ b/src/bors/handlers/mod.rs @@ -721,7 +721,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_text(()).await?, @r#"Unknown command "foo". Run `@bors help` to see available commands."#); + insta::assert_snapshot!(tester.get_next_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 431df8dc..84263c70 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_text(()).await?, "Pong 🏓!"); + assert_eq!(tester.get_next_comment_text(()).await?, "Pong 🏓!"); Ok(()) }) .await; diff --git a/src/bors/handlers/pr_events.rs b/src/bors/handlers/pr_events.rs index fe7e7f07..80d1048e 100644 --- a/src/bors/handlers/pr_events.rs +++ b/src/bors/handlers/pr_events.rs @@ -318,7 +318,7 @@ mod tests { .await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r" :warning: The base branch changed to `beta`, and the PR will need to be re-approved. @@ -368,7 +368,7 @@ mod tests { tester.push_to_pr(()).await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_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. @@ -681,7 +681,7 @@ mod tests { .workflow_start(WorkflowRunData::from(tester.auto_branch().await).with_run_id(123)) .await?; tester.push_to_pr(()).await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_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.workflow_start(tester.auto_branch().await).await?; tester.push_to_pr(()).await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_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. diff --git a/src/bors/handlers/refresh.rs b/src/bors/handlers/refresh.rs index d60d326c..59062cbd 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_text(()).await?, @":boom: Test timed out after `3600`s"); + insta::assert_snapshot!(tester.get_next_comment_text(()).await?, @":boom: Test timed out after `3600`s"); assert_eq!( tester .db() diff --git a/src/bors/handlers/retry.rs b/src/bors/handlers/retry.rs index 1f9e82a5..8aec2478 100644 --- a/src/bors/handlers/retry.rs +++ b/src/bors/handlers/retry.rs @@ -55,7 +55,7 @@ mod tests { .post_comment(Comment::from("@bors retry").with_author(User::unprivileged())) .await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @"@unprivileged-user: :key: Insufficient privileges: not in review users" ); Ok(()) @@ -68,7 +68,7 @@ mod tests { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment(Comment::from("@bors retry")).await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @":exclamation: You can only retry pull requests that are approved and have a previously failed auto build" ); Ok(()) @@ -89,7 +89,7 @@ mod tests { tester.wait_for_pr((), |pr| pr.auto_build.is_none()).await?; tester.process_merge_queue().await; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @":hourglass: Testing commit pr-1-sha with merge merge-1-pr-1..." ); Ok(()) diff --git a/src/bors/handlers/review.rs b/src/bors/handlers/review.rs index 26134aa1..497674e9 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_text(()).await?, + tester.get_next_comment_text(()).await?, @r" :pushpin: Commit pr-1-sha has been approved by `default-user` @@ -414,7 +414,7 @@ mod tests { .post_comment(format!(r#"@bors r={approve_user}"#).as_str()) .await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r" :pushpin: Commit pr-1-sha has been approved by `user1` @@ -438,7 +438,7 @@ mod tests { .post_comment(Comment::from("@bors try").with_author(User::unprivileged())) .await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @"@unprivileged-user: :key: Insufficient privileges: not in try users" ); Ok(()) @@ -454,7 +454,7 @@ mod tests { .await?; tester.post_comment("@bors p=2").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @"@unprivileged-user: :key: Insufficient privileges: not in review users" ); Ok(()) @@ -467,7 +467,7 @@ mod tests { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors r+").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r" :pushpin: Commit pr-1-sha has been approved by `default-user` @@ -480,7 +480,7 @@ mod tests { .expect_approved_by(&User::default_pr_author().name); tester.post_comment("@bors r-").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @"Commit pr-1-sha has been unapproved." ); @@ -498,7 +498,7 @@ mod tests { .post_comment(Comment::from("@bors r-").with_author(User::unprivileged())) .await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @"@unprivileged-user: :key: Insufficient privileges: not in review users" ); @@ -518,7 +518,7 @@ mod tests { tester.set_pr_status_closed(()).await?; tester.post_comment("@bors r-").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @":clipboard: Only unclosed PRs can be unapproved." ); @@ -609,7 +609,7 @@ mod tests { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors treeclosed=5").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @"Tree closed for PRs with priority less than 5" ); @@ -640,7 +640,7 @@ mod tests { .run_test(async |tester: &mut BorsTester| { tester.post_comment("@bors treeclosed=5").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @"@default-user: :key: Insufficient privileges: not in review users" ); Ok(()) @@ -659,7 +659,7 @@ mod tests { .run_test(async |tester: &mut BorsTester| { tester.post_comment("@bors r+").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @"@default-user: :key: Insufficient privileges: not in review users" ); Ok(()) @@ -676,7 +676,7 @@ mod tests { .post_comment(review_comment("@bors delegate+")) .await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r#" :v: @default-user, you can now approve this pull request! @@ -762,7 +762,7 @@ mod tests { .run_test(async |tester: &mut BorsTester| { tester.post_comment("@bors delegate+").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @"@default-user: :key: Insufficient privileges: not in review users" ); Ok(()) @@ -872,7 +872,7 @@ mod tests { .post_comment(Comment::from("@bors delegate+").with_author(User::try_user())) .await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @"@user-with-try-privileges: :key: Insufficient privileges: not in review users" ); Ok(()) @@ -1113,7 +1113,7 @@ mod tests { .post_comment(review_comment("@bors delegate=try")) .await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r" :v: @default-user, you can now perform try builds on this pull request! @@ -1140,7 +1140,7 @@ mod tests { tester.expect_comments((), 1).await; tester.post_comment("@bors try").await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_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`. @@ -1167,7 +1167,7 @@ mod tests { tester.post_comment("@bors r+").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @"@default-user: :key: Insufficient privileges: not in review users" ); tester.get_pr_copy(()).await.expect_unapproved(); @@ -1207,7 +1207,7 @@ mod tests { .set_pr_status_draft(()) .await?; tester.post_comment("@bors r+").await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @":clipboard: Only open, non-draft PRs can be approved."); + insta::assert_snapshot!(tester.get_next_comment_text(()).await?, @":clipboard: Only open, non-draft PRs can be approved."); tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) @@ -1221,7 +1221,7 @@ mod tests { .set_pr_status_closed(()) .await?; tester.post_comment("@bors r+").await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @":clipboard: Only open, non-draft PRs can be approved."); + insta::assert_snapshot!(tester.get_next_comment_text(()).await?, @":clipboard: Only open, non-draft PRs can be approved."); tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) @@ -1235,7 +1235,7 @@ mod tests { .set_pr_status_merged(()) .await?; tester.post_comment("@bors r+").await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @":clipboard: Only open, non-draft PRs can be approved."); + insta::assert_snapshot!(tester.get_next_comment_text(()).await?, @":clipboard: Only open, non-draft PRs can be approved."); tester.get_pr_copy(()).await.expect_unapproved(); Ok(()) }) @@ -1251,7 +1251,7 @@ mod tests { }) .await?; tester.post_comment("@bors r+").await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_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. @@ -1280,7 +1280,7 @@ labels_blocking_approval = ["proposed-final-comment-period"] }) .await?; tester.post_comment("@bors r+").await?; - 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`."); + insta::assert_snapshot!(tester.get_next_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(()) }) @@ -1307,7 +1307,7 @@ labels_blocking_approval = ["proposed-final-comment-period", "final-comment-peri }) .await?; tester.post_comment("@bors r+").await?; - 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`."); + insta::assert_snapshot!(tester.get_next_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(()) }) @@ -1322,7 +1322,7 @@ labels_blocking_approval = ["proposed-final-comment-period", "final-comment-peri tester.get_pr_copy(()).await.expect_auto_build(|_| true); tester.workflow_start(tester.auto_branch().await).await?; tester.post_comment("@bors r-").await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_comment_text(()).await?, @r" Commit pr-1-sha has been unapproved. Auto build cancelled due to unapproval. Cancelled workflows: @@ -1346,7 +1346,7 @@ labels_blocking_approval = ["proposed-final-comment-period", "final-comment-peri tester.get_pr_copy(()).await.expect_auto_build(|_| true); tester.workflow_start(tester.auto_branch().await).await?; tester.post_comment("@bors r-").await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_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. diff --git a/src/bors/handlers/trybuild.rs b/src/bors/handlers/trybuild.rs index e94046b1..c06a9ec7 100644 --- a/src/bors/handlers/trybuild.rs +++ b/src/bors/handlers/trybuild.rs @@ -303,7 +303,7 @@ mod tests { tester.expect_comments((), 1).await; tester.workflow_full_success(tester.try_branch().await).await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_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`) @@ -323,7 +323,7 @@ mod tests { tester.expect_comments((), 1).await; tester.workflow_full_failure(tester.try_branch().await).await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @":broken_heart: Test for merge-0-pr-1 failed: [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1)" ); Ok(()) @@ -354,7 +354,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_text(()).await?, + tester.get_next_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: @@ -374,7 +374,7 @@ mod tests { .post_comment(Comment::from("@bors try").with_author(User::unprivileged())) .await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @"@unprivileged-user: :key: Insufficient privileges: not in try users" ); Ok(()) @@ -389,7 +389,7 @@ mod tests { .post_comment(Comment::from("@bors try").with_author(User::try_user())) .await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… @@ -406,7 +406,7 @@ mod tests { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors try").await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… @@ -549,7 +549,7 @@ try-job: Bar .await?; tester.expect_comments((), 1).await; tester.post_comment("@bors try parent=last").await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_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`. @@ -575,7 +575,7 @@ try-job: Bar tester .post_comment("@bors try parent=last") .await?; - 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."); + insta::assert_snapshot!(tester.get_next_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; @@ -588,7 +588,7 @@ try-job: Bar tester .post_comment("@bors try") .await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r###" + insta::assert_snapshot!(tester.get_next_comment_text(()).await?, @r###" :lock: Merge conflict This pull request and the base branch diverged in a way that cannot @@ -650,7 +650,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_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_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`. @@ -672,12 +672,12 @@ try-job: Bar "#, ) .await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_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_text(()).await?, @"Try build cancelled. Cancelled workflows:"); + insta::assert_snapshot!(tester.get_next_comment_text(()).await?, @"Try build cancelled. Cancelled workflows:"); Ok(()) }) .await; @@ -693,7 +693,7 @@ try-job: Bar .await?; tester.post_comment("@bors try").await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_comment_text(()).await?, @r" :hourglass: Trying commit pr-1-sha with merge merge-1-pr-1… (The previously running try build was automatically cancelled.) @@ -717,7 +717,7 @@ try-job: Bar tester.expect_comments((), 1).await; tester.post_comment("@bors try").await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_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`. @@ -729,7 +729,7 @@ try-job: Bar .with_check_suite_id(2), ) .await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r#" + insta::assert_snapshot!(tester.get_next_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`) @@ -744,7 +744,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_text(()).await?, @":exclamation: There is currently no try build in progress."); + insta::assert_snapshot!(tester.get_next_comment_text(()).await?, @":exclamation: There is currently no try build in progress."); Ok(()) }) .await; @@ -768,7 +768,7 @@ try-job: Bar )) .await?; tester.post_comment("@bors try cancel").await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_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 @@ -789,7 +789,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_text(()).await?, @"Try build was cancelled. It was not possible to cancel some workflows."); + insta::assert_snapshot!(tester.get_next_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(), @@ -843,7 +843,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_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_comment_text(()).await?, @r" Try build cancelled. Cancelled workflows: - https://github.com/rust-lang/borstest/actions/runs/3 "); @@ -881,7 +881,7 @@ try_failed = ["+foo", "+bar", "-baz"] )) .run_test(async |tester: &mut BorsTester| { tester.post_comment("@bors try").await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_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`. @@ -1036,7 +1036,7 @@ try_failed = ["+foo", "+bar", "-baz"] async fn hide_try_build_started_comment_after_success(pool: sqlx::PgPool) { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors try").await?; - let comment = tester.get_comment(()).await?; + let comment = tester.get_next_comment(()).await?; tester .workflow_full_success(tester.try_branch().await) .await?; @@ -1054,7 +1054,7 @@ try_failed = ["+foo", "+bar", "-baz"] async fn hide_try_build_started_comment_after_failure(pool: sqlx::PgPool) { run_test(pool, async |tester: &mut BorsTester| { tester.post_comment("@bors try").await?; - let comment = tester.get_comment(()).await?; + let comment = tester.get_next_comment(()).await?; tester .workflow_full_failure(tester.try_branch().await) .await?; @@ -1072,7 +1072,7 @@ try_failed = ["+foo", "+bar", "-baz"] 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?; + let comment = tester.get_next_comment(()).await?; // Hide the previous "Try build started" comment when we restart the build tester.post_comment("@bors try").await?; diff --git a/src/bors/handlers/workflow.rs b/src/bors/handlers/workflow.rs index a53783ad..332443d1 100644 --- a/src/bors/handlers/workflow.rs +++ b/src/bors/handlers/workflow.rs @@ -595,7 +595,7 @@ mod tests { tester.workflow_full_success(w2).await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r#" :sunny: Try build successful - [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1) :white_check_mark: @@ -625,7 +625,7 @@ mod tests { tester.workflow_event(WorkflowEvent::success(w1)).await?; tester.workflow_event(WorkflowEvent::success(w2)).await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r#" :sunny: Try build successful - [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1) :white_check_mark: @@ -654,7 +654,7 @@ mod tests { tester.workflow_event(WorkflowEvent::failure(w2)).await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_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(()) @@ -681,7 +681,7 @@ min_ci_time = 10 .with_duration(Duration::from_secs(1)), ) .await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r" + insta::assert_snapshot!(tester.get_next_comment_text(()).await?, @r" :broken_heart: Test for merge-0-pr-1 failed: [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1) A workflow was considered to be a failure because it took only `1s`. The minimum duration for CI workflows is configured to be `10s`. "); @@ -708,7 +708,7 @@ min_ci_time = 20 .with_duration(Duration::from_secs(100)), ) .await?; - insta::assert_snapshot!(tester.get_comment_text(()).await?, @r#" + insta::assert_snapshot!(tester.get_next_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`) diff --git a/src/bors/merge_queue.rs b/src/bors/merge_queue.rs index b61c2ec7..d298081e 100644 --- a/src/bors/merge_queue.rs +++ b/src/bors/merge_queue.rs @@ -520,7 +520,7 @@ merge_queue_enabled = false tester.approve(()).await?; tester.process_merge_queue().await; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @":hourglass: Testing commit pr-1-sha with merge merge-0-pr-1..." ); Ok(()) @@ -537,7 +537,7 @@ merge_queue_enabled = false tester.process_merge_queue().await; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r" :sunny: Test successful - [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1) Approved by: `default-user` @@ -557,7 +557,7 @@ merge_queue_enabled = false tester.workflow_full_failure(tester.auto_branch().await).await?; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @":broken_heart: Test for merge-0-pr-1 failed: [Workflow1](https://github.com/rust-lang/borstest/actions/runs/1)" ); Ok(()) @@ -699,7 +699,7 @@ merge_queue_enabled = false tester.workflow_full_success(tester.auto_branch().await).await?; tester.process_merge_queue().await; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @":eyes: Test was successful, but fast-forwarding failed: this PR has conflicts with the `main` branch" ); Ok(()) @@ -720,7 +720,7 @@ merge_queue_enabled = false tester.workflow_full_success(tester.auto_branch().await).await?; tester.process_merge_queue().await; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @":eyes: Test was successful, but fast-forwarding failed: the tested commit was behind the `main` branch" ); Ok(()) @@ -744,7 +744,7 @@ merge_queue_enabled = false .await?; tester.process_merge_queue().await; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @":eyes: Test was successful, but fast-forwarding failed: IO error" ); Ok(()) @@ -816,7 +816,7 @@ merge_queue_enabled = false tester.approve(()).await?; tester.process_merge_queue().await; insta::assert_snapshot!( - tester.get_comment_text(()).await?, + tester.get_next_comment_text(()).await?, @r#" :lock: Merge conflict diff --git a/src/tests/mocks/mod.rs b/src/tests/mocks/mod.rs index 26159cdc..0c5999de 100644 --- a/src/tests/mocks/mod.rs +++ b/src/tests/mocks/mod.rs @@ -103,7 +103,7 @@ impl GitHubState { /// events to arrive from the bors service. /// As such, it has to be written carefully to avoid holding GH/repo locks that are also /// acquired by dynamic HTTP mock handlers. - pub async fn get_comment>( + pub async fn get_next_comment>( state: Arc>, id: Id, ) -> anyhow::Result { diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 23ad882c..4cf5ec0b 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -364,18 +364,21 @@ impl BorsTester { /// Wait until the next bot comment is received on the specified repo and PR, and return its /// text. - pub async fn get_comment_text>( + pub async fn get_next_comment_text>( &mut self, id: Id, ) -> anyhow::Result { - Ok(GitHubState::get_comment(self.github.clone(), id) + Ok(GitHubState::get_next_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 + pub async fn get_next_comment>( + &mut self, + id: Id, + ) -> anyhow::Result { + GitHubState::get_next_comment(self.github.clone(), id).await } //-- Generation of GitHub events --// @@ -741,7 +744,7 @@ impl BorsTester { pub async fn start_auto_build>(&mut self, id: Id) -> anyhow::Result<()> { let id = id.into(); self.process_merge_queue().await; - let comment = self.get_comment_text(id).await?; + let comment = self.get_next_comment_text(id).await?; assert!(comment.contains("Testing commit")); Ok(()) } @@ -754,7 +757,7 @@ impl BorsTester { ) -> anyhow::Result<()> { self.workflow_full_success(self.auto_branch().await).await?; self.process_merge_queue().await; - let comment = self.get_comment_text(id).await?; + let comment = self.get_next_comment_text(id).await?; assert!(comment.contains("Test successful")); Ok(()) } @@ -775,7 +778,7 @@ impl BorsTester { let id = id.into(); for i in 0..count { let id = id.clone(); - self.get_comment_text(id) + self.get_next_comment_text(id) .await .unwrap_or_else(|_| panic!("Failed to get comment #{i}")); } @@ -852,7 +855,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_text] to synchronize + /// Prefer using [BorsTester::expect_comments] or [BorsTester::get_next_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 From 6e17c20032490a1504ac790e7ac3cf4bf1a6de00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Mon, 22 Sep 2025 21:15:52 +0200 Subject: [PATCH 3/5] Refactor comment hiding mock --- src/tests/mocks/comment.rs | 14 ++++---- src/tests/mocks/github.rs | 7 ++-- src/tests/mocks/mod.rs | 61 ++++++++++++++++++--------------- src/tests/mocks/pull_request.rs | 2 +- src/tests/mod.rs | 2 +- 5 files changed, 46 insertions(+), 40 deletions(-) diff --git a/src/tests/mocks/comment.rs b/src/tests/mocks/comment.rs index da4f2800..5d760464 100644 --- a/src/tests/mocks/comment.rs +++ b/src/tests/mocks/comment.rs @@ -1,3 +1,8 @@ +use crate::github::api::client::HideCommentReason; +use crate::tests::mocks::User; +use crate::tests::mocks::pull_request::PrIdentifier; +use crate::tests::mocks::repository::GitHubRepository; +use crate::tests::mocks::user::GitHubUser; use chrono::Utc; use octocrab::models::events::payload::{IssueCommentEventAction, IssueCommentEventChanges}; use octocrab::models::issues::IssueStateReason; @@ -5,18 +10,14 @@ use octocrab::models::{Author, CommentId, IssueId, IssueState, Label}; use serde::Serialize; use url::Url; -use crate::tests::mocks::User; -use crate::tests::mocks::pull_request::PrIdentifier; -use crate::tests::mocks::repository::GitHubRepository; -use crate::tests::mocks::user::GitHubUser; - -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq)] pub struct Comment { pub pr_ident: PrIdentifier, pub author: User, pub content: String, pub id: Option, pub node_id: Option, + pub hide_reason: Option, } impl Comment { @@ -27,6 +28,7 @@ impl Comment { content: content.to_string(), id: None, node_id: None, + hide_reason: None, } } diff --git a/src/tests/mocks/github.rs b/src/tests/mocks/github.rs index f4f33648..0efc2dae 100644 --- a/src/tests/mocks/github.rs +++ b/src/tests/mocks/github.rs @@ -8,9 +8,9 @@ use crate::create_github_client; use crate::github::api::client::HideCommentReason; use crate::tests::GitHubState; use crate::tests::mocks::app::{AppHandler, default_app_id}; +use crate::tests::mocks::dynamic_mock_req; use crate::tests::mocks::pull_request::CommentMsg; use crate::tests::mocks::repository::{mock_repo, mock_repo_list}; -use crate::tests::mocks::{HiddenComment, dynamic_mock_req}; pub struct GitHubMockServer { mock_server: MockServer, @@ -135,10 +135,7 @@ 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_hidden_comment(HiddenComment { - node_id: data.node_id, - reason: data.reason, - }); + github.blocking_lock().modify_comment(&data.node_id, |c| c.hide_reason = Some(data.reason)); }) .join() .unwrap(); diff --git a/src/tests/mocks/mod.rs b/src/tests/mocks/mod.rs index 0c5999de..65c20f9b 100644 --- a/src/tests/mocks/mod.rs +++ b/src/tests/mocks/mod.rs @@ -26,16 +26,10 @@ pub mod repository; pub mod user; pub mod workflow; -#[derive(Debug)] -pub struct HiddenComment { - pub node_id: String, - pub reason: HideCommentReason, -} - /// Represents the state of GitHub. pub struct GitHubState { pub(super) repos: HashMap>>, - hidden_comments: Vec, + comments: HashMap, } impl GitHubState { @@ -70,8 +64,8 @@ impl GitHubState { self } - pub fn add_hidden_comment(&mut self, comment: HiddenComment) { - self.hidden_comments.push(comment); + pub fn modify_comment(&mut self, node_id: &str, func: F) { + func(self.comments.get_mut(node_id).unwrap()); } pub fn check_sha_history(&self, repo: GithubRepoName, branch: &str, expected_shas: &[&str]) { @@ -166,16 +160,27 @@ impl GitHubState { { let mut gh_state = state.lock().await; - let repo = gh_state - .repos - .get_mut(&id.repo) - .unwrap_or_else(|| panic!("Repository `{}` not found", id.repo)); - let mut repo = repo.lock(); - let pr = repo - .pull_requests - .get_mut(&id.number) - .expect("Pull request not found"); - pr.add_comment_to_history(comment.clone()); + { + let repo = gh_state + .repos + .get_mut(&id.repo) + .unwrap_or_else(|| panic!("Repository `{}` not found", id.repo)); + let mut repo = repo.lock(); + let pr = repo + .pull_requests + .get_mut(&id.number) + .expect("Pull request not found"); + pr.add_comment_to_history(comment.clone()); + } + + assert_eq!( + gh_state.comments.insert( + comment.node_id.clone().expect("Comment without node ID"), + comment.clone() + ), + None, + "Duplicated comment {comment:?}" + ); } eprintln!( @@ -185,13 +190,15 @@ impl GitHubState { Ok(comment) } - pub fn check_hidden_comment(&self, comment: &Comment, reason: HideCommentReason) { - assert!( - self.hidden_comments - .iter() - .any(|c| &c.node_id == comment.node_id.as_ref().unwrap() && c.reason == reason), - "Comment {comment:?} was not hidden with reason {reason:?}.\nHidden comments: {:?}", - self.hidden_comments + pub fn check_comment_was_hidden(&self, node_id: &str, reason: HideCommentReason) { + let comment = self + .comments + .get(node_id) + .expect("Comment with node_id {node_id} was not modified through GraphQL"); + assert_eq!( + comment.hide_reason, + Some(reason), + "Comment {comment:?} was not hidden with reason {reason:?}." ); } } @@ -201,7 +208,7 @@ impl Default for GitHubState { let repo = Repo::default(); Self { repos: HashMap::from([(repo.name.clone(), Arc::new(Mutex::new(repo)))]), - hidden_comments: Default::default(), + comments: Default::default(), } } } diff --git a/src/tests/mocks/pull_request.rs b/src/tests/mocks/pull_request.rs index 1e735485..c75ac731 100644 --- a/src/tests/mocks/pull_request.rs +++ b/src/tests/mocks/pull_request.rs @@ -34,7 +34,7 @@ pub fn default_pr_number() -> u64 { /// - `()`, which uses the default repo and default PR number. /// - A PR number, which uses the default repository. /// - A tuple (, ). -#[derive(Clone, Debug)] +#[derive(Clone, Debug, PartialEq)] pub struct PrIdentifier { pub repo: GithubRepoName, pub number: u64, diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 4cf5ec0b..c5bf2aeb 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -789,7 +789,7 @@ impl BorsTester { self.github .lock() .await - .check_hidden_comment(comment, reason); + .check_comment_was_hidden(&comment.node_id.as_ref().unwrap(), reason); } pub async fn expect_check_run( From 6d987da180a4bb3e10902baca2af6ab458393491 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Mon, 22 Sep 2025 21:29:23 +0200 Subject: [PATCH 4/5] Add a test for adding workflow link(s) to try build started comment --- src/bors/comment.rs | 2 +- src/bors/handlers/trybuild.rs | 28 ++++++++++++++++++++++++++++ src/tests/mocks/github.rs | 29 ++++++++++++++++++++++++++--- src/tests/mocks/mod.rs | 4 ++++ src/tests/mod.rs | 9 +++++++++ 5 files changed, 68 insertions(+), 4 deletions(-) diff --git a/src/bors/comment.rs b/src/bors/comment.rs index 4fba8fd2..35844703 100644 --- a/src/bors/comment.rs +++ b/src/bors/comment.rs @@ -199,7 +199,7 @@ pub fn try_build_started_comment( } pub fn append_workflow_links_to_comment(comment_content: &mut String, workflow_links: Vec) { - comment_content.push_str("\n\n**Workflows**:\n\n"); + comment_content.push_str("\n**Workflows**:\n\n"); for link in workflow_links { comment_content.push_str(&format!("- {link}\n")); diff --git a/src/bors/handlers/trybuild.rs b/src/bors/handlers/trybuild.rs index c06a9ec7..e0667090 100644 --- a/src/bors/handlers/trybuild.rs +++ b/src/bors/handlers/trybuild.rs @@ -1085,4 +1085,32 @@ try_failed = ["+foo", "+bar", "-baz"] }) .await; } + + #[sqlx::test] + async fn update_try_build_started_comment_after_workflow_starts(pool: sqlx::PgPool) { + run_test(pool, async |tester: &mut BorsTester| { + tester.post_comment("@bors try").await?; + let comment = tester.get_next_comment(()).await?; + + tester.workflow_start(tester.try_branch().await).await?; + + // Check that the comment text has been updated with a link to the started workflow + let updated_comment = tester + .get_comment_by_node_id(&comment.node_id.unwrap()) + .await + .unwrap(); + insta::assert_snapshot!(updated_comment.content, @r" + :hourglass: Trying commit pr-1-sha with merge merge-0-pr-1… + + To cancel the try build, run the command `@bors try cancel`. + + **Workflows**: + + - https://github.com/rust-lang/borstest/actions/runs/1 + "); + + Ok(()) + }) + .await; + } } diff --git a/src/tests/mocks/github.rs b/src/tests/mocks/github.rs index 0efc2dae..9fb35631 100644 --- a/src/tests/mocks/github.rs +++ b/src/tests/mocks/github.rs @@ -135,7 +135,9 @@ 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().modify_comment(&data.node_id, |c| c.hide_reason = Some(data.reason)); + github + .blocking_lock() + .modify_comment(&data.node_id, |c| c.hide_reason = Some(data.reason)); }) .join() .unwrap(); @@ -158,8 +160,18 @@ async fn mock_graphql(github: Arc>, mock_server: } }); + let github = github.clone(); + std::thread::spawn(move || { + github + .blocking_lock() + .modify_comment(&data.id, |c| c.content = data.body); + }) + .join() + .unwrap(); + ResponseTemplate::new(200).set_body_json(response) } + // Get comment content "node" => { #[derive(serde::Deserialize)] struct Variables { @@ -168,10 +180,21 @@ async fn mock_graphql(github: Arc>, mock_server: let data: Variables = serde_json::from_value(body.variables).unwrap(); + let github = github.clone(); + let comment_text = std::thread::spawn(move || { + github + .blocking_lock() + .get_comment_by_node_id(&data.node_id) + .unwrap() + .content + .clone() + }) + .join() + .unwrap(); let response = serde_json::json!({ "node": { - "__typename": "IssueComment", - "body":format!(":hourglass: Trying commit {} \n To cancel the try build, run the command @bors try cancel`.", data.node_id) + "__typename": "IssueComment", + "body": comment_text } }); ResponseTemplate::new(200).set_body_json(response) diff --git a/src/tests/mocks/mod.rs b/src/tests/mocks/mod.rs index 65c20f9b..903e506f 100644 --- a/src/tests/mocks/mod.rs +++ b/src/tests/mocks/mod.rs @@ -190,6 +190,10 @@ impl GitHubState { Ok(comment) } + pub fn get_comment_by_node_id(&self, node_id: &str) -> Option<&Comment> { + self.comments.get(node_id) + } + pub fn check_comment_was_hidden(&self, node_id: &str, reason: HideCommentReason) { let comment = self .comments diff --git a/src/tests/mod.rs b/src/tests/mod.rs index c5bf2aeb..0285b51c 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -897,6 +897,15 @@ impl BorsTester { result } + /// Get a GitHub comment that might have been modified by API calls from bors. + pub async fn get_comment_by_node_id(&self, node_id: &str) -> Option { + self.github + .lock() + .await + .get_comment_by_node_id(node_id) + .cloned() + } + //-- Internal helper functions --/ async fn webhook_comment(&mut self, comment: Comment) -> anyhow::Result<()> { self.send_webhook( From 4e9f6497b1bc1871d0f9379d4b74faf5252234c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Mon, 22 Sep 2025 22:25:19 +0200 Subject: [PATCH 5/5] Fix GraphQL calls --- src/github/api/client.rs | 99 +++++++++++++++++++-------------------- src/tests/mocks/github.rs | 16 +++---- src/tests/mod.rs | 2 +- 3 files changed, 57 insertions(+), 60 deletions(-) diff --git a/src/github/api/client.rs b/src/github/api/client.rs index 0b9c85b4..b7d372ac 100644 --- a/src/github/api/client.rs +++ b/src/github/api/client.rs @@ -457,12 +457,31 @@ impl GithubRepositoryClient { variables: V, } + #[derive(serde::Deserialize, Debug)] + struct Error { + #[allow(unused)] + message: String, + } + + #[derive(serde::Deserialize)] + struct RawResponse { + errors: Option>, + #[serde(flatten)] + result: T, + } + let response = self .client - .graphql::(&Payload { query, variables }) + .graphql::>(&Payload { query, variables }) .await .context("GraphQL request failed")?; - Ok(response) + + let errors = response.errors.unwrap_or_default(); + if !errors.is_empty() { + Err(anyhow::anyhow!("Query ended with error(s): {errors:?}")) + } else { + Ok(response.result) + } } /// Hides a comment on an Issue, Commit, Pull Request, or Gist. @@ -517,6 +536,11 @@ impl GithubRepositoryClient { #[derive(Deserialize)] struct Output { + data: OutputInner, + } + + #[derive(Deserialize)] + struct OutputInner { node: Option, } @@ -527,17 +551,16 @@ impl GithubRepositoryClient { tracing::debug!(node_id, "Fetching comment content"); - let output: Output = - perform_retryable("get_comment_content", RetryMethod::default(), || async { - self.graphql::(QUERY, Variables { node_id }) - .await - .context("Failed to fetch comment content") - }) - .await?; + let output = perform_retryable("get_comment_content", RetryMethod::default(), || async { + self.graphql::(QUERY, Variables { node_id }) + .await + .context("Failed to fetch comment content") + }) + .await?; - match output.node { + match output.data.node { Some(comment) => Ok(comment.body), - None => anyhow::bail!("No comment found for node_id: {}", node_id), + None => anyhow::bail!("No comment found for node_id: {node_id}"), } } pub async fn update_comment_content( @@ -547,10 +570,9 @@ impl GithubRepositoryClient { ) -> anyhow::Result<()> { const QUERY: &str = r#" mutation($id: ID!, $body: String!) { - updateComment(input: {id: $id, body: $body}) { - comment { + updateIssueComment(input: {id: $id, body: $body}) { + issueComment { id - body } } } @@ -562,44 +584,21 @@ impl GithubRepositoryClient { body: &'a str, } - #[derive(Deserialize)] - struct Output { - update_comment: Option, - } - - #[derive(Deserialize)] - struct UpdatedComment { - comment: CommentNode, - } - - #[derive(Deserialize)] - struct CommentNode { - body: String, - } - tracing::debug!(node_id, "Updating comment content"); - let output: Output = - perform_retryable("update_comment_content", RetryMethod::default(), || async { - self.graphql::( - QUERY, - Variables { - id: node_id, - body: new_body, - }, - ) - .await - .context("Failed to update comment") - }) - .await?; - - match output.update_comment { - Some(updated_comment) => { - tracing::debug!("Updated comment content: {}", updated_comment.comment.body); - Ok(()) - } - None => anyhow::bail!("Failed to update comment: {node_id}"), - } + perform_retryable("update_comment_content", RetryMethod::default(), || async { + self.graphql::<(), Variables>( + QUERY, + Variables { + id: node_id, + body: new_body, + }, + ) + .await + .context("Failed to update comment") + }) + .await?; + Ok(()) } } diff --git a/src/tests/mocks/github.rs b/src/tests/mocks/github.rs index 9fb35631..7ee9ecd7 100644 --- a/src/tests/mocks/github.rs +++ b/src/tests/mocks/github.rs @@ -143,7 +143,7 @@ async fn mock_graphql(github: Arc>, mock_server: .unwrap(); ResponseTemplate::new(200).set_body_json(HashMap::::new()) } - "updateComment" => { + "updateIssueComment" => { #[derive(serde::Deserialize)] struct Variables { id: String, @@ -152,11 +152,8 @@ async fn mock_graphql(github: Arc>, mock_server: let data: Variables = serde_json::from_value(body.variables).unwrap(); let response = serde_json::json!({ - "update_comment": { - "comment": { - "id": data.id, - "body": data.body - } + "issueComment": { + "id": data.id, } }); @@ -192,9 +189,10 @@ async fn mock_graphql(github: Arc>, mock_server: .join() .unwrap(); let response = serde_json::json!({ - "node": { - "__typename": "IssueComment", - "body": comment_text + "data": { + "node": { + "body": comment_text + } } }); ResponseTemplate::new(200).set_body_json(response) diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 0285b51c..07df1b09 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -789,7 +789,7 @@ impl BorsTester { self.github .lock() .await - .check_comment_was_hidden(&comment.node_id.as_ref().unwrap(), reason); + .check_comment_was_hidden(comment.node_id.as_deref().unwrap(), reason); } pub async fn expect_check_run(