From 4fc539b801065e26261b9501a191a2500a9413f6 Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Sun, 8 Dec 2024 23:03:25 -0800
Subject: [PATCH] progress: add test of progress bar's periodic output

The progress bar is supposed to update only 30 times per second, but
as was reported in #5057, that doesn't seem to be what's
happening. This patch adds tests showing how we update the progress
bar for every event.
---
 cli/src/progress.rs | 43 ++++++++++++++++++++++++++++++++++++++++---
 cli/src/ui.rs       | 42 ++++++++++++++++++++++++++++++------------
 2 files changed, 70 insertions(+), 15 deletions(-)

diff --git a/cli/src/progress.rs b/cli/src/progress.rs
index c4b79b37c6..55b4dc0be1 100644
--- a/cli/src/progress.rs
+++ b/cli/src/progress.rs
@@ -33,11 +33,11 @@ impl Progress {
         }
     }
 
-    pub fn update(
+    pub fn update<W: std::io::Write>(
         &mut self,
         now: Instant,
         progress: &git::Progress,
-        output: &mut ProgressOutput,
+        output: &mut ProgressOutput<W>,
     ) -> io::Result<()> {
         use std::fmt::Write as _;
 
@@ -166,7 +166,7 @@ impl RateEstimateState {
 pub fn snapshot_progress(ui: &Ui) -> Option<impl Fn(&RepoPath) + '_> {
     struct State {
         guard: Option<OutputGuard>,
-        output: ProgressOutput,
+        output: ProgressOutput<std::io::Stderr>,
         next_display_time: Instant,
     }
 
@@ -215,6 +215,8 @@ pub fn snapshot_progress(ui: &Ui) -> Option<impl Fn(&RepoPath) + '_> {
 
 #[cfg(test)]
 mod tests {
+    use insta::assert_snapshot;
+
     use super::*;
 
     #[test]
@@ -233,4 +235,39 @@ mod tests {
         assert_eq!(buf, "█████▍    ");
         buf.clear();
     }
+
+    #[test]
+    fn test_update() {
+        let start = Instant::now();
+        let mut progress = Progress::new(start);
+        let mut current_time = start;
+        let mut update = |duration, overall| -> String {
+            current_time += duration;
+            let mut buf = vec![];
+            let mut output = ProgressOutput::for_test(&mut buf, 25);
+            progress
+                .update(
+                    current_time,
+                    &jj_lib::git::Progress {
+                        bytes_downloaded: None,
+                        overall,
+                    },
+                    &mut output,
+                )
+                .unwrap();
+            String::from_utf8(buf).unwrap()
+        };
+        // First output is after the initial delay
+        assert_snapshot!(update(INITIAL_DELAY - Duration::from_millis(1), 0.1), @"");
+        assert_snapshot!(update(Duration::from_millis(1), 0.10), @"[?25l\r 10% [█▊                ]");
+        // TODO: No updates for the next 30 milliseconds
+        assert_snapshot!(update(Duration::from_millis(10), 0.11), @" 11% [██                ]");
+        assert_snapshot!(update(Duration::from_millis(10), 0.12), @" 12% [██▏               ]");
+        assert_snapshot!(update(Duration::from_millis(10), 0.13), @" 13% [██▍               ]");
+        // We get an update now that we go over the threshold
+        assert_snapshot!(update(Duration::from_millis(100), 0.30), @" 30% [█████▍            ]");
+        // TODO: Even though we went over by quite a bit, the new threshold is relative
+        // to the previous output, so we don't get an update here
+        assert_snapshot!(update(Duration::from_millis(30), 0.40), @" 40% [███████▎          ]");
+    }
 }
diff --git a/cli/src/ui.rs b/cli/src/ui.rs
index a1df2066a4..f66c480829 100644
--- a/cli/src/ui.rs
+++ b/cli/src/ui.rs
@@ -454,10 +454,9 @@ impl Ui {
         }
     }
 
-    pub fn progress_output(&self) -> Option<ProgressOutput> {
-        self.use_progress_indicator().then(|| ProgressOutput {
-            output: io::stderr(),
-        })
+    pub fn progress_output(&self) -> Option<ProgressOutput<std::io::Stderr>> {
+        self.use_progress_indicator()
+            .then(ProgressOutput::for_stderr)
     }
 
     /// Writer to print an update that's not part of the command's main output.
@@ -633,22 +632,31 @@ impl Ui {
 }
 
 #[derive(Debug)]
-pub struct ProgressOutput {
-    output: Stderr,
+pub struct ProgressOutput<W> {
+    output: W,
+    term_width: Option<u16>,
 }
 
-impl ProgressOutput {
-    pub fn write_fmt(&mut self, fmt: fmt::Arguments<'_>) -> io::Result<()> {
-        self.output.write_fmt(fmt)
+impl ProgressOutput<io::Stderr> {
+    pub fn for_stderr() -> ProgressOutput<io::Stderr> {
+        ProgressOutput {
+            output: io::stderr(),
+            term_width: None,
+        }
     }
+}
 
-    pub fn flush(&mut self) -> io::Result<()> {
-        self.output.flush()
+impl<W> ProgressOutput<W> {
+    pub fn for_test(output: W, term_width: u16) -> Self {
+        Self {
+            output,
+            term_width: Some(term_width),
+        }
     }
 
     pub fn term_width(&self) -> Option<u16> {
         // Terminal can be resized while progress is displayed, so don't cache it.
-        term_width()
+        self.term_width.or_else(term_width)
     }
 
     /// Construct a guard object which writes `text` when dropped. Useful for
@@ -661,6 +669,16 @@ impl ProgressOutput {
     }
 }
 
+impl<W: Write> ProgressOutput<W> {
+    pub fn write_fmt(&mut self, fmt: fmt::Arguments<'_>) -> io::Result<()> {
+        self.output.write_fmt(fmt)
+    }
+
+    pub fn flush(&mut self) -> io::Result<()> {
+        self.output.flush()
+    }
+}
+
 pub struct OutputGuard {
     text: String,
     output: Stderr,