Merge pull request #365 from Kobzol/test-failed-job-list

Show a sample of failed jobs when a build fails

Author: Kobzol

src/bors/comment.rs

@@ -1,7 +1,12 @@
+use itertools::Itertools;
+use octocrab::models::workflows::Job;
 use serde::Serialize;
 
+use crate::bors::FailedWorkflowRun;
 use crate::bors::command::CommandPrefix;
 use crate::database::BuildModel;
+use crate::github::GithubRepoName;
+use crate::utils::text::pluralize;
 use crate::{
     database::{WorkflowModel, WorkflowStatus},
     github::CommitSha,
@@ -29,13 +34,14 @@ impl Comment {
 
     pub fn render(&self) -> String {
         if let Some(metadata) = &self.metadata {
-            return format!(
+            format!(
                 "{}\n<!-- homu: {} -->",
                 self.text,
                 serde_json::to_string(metadata).unwrap()
-            );
+            )
+        } else {
+            self.text.clone()
         }
-        self.text.clone()
     }
 }
 
@@ -94,13 +100,57 @@ pub fn try_build_cancelled_comment(workflow_urls: impl Iterator<Item = String>)
     Comment::new(try_build_cancelled_comment)
 }
 
-pub fn workflow_failed_comment(workflows: &[WorkflowModel]) -> Comment {
-    let workflows_status = list_workflows_status(workflows);
-    Comment::new(format!(
-        r#":broken_heart: Test failed
-{}"#,
-        workflows_status
-    ))
+pub fn build_failed_comment(
+    repo: &GithubRepoName,
+    failed_workflows: Vec<FailedWorkflowRun>,
+) -> Comment {
+    use std::fmt::Write;
+
+    let mut msg = ":broken_heart: Test failed".to_string();
+    let mut workflow_links = failed_workflows
+        .iter()
+        .map(|w| format!("[{}]({})", w.workflow_run.name, w.workflow_run.url));
+    if !failed_workflows.is_empty() {
+        write!(msg, " ({})", workflow_links.join(", ")).unwrap();
+
+        let mut failed_jobs: Vec<Job> = failed_workflows
+            .into_iter()
+            .flat_map(|w| w.failed_jobs)
+            .collect();
+        failed_jobs.sort_by(|l, r| l.name.cmp(&r.name));
+
+        if !failed_jobs.is_empty() {
+            write!(msg, ". Failed {}:\n\n", pluralize("job", failed_jobs.len())).unwrap();
+
+            let max_jobs_to_show = 5;
+            for job in failed_jobs.iter().take(max_jobs_to_show) {
+                let logs_url = job.html_url.to_string();
+                let extended_logs_url = format!(
+                    "https://triage.rust-lang.org/gha-logs/{}/{}/{}",
+                    repo.owner(),
+                    repo.name(),
+                    job.id
+                );
+                writeln!(
+                    msg,
+                    "- `{}` ([web logs]({}), [extended logs]({}))",
+                    job.name, logs_url, extended_logs_url
+                )
+                .unwrap();
+            }
+            if failed_jobs.len() > max_jobs_to_show {
+                let remaining = failed_jobs.len() - max_jobs_to_show;
+                writeln!(
+                    msg,
+                    "- (and {remaining} other {})",
+                    pluralize("job", remaining)
+                )
+                .unwrap();
+            }
+        }
+    }
+
+    Comment::new(msg)
 }
 
 pub fn try_build_started_comment(

src/bors/handlers/trybuild.rs

@@ -10,7 +10,6 @@ use crate::bors::comment::{
     cant_find_last_parent_comment, merge_conflict_comment, try_build_started_comment,
 };
 use crate::bors::handlers::labels::handle_label_trigger;
-use crate::database::RunId;
 use crate::database::{BuildModel, BuildStatus, PullRequestModel};
 use crate::github::GithubRepoName;
 use crate::github::api::client::GithubRepositoryClient;
@@ -20,6 +19,7 @@ use crate::permissions::PermissionType;
 use crate::utils::text::suppress_github_mentions;
 use anyhow::{Context, anyhow};
 use itertools::Itertools;
+use octocrab::models::CheckRunId;
 use octocrab::params::checks::CheckRunConclusion;
 use octocrab::params::checks::CheckRunOutput;
 use octocrab::params::checks::CheckRunStatus;
@@ -312,8 +312,12 @@ pub async fn cancel_build_workflows(
     db: &PgDbClient,
     build: &BuildModel,
     check_run_conclusion: CheckRunConclusion,
-) -> anyhow::Result<Vec<RunId>> {
+) -> anyhow::Result<Vec<octocrab::models::RunId>> {
     let pending_workflows = db.get_pending_workflows_for_build(build).await?;
+    let pending_workflows: Vec<octocrab::models::RunId> = pending_workflows
+        .into_iter()
+        .map(|id| octocrab::models::RunId(id.0))
+        .collect();
 
     tracing::info!("Cancelling workflows {:?}", pending_workflows);
     client.cancel_workflows(&pending_workflows).await?;
@@ -324,7 +328,7 @@ pub async fn cancel_build_workflows(
     if let Some(check_run_id) = build.check_run_id {
         if let Err(error) = client
             .update_check_run(
-                check_run_id as u64,
+                CheckRunId(check_run_id as u64),
                 CheckRunStatus::Completed,
                 Some(check_run_conclusion),
                 None,
@@ -413,9 +417,10 @@ mod tests {
     use crate::github::CommitSha;
     use crate::tests::BorsTester;
     use crate::tests::mocks::{
-        BorsBuilder, Comment, GitHubState, User, WorkflowEvent, WorkflowRunData, default_pr_number,
-        default_repo_name, run_test,
+        BorsBuilder, Comment, GitHubState, User, WorkflowEvent, WorkflowJob, WorkflowRunData,
+        default_pr_number, default_repo_name, run_test,
     };
+    use octocrab::models::JobId;
     use octocrab::params::checks::{CheckRunConclusion, CheckRunStatus};
 
     #[sqlx::test]
@@ -446,16 +451,49 @@ mod tests {
             tester.workflow_full_failure(tester.try_branch()).await?;
             insta::assert_snapshot!(
                 tester.get_comment().await?,
-                @r###"
-            :broken_heart: Test failed
-            - [Workflow1](https://github.com/workflows/Workflow1/1) :x:
-            "###
+                @":broken_heart: Test failed ([Workflow1](https://github.com/workflows/Workflow1/1))"
             );
             Ok(())
         })
         .await;
     }
 
+    #[sqlx::test]
+    async fn try_failure_job(pool: sqlx::PgPool) {
+        run_test(pool, async |tester: &mut BorsTester| {
+            tester.post_comment("@bors try").await?;
+            tester.expect_comments(1).await;
+
+            let mut workflow = WorkflowRunData::from(tester.try_branch());
+            workflow.jobs.push(WorkflowJob {
+                id: JobId(42),
+                status: WorkflowStatus::Failure,
+            });
+            workflow.jobs.push(WorkflowJob {
+                id: JobId(50),
+                status: WorkflowStatus::Failure,
+            });
+            workflow.jobs.push(WorkflowJob {
+                id: JobId(51),
+                status: WorkflowStatus::Success,
+            });
+
+            tester.default_repo().lock().update_workflow_run(workflow.clone(), WorkflowStatus::Pending);
+            tester.workflow_full_failure(workflow).await?;
+            insta::assert_snapshot!(
+                tester.get_comment().await?,
+                @r"
+            :broken_heart: Test failed ([Workflow1](https://github.com/workflows/Workflow1/1)). Failed jobs:
+
+            - `Job 42` ([web logs](https://github.com/job-logs/42), [extended logs](https://triage.rust-lang.org/gha-logs/rust-lang/borstest/42))
+            - `Job 50` ([web logs](https://github.com/job-logs/50), [extended logs](https://triage.rust-lang.org/gha-logs/rust-lang/borstest/50))
+            "
+            );
+            Ok(())
+        })
+            .await;
+    }
+
     #[sqlx::test]
     async fn try_no_permissions(pool: sqlx::PgPool) {
         run_test(pool, async |tester| {

src/bors/handlers/workflow.rs

@@ -1,19 +1,19 @@
-use std::sync::Arc;
-use std::time::Duration;
-
-use octocrab::params::checks::CheckRunConclusion;
-use octocrab::params::checks::CheckRunStatus;
-
 use crate::PgDbClient;
-use crate::bors::comment::{try_build_succeeded_comment, workflow_failed_comment};
+use crate::bors::comment::{build_failed_comment, try_build_succeeded_comment};
 use crate::bors::event::{WorkflowRunCompleted, WorkflowRunStarted};
 use crate::bors::handlers::is_bors_observed_branch;
 use crate::bors::handlers::labels::handle_label_trigger;
 use crate::bors::merge_queue::AUTO_BRANCH_NAME;
 use crate::bors::merge_queue::MergeQueueSender;
-use crate::bors::{RepositoryState, WorkflowRun};
-use crate::database::{BuildStatus, WorkflowStatus};
+use crate::bors::{FailedWorkflowRun, RepositoryState, WorkflowRun};
+use crate::database::{BuildStatus, WorkflowModel, WorkflowStatus};
 use crate::github::LabelTrigger;
+use octocrab::models::CheckRunId;
+use octocrab::models::workflows::{Conclusion, Job, Status};
+use octocrab::params::checks::CheckRunConclusion;
+use octocrab::params::checks::CheckRunStatus;
+use std::sync::Arc;
+use std::time::Duration;
 
 pub(super) async fn handle_workflow_started(
     db: Arc<PgDbClient>,
@@ -203,20 +203,40 @@ async fn maybe_complete_build(
 
         if let Err(error) = repo
             .client
-            .update_check_run(check_run_id as u64, status, conclusion, None)
+            .update_check_run(CheckRunId(check_run_id as u64), status, conclusion, None)
             .await
         {
             tracing::error!("Could not update check run {check_run_id}: {error:?}");
         }
     }
 
     db_workflow_runs.sort_by(|a, b| a.name.cmp(&b.name));
+
     let message = if !has_failure {
-        tracing::info!("Workflow succeeded");
+        tracing::info!("Build succeeded");
         try_build_succeeded_comment(&db_workflow_runs, payload.commit_sha, &build)
     } else {
-        tracing::info!("Workflow failed");
-        workflow_failed_comment(&db_workflow_runs)
+        // Download failed jobs
+        let mut workflow_runs: Vec<FailedWorkflowRun> = vec![];
+        for workflow_run in db_workflow_runs {
+            let failed_jobs = match get_failed_jobs(repo, &workflow_run).await {
+                Ok(jobs) => jobs,
+                Err(error) => {
+                    tracing::error!(
+                        "Cannot download jobs for workflow run {}: {error:?}",
+                        workflow_run.run_id
+                    );
+                    vec![]
+                }
+            };
+            workflow_runs.push(FailedWorkflowRun {
+                workflow_run,
+                failed_jobs,
+            })
+        }
+
+        tracing::info!("Build failed");
+        build_failed_comment(repo.repository(), workflow_runs)
     };
     repo.client.post_comment(pr.number, message).await?;
 
@@ -228,6 +248,29 @@ async fn maybe_complete_build(
     Ok(())
 }
 
+/// Return failed jobs from the given workflow run.
+async fn get_failed_jobs(
+    repo: &RepositoryState,
+    workflow_run: &WorkflowModel,
+) -> anyhow::Result<Vec<Job>> {
+    let jobs = repo
+        .client
+        .get_jobs_for_workflow_run(workflow_run.run_id.into())
+        .await?;
+    Ok(jobs
+        .into_iter()
+        .filter(|j| {
+            j.status == Status::Failed || {
+                j.status == Status::Completed
+                    && matches!(
+                        j.conclusion,
+                        Some(Conclusion::Failure | Conclusion::Cancelled | Conclusion::TimedOut)
+                    )
+            }
+        })
+        .collect())
+}
+
 #[cfg(test)]
 mod tests {
     use crate::database::WorkflowStatus;
@@ -376,11 +419,7 @@ mod tests {
             tester.workflow_event(WorkflowEvent::failure(w2)).await?;
             insta::assert_snapshot!(
                 tester.get_comment().await?,
-                @r"
-            :broken_heart: Test failed
-            - [Workflow1](https://github.com/workflows/Workflow1/1) :question:
-            - [Workflow1](https://github.com/workflows/Workflow1/2) :x:
-            "
+                @":broken_heart: Test failed ([Workflow1](https://github.com/workflows/Workflow1/1), [Workflow1](https://github.com/workflows/Workflow1/2))"
             );
             Ok(())
         })

src/bors/mod.rs

@@ -8,6 +8,7 @@ pub use comment::Comment;
 pub use context::BorsContext;
 pub use handlers::{handle_bors_global_event, handle_bors_repository_event};
 use octocrab::models::RunId;
+use octocrab::models::workflows::Job;
 use serde::Serialize;
 
 use crate::config::RepositoryConfig;
@@ -25,7 +26,7 @@ mod handlers;
 pub mod merge_queue;
 pub mod mergeable_queue;
 
-use crate::database::WorkflowStatus;
+use crate::database::{WorkflowModel, WorkflowStatus};
 pub use command::CommandPrefix;
 
 #[cfg(test)]
@@ -47,6 +48,11 @@ pub struct WorkflowRun {
     pub status: WorkflowStatus,
 }
 
+pub struct FailedWorkflowRun {
+    pub workflow_run: WorkflowModel,
+    pub failed_jobs: Vec<Job>,
+}
+
 /// An access point to a single repository.
 /// Can be used to query permissions for the repository, and also to perform various
 /// actions using the stored client.