vercel · chris-olszewski · Feb 14, 2024 · Feb 9, 2024 · Feb 10, 2024 · Feb 14, 2024
@@ -4,4 +4,23 @@ type GroupPrefixFn = fn(group_name: &str) -> String;
 pub struct VendorBehavior {
     pub group_prefix: GroupPrefixFn,
     pub group_suffix: GroupPrefixFn,
+    pub error_group_prefix: Option<GroupPrefixFn>,
+    pub error_group_suffix: Option<GroupPrefixFn>,
+}
+
+impl VendorBehavior {
+    pub fn new(prefix: GroupPrefixFn, suffix: GroupPrefixFn) -> Self {
+        Self {
+            group_prefix: prefix,
+            group_suffix: suffix,
+            error_group_prefix: None,
+            error_group_suffix: None,
+        }
+    }
+
+    pub fn with_error(mut self, prefix: GroupPrefixFn, suffix: GroupPrefixFn) -> Self {
+        self.error_group_prefix = Some(prefix);
+        self.error_group_suffix = Some(suffix);
+        self
+    }
 }
@@ -77,10 +77,10 @@ pub(crate) fn get_vendors() -> &'static [Vendor] {
                     sha_env_var: None,
                     branch_env_var: None,
                     username_env_var: None,
-                    behavior: Some(VendorBehavior {
-                        group_prefix: |group_name| format!("##[group]{group_name}\r\n"),
-                        group_suffix: |_| String::from("##[endgroup]\r\n"),
-                    }),
+                    behavior: Some(VendorBehavior::new(
+                        |group_name| format!("##[group]{group_name}\r\n"),
+                        |_| String::from("##[endgroup]\r\n"),
+                    )),
                 },
                 Vendor {
                     name: "Bamboo",
@@ -266,10 +266,16 @@ pub(crate) fn get_vendors() -> &'static [Vendor] {
                     sha_env_var: Some("GITHUB_SHA"),
                     branch_env_var: Some("GITHUB_REF_NAME"),
                     username_env_var: Some("GITHUB_ACTOR"),
-                    behavior: Some(VendorBehavior {
-                        group_prefix: |group_name| format!("::group::{group_name}\n"),
-                        group_suffix: |_| String::from("::endgroup::\n"),
-                    }),
+                    behavior: Some(
+                        VendorBehavior::new(
+                            |group_name| format!("::group::{group_name}\n"),
+                            |_| String::from("::endgroup::\n"),
+                        )
+                        .with_error(
+                            |group_name| format!("\x1B[;31m{group_name}\x1B[;0m\n"),
+                            |_| String::new(),
+                        ),
+                    ),
                 },
                 Vendor {
                     name: "GitLab CI",
@@ -546,14 +552,10 @@ pub(crate) fn get_vendors() -> &'static [Vendor] {
                     sha_env_var: None,
                     branch_env_var: None,
                     username_env_var: None,
-                    behavior: Some(VendorBehavior {
-                        group_prefix: |group_name| {
-                            format!("##teamcity[blockOpened name='{group_name}']")
-                        },
-                        group_suffix: |group_name| {
-                            format!("##teamcity[blockClosed name='{group_name}']")
-                        },
-                    }),
+                    behavior: Some(VendorBehavior::new(
+                        |group_name| format!("##teamcity[blockOpened name='{group_name}']"),
+                        |group_name| format!("##teamcity[blockClosed name='{group_name}']"),
+                    )),
                 },
                 Vendor {
                     name: "Travis CI",
@@ -566,10 +568,10 @@ pub(crate) fn get_vendors() -> &'static [Vendor] {
                     sha_env_var: None,
                     branch_env_var: None,
                     username_env_var: None,
-                    behavior: Some(VendorBehavior {
-                        group_prefix: |group_name| format!("travis_fold:start:{group_name}\r\n"),
-                        group_suffix: |group_name| format!("travis_fold:end:{group_name}\r\n"),
-                    }),
+                    behavior: Some(VendorBehavior::new(
+                        |group_name| format!("travis_fold:start:{group_name}\r\n"),
+                        |group_name| format!("travis_fold:end:{group_name}\r\n"),
+                    )),
                 },
                 Vendor {
                     name: "Vercel",

@@ -392,6 +392,12 @@ impl<'a> Visitor<'a> {
                 (vendor_behavior.group_suffix)(&group_name),
             );
             logger.with_header_footer(Some(header), Some(footer));
+
+            let (error_header, error_footer) = (
+                vendor_behavior.error_group_prefix.map(|f| f(&group_name)),
+                vendor_behavior.error_group_suffix.map(|f| f(&group_name)),
+            );
+            logger.with_error_header_footer(error_header, error_footer);
         }
         logger
     }
@@ -690,7 +696,10 @@ impl ExecContext {
             .instrument(span)
             .await;
 
-        let logs = match output_client.finish() {
+        // If the task resulted in an error, do not group in order to better highlight
+        // the error.
+        let is_error = matches!(result, ExecOutcome::Task { .. });
+        let logs = match output_client.finish(is_error) {
             Ok(logs) => logs,
             Err(e) => {
                 telemetry.track_error(TrackedErrors::DaemonFailedToMarkOutputsAsCached);

@@ -22,6 +22,12 @@ pub struct OutputClient<W> {
     // Any locals held across an await must implement Sync and RwLock lets us achieve this
     buffer: Option<RwLock<Vec<SinkBytes<'static>>>>,
     writers: Arc<Mutex<SinkWriters<W>>>,
+    primary: Marginals,
+    error: Marginals,
+}
+
+#[derive(Default)]
+struct Marginals {
     header: Option<String>,
     footer: Option<String>,
 }
@@ -82,16 +88,19 @@ impl<W: Write> OutputSink<W> {
             behavior,
             buffer,
             writers,
-            header: None,
-            footer: None,
+            primary: Default::default(),
+            error: Default::default(),
         }
     }
 }
 
 impl<W: Write> OutputClient<W> {
     pub fn with_header_footer(&mut self, header: Option<String>, footer: Option<String>) {
-        self.header = header;
-        self.footer = footer;
+        self.primary = Marginals { header, footer };
+    }
+
+    pub fn with_error_header_footer(&mut self, header: Option<String>, footer: Option<String>) {
+        self.error = Marginals { header, footer };
     }
 
     /// A writer that will write to the underlying sink's out writer according
@@ -116,15 +125,23 @@ impl<W: Write> OutputClient<W> {
 
     /// Consume the client and flush any bytes to the underlying sink if
     /// necessary
-    pub fn finish(self) -> io::Result<Option<Vec<u8>>> {
+    pub fn finish(self, use_error: bool) -> io::Result<Option<Vec<u8>>> {
         let Self {
             behavior,
             buffer,
             writers,
-            header,
-            footer,
+            primary,
+            error,
         } = self;
         let buffers = buffer.map(|cell| cell.into_inner().expect("lock poisoned"));
+        let header = use_error
+            .then_some(error.header)
+            .flatten()
+            .or(primary.header);
+        let footer = use_error
+            .then_some(error.footer)
+            .flatten()
+            .or(primary.footer);
 
         if matches!(behavior, OutputClientBehavior::Grouped) {
             let buffers = buffers
@@ -253,15 +270,15 @@ mod test {
                 let mut err = pass_thru_logger.stderr();
                 writeln!(&mut out, "task 1: out").unwrap();
                 writeln!(&mut err, "task 1: err").unwrap();
-                assert!(pass_thru_logger.finish().unwrap().is_none());
+                assert!(pass_thru_logger.finish(true).unwrap().is_none());
             });
             s.spawn(move || {
                 let mut out = buffer_logger.stdout();
                 let mut err = buffer_logger.stderr();
                 writeln!(&mut out, "task 2: out").unwrap();
                 writeln!(&mut err, "task 2: err").unwrap();
                 assert_eq!(
-                    buffer_logger.finish().unwrap().unwrap(),
+                    buffer_logger.finish(true).unwrap().unwrap(),
                     b"task 2: out\ntask 2: err\n"
                 );
             });
@@ -291,7 +308,7 @@ mod test {
             "pass thru should end up in sink immediately"
         );
         assert!(
-            logger.finish()?.is_none(),
+            logger.finish(true)?.is_none(),
             "pass through logs shouldn't keep a buffer"
         );
         assert_eq!(
@@ -317,7 +334,7 @@ mod test {
             "buffer should end up in sink immediately"
         );
         assert_eq!(
-            logger.finish()?.unwrap(),
+            logger.finish(true)?.unwrap(),
             b"output for 1\n",
             "buffer should return buffer"
         );
@@ -343,11 +360,11 @@ mod test {
         writeln!(&mut group2_out, "output for 2")?;
         writeln!(&mut group1_out, "output for 1")?;
         let group1_logs = group1_logger
-            .finish()?
+            .finish(true)?
             .expect("grouped logs should have buffer");
         writeln!(&mut group2_err, "warning for 2")?;
         let group2_logs = group2_logger
-            .finish()?
+            .finish(true)?
             .expect("grouped logs should have buffer");
 
         assert_eq!(group1_logs, b"output for 1\n");
@@ -360,6 +377,44 @@ mod test {
         Ok(())
     }
 
+    #[test]
+    fn test_marginals() -> io::Result<()> {
+        let sink = OutputSink::new(Vec::new(), Vec::new());
+        let mut group1_logger = sink.logger(OutputClientBehavior::Grouped);
+        group1_logger
+            .with_header_footer(Some("good header\n".into()), Some("good footer\n".into()));
+        group1_logger
+            .with_error_header_footer(Some("bad header\n".into()), Some("bad footer\n".into()));
+        let mut group2_logger = sink.logger(OutputClientBehavior::Grouped);
+        group2_logger
+            .with_header_footer(Some("good header\n".into()), Some("good footer\n".into()));
+
+        let mut group1_out = group1_logger.stdout();
+        let mut group2_out = group2_logger.stdout();
+
+        writeln!(&mut group2_out, "output for 2")?;
+        writeln!(&mut group1_out, "output for 1")?;
+        let group1_logs = group1_logger
+            .finish(true)?
+            .expect("grouped logs should have buffer");
+        let group2_logs = group2_logger
+            .finish(true)?
+            .expect("grouped logs should have buffer");
+
+        assert_eq!(group1_logs, b"output for 1\n");
+        assert_eq!(group2_logs, b"output for 2\n");
+
+        let SinkWriters { out, .. } = Arc::into_inner(sink.writers).unwrap().into_inner().unwrap();
+        // Error marginals used when present, primary ones used if errors aren't
+        // provided
+        assert_eq!(
+            out,
+            b"bad header\noutput for 1\nbad footer\ngood header\noutput for 2\ngood footer\n"
+        );
+
+        Ok(())
+    }
+
     #[test]
     fn test_loggers_wait_for_newline() {
         let b1 = Arc::new(Barrier::new(2));
@@ -374,14 +429,14 @@ mod test {
                 write!(&mut out, "task 1:").unwrap();
                 b1.wait();
                 writeln!(&mut out, " echo building").unwrap();
-                assert!(logger1.finish().unwrap().is_none());
+                assert!(logger1.finish(true).unwrap().is_none());
             });
             s.spawn(move || {
                 let mut out = logger2.stdout();
                 write!(&mut out, "task 2:").unwrap();
                 b2.wait();
                 writeln!(&mut out, " echo failing").unwrap();
-                assert!(logger2.finish().unwrap().is_none(),);
+                assert!(logger2.finish(true).unwrap().is_none(),);
             });
         });
         let SinkWriters { out, .. } = Arc::into_inner(sink.writers).unwrap().into_inner().unwrap();

@@ -104,7 +104,7 @@ fn echo_task(
     }
     process.wait()?;
 
-    client.finish()?;
+    client.finish(true)?;
 
     Ok(())
 }
@@ -56,7 +56,7 @@ Verify that errors are grouped properly
   \xe2\x80\xa2 Packages in scope: my-app, util (esc)
   \xe2\x80\xa2 Running fail in 2 packages (esc)
   \xe2\x80\xa2 Remote caching disabled (esc)
-  ::group::util:fail
+  \x1b[;31mutil:fail\x1b[;0m (esc)
   cache miss, executing 122cca10fdcda4f0
 
   \> fail (re)
@@ -68,7 +68,6 @@ Verify that errors are grouped properly
   npm ERR!   in workspace: util 
   npm ERR\!   at location: (.*)(\/|\\)packages(\/|\\)util  (re)
   \[ERROR\] command finished with error: command \((.*)(\/|\\)packages(\/|\\)util\) (.*)npm(?:\.cmd)? run fail exited \(1\) (re)
-  ::endgroup::
   ::error::util#fail: command \(.*(\/|\\)packages(\/|\\)util\) (.*)npm(?:\.cmd)? run fail exited \(1\) (re)
 
    Tasks:    0 successful, 1 total