Update comment on benchmark failure (#42)

Cargo.toml

@@ -33,6 +33,7 @@ serde_json = "1.0"
 libsystemd = "0.7.0"
 
 [dev-dependencies]
+rstest = "0.19.0"
 assert-json-diff = "2.0.2"
 wiremock = "0.6.0"
 temp-env = "0.3.6"

src/benchmark.rs

@@ -19,13 +19,20 @@ impl EnvSpecs {
     pub fn args(&self) -> Vec<&str> {
         self.0
             .iter()
-            .flat_map(|env_spec| ["-E", &env_spec])
+            .flat_map(|env_spec| ["-E", env_spec])
             .collect()
     }
 }
 
+#[derive(Debug, Clone)]
+pub(crate) struct RunResult {
+    pub success: bool,
+    pub wd: PathBuf,
+    pub env_specs: EnvSpecs,
+}
+
 /// Sync repo to match remote’s branch, and run ASV afterwards.
-pub(crate) async fn sync_repo_and_run<R>(req: &R) -> Result<(PathBuf, EnvSpecs)>
+pub(crate) async fn sync_repo_and_run<R>(req: &R) -> Result<RunResult>
 where
     R: RunConfig + Send + Sync + Clone,
 {
@@ -129,7 +136,7 @@ async fn resolve_env_from_stdout(command: &mut Command) -> Result<Vec<String>> {
     Ok(parsed)
 }
 
-async fn run_benchmark(repo: git2::Repository, on: &[String]) -> Result<(PathBuf, EnvSpecs)> {
+async fn run_benchmark(repo: git2::Repository, on: &[String]) -> Result<RunResult> {
     let wd = {
         let on = on.to_owned();
         tokio::task::spawn_blocking(move || fetch_configured_refs(&repo, &on)).await??
@@ -142,9 +149,11 @@ async fn run_benchmark(repo: git2::Repository, on: &[String]) -> Result<(PathBuf
         .spawn()?
         .wait()
         .await?;
-    if result.code() != Some(0) {
-        bail!("asv run --bench=just-discover exited with {result}");
-    }
+    let success = match result.code() {
+        Some(0) => true,
+        Some(2) => false,
+        _ => bail!("asv run --bench=just-discover exited with {result}"),
+    };
 
     tracing::info!("Running asv in {}", wd.display());
     let mut command = asv_command(&wd);
@@ -171,7 +180,11 @@ async fn run_benchmark(repo: git2::Repository, on: &[String]) -> Result<(PathBuf
         bail!("asv run exited with {result}");
     }
 
-    Ok((wd, env_specs))
+    Ok(RunResult {
+        success,
+        wd,
+        env_specs,
+    })
 }
 
 #[derive(Deserialize)]

src/main.rs

@@ -1,6 +1,7 @@
 #![warn(clippy::pedantic)]
 
-use anyhow::Result;
+use anyhow::{bail, Result};
+use benchmark::RunResult;
 use clap::Parser;
 
 mod benchmark;
@@ -30,14 +31,21 @@ async fn main() -> Result<()> {
             server::serve(args).await?;
         }
         cli::Commands::Run(args) => {
-            let (wd, env_specs) = benchmark::sync_repo_and_run(&args).await?;
+            let RunResult {
+                success,
+                wd,
+                env_specs,
+            } = benchmark::sync_repo_and_run(&args).await?;
             // if exactly two are specified, show a comparison
             if let [before, after] = args.run_on.as_slice() {
                 benchmark::AsvCompare::new(&wd, before, after)
                     .in_envs(env_specs)
                     .run()
                     .await?;
             }
+            if !success {
+                bail!("Benchmark run failed");
+            }
         }
     }
     Ok(())

src/server/runner.rs

@@ -1,10 +1,8 @@
-use std::path::Path;
-
 use anyhow::Result;
 use futures::{channel::mpsc::Receiver, StreamExt};
 use tracing::Instrument;
 
-use crate::benchmark::{sync_repo_and_run, AsvCompare, EnvSpecs};
+use crate::benchmark::{sync_repo_and_run, AsvCompare, RunResult};
 use crate::constants::ORG;
 use crate::event::{Compare, Event};
 
@@ -39,16 +37,18 @@ async fn handle_event(event: Event) -> Result<()> {
     Ok(())
 }
 
-async fn full_compare(cmp: &Compare) -> Result<String, anyhow::Error> {
-    let (wd, env_specs) = sync_repo_and_run(cmp).await?;
-    compare(&wd, env_specs, cmp).await
+async fn full_compare(cmp: &Compare) -> Result<(String, bool), anyhow::Error> {
+    let rr = sync_repo_and_run(cmp).await?;
+    let success = rr.success;
+    let output = compare(rr, cmp).await?;
+    Ok((output, success))
 }
 
-async fn compare(wd: &Path, env_specs: EnvSpecs, cmp: &Compare) -> Result<String> {
-    let mut compare = AsvCompare::new(wd, &cmp.commits[0], &cmp.commits[1]);
-    compare.in_envs(env_specs);
+async fn compare(rr: RunResult, cmp: &Compare) -> Result<String> {
+    let mut compare = AsvCompare::new(&rr.wd, &cmp.commits[0], &cmp.commits[1]);
+    compare.in_envs(rr.env_specs);
     // Try updating comment with short comparison
-    if let Err(e) = comment::update(cmp, &compare.output().await?)
+    if let Err(e) = comment::update(cmp, &compare.output().await?, rr.success)
         .instrument(tracing::info_span!("comment_update"))
         .await
     {

src/server/runner/checks.rs

@@ -16,7 +16,7 @@ pub(super) async fn with_check<Fut>(
     func: impl Fn() -> Fut,
 ) -> Result<String>
 where
-    Fut: Future<Output = Result<String>>,
+    Fut: Future<Output = Result<(String, bool)>>,
 {
     checks
         .update_check_run(check_id)
@@ -31,13 +31,18 @@ where
         images: vec![],
     };
     let (conclusion, res) = match func().await {
-        Ok(text) => {
-            output.summary = "Benchmark run successful".to_owned();
+        Ok((text, success)) => {
+            "Benchmark run successful".clone_into(&mut output.summary);
             output.text = Some(clamp_lines(&text, u16::MAX.into()).to_owned());
-            (CheckRunConclusion::Success, Ok(text))
+            let conclusion = if success {
+                CheckRunConclusion::Success
+            } else {
+                CheckRunConclusion::Failure
+            };
+            (conclusion, Ok(text))
         }
         Err(e) => {
-            output.summary = "Benchmark run failed".to_owned();
+            "Benchmark run failed".clone_into(&mut output.summary);
             output.text = Some(format!("## Error message\n{e}"));
             (CheckRunConclusion::Failure, Err(e))
         }

src/server/runner/comment.rs

@@ -6,8 +6,8 @@ use crate::constants::{is_pr_comparison, ORG, PR_COMPARISON_MARKER};
 use crate::event::Compare;
 use crate::octocrab_utils::PageExt;
 
-pub(super) async fn update(cmp: &Compare, markdown: &str) -> Result<()> {
-    let markdown = make(cmp, markdown)?;
+pub(super) async fn update(cmp: &Compare, markdown: &str, success: bool) -> Result<()> {
+    let markdown = make(cmp, markdown, success)?;
 
     tracing::info!("Updating comment for {ORG}/{}’s PR {}", cmp.repo, cmp.pr);
     let github_api = octocrab::instance();
@@ -35,52 +35,51 @@ struct Comment<'a> {
     content: &'a str,
     now: DateTime<Utc>,
     cmp: &'a Compare,
+    success: bool,
 }
 
-fn make(cmp: &Compare, content: &str) -> Result<String> {
+fn make(cmp: &Compare, content: &str, success: bool) -> Result<String> {
     Ok(Comment {
         pr_comparison_marker: PR_COMPARISON_MARKER,
         content,
         cmp,
         now: Utc::now(),
+        success,
     }
     .render()?)
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
+    use octocrab::models::CheckRunId;
+    use rstest::rstest;
 
-    #[test]
-    fn test_make_empty() {
-        let cmp = Compare {
-            repo: "repo1".to_owned(),
-            pr: 1,
-            commits: ["a".to_owned(), "b".to_owned()],
-            check_id: None,
-        };
-        let markdown = make(&cmp, "").unwrap();
-        assert!(markdown.contains(PR_COMPARISON_MARKER));
-        assert!(!markdown.contains("## Benchmark changes"));
-        assert!(markdown.contains("No changes in benchmarks."));
-        assert!(!markdown.contains("More details:"));
-    }
-
-    #[test]
-    fn test_make_filled() {
+    #[rstest]
+    fn test_make(
+        #[values(true, false)] success: bool,
+        #[values("", "Some | Table")] content: &str,
+        #[values(None, Some(3u64.into()))] check_id: Option<CheckRunId>,
+    ) {
         let cmp = Compare {
             repo: "repo2".to_owned(),
             pr: 2,
             commits: ["c".to_owned(), "d".to_owned()],
-            check_id: Some(3.into()),
+            check_id,
         };
-        let content = "Some | table";
-        let markdown = make(&cmp, content).unwrap();
+        let markdown = make(&cmp, content, success).unwrap();
         assert!(markdown.contains(PR_COMPARISON_MARKER));
-        assert!(markdown.contains("## Benchmark changes"));
+        assert_eq!(
+            !content.is_empty(),
+            markdown.contains("## Benchmark changes")
+        );
         assert!(markdown.contains(content));
-        assert!(markdown.contains(
-            "More details: <https://github.com/scverse/repo2/pull/2/checks?check_run_id=3>"
-        ));
+        assert_eq!(!success, markdown.contains("> [!WARNING]"));
+        assert_eq!(check_id.is_some(), markdown.contains("More details:"));
+        if check_id.is_some() {
+            assert!(markdown.contains(
+                "More details: <https://github.com/scverse/repo2/pull/2/checks?check_run_id=3>"
+            ));
+        }
     }
 }

src/utils.rs

@@ -1,35 +1,3 @@
-pub(crate) trait PipeMap: tap::Pipe {
-    fn pipe_map<O>(self, option: Option<O>, func: impl FnOnce(Self, O) -> Self) -> Self
-    where
-        Self: Sized,
-        O: Sized,
-    {
-        if let Some(inner) = option {
-            func(self, inner)
-        } else {
-            self
-        }
-    }
-
-    fn pipe_map_ref<O>(
-        &mut self,
-        option: Option<O>,
-        func: impl FnOnce(&mut Self, O) -> &mut Self,
-    ) -> &mut Self
-    where
-        Self: Sized,
-        O: Sized,
-    {
-        if let Some(inner) = option {
-            func(self, inner)
-        } else {
-            self
-        }
-    }
-}
-
-impl<T: tap::Pipe> PipeMap for T {}
-
 /// Get a systemd credential (see <https://systemd.io/CREDENTIALS/>).
 #[cfg(target_os = "linux")]
 pub(crate) fn get_credential(name: &str) -> anyhow::Result<secrecy::SecretString> {