A newline character should be printed after completion

[redzic]

This code used to print the progress bar, and then "Something" on the next line:

use std::thread;
use std::time::Duration;

use indicatif::ProgressBar;

fn main() {
    let pb = ProgressBar::new(1024);
    for _ in 0..1024 {
        pb.inc(1);
        thread::sleep(Duration::from_millis(5));
    }
    pb.finish_with_message("done");
    println!("Something");
}

However after #338 it prints the progress bar and the first character of what should be the next line ("S") on the same line, and then "omething" on the next line, like so:

██████████████████ 1024/1024S
omething

[djc]

Thanks for testing! @sigmaSd, would you be able to look into this?

Separately, I wonder how we could improve our testing story to catch this kind of thing.

[sigmaSd]

Yes , I think there is an io flush missing, hopefully I can look at it tomorrow

[sigmaSd]

I read the code again, it does look to work as intended since I removed the new line at the end, but I guess the progress bar line still contains a place for one more character, maybe it will work if add an extra character something like write_str(line + ' ')

[sigmaSd]

For testing, what about adding a custom draw target, something like:

diff --git a/src/draw_target.rs b/src/draw_target.rs
index 1bb9c56..d35030d 100644
--- a/src/draw_target.rs
+++ b/src/draw_target.rs
@@ -136,11 +136,14 @@ impl ProgressDrawTarget {
             ProgressDrawTargetKind::Term { ref term, .. } => term.size().1 as usize,
             ProgressDrawTargetKind::Remote { ref state, .. } => state.read().unwrap().width(),
             ProgressDrawTargetKind::Hidden => 0,
+            #[cfg(test)]
+            ProgressDrawTargetKind::TestTerm { ref term, .. } => term.width(),
         }
     }
 
     /// Apply the given draw state (draws it).
     pub(crate) fn apply_draw_state(&mut self, draw_state: ProgressDrawState) -> io::Result<()> {
+        #[cfg(not(test))]
         let (term, last_line_count) = match self.kind {
             ProgressDrawTargetKind::Term {
                 ref term,
@@ -169,11 +172,22 @@ impl ProgressDrawTarget {
             // Hidden, finished, or no need to refresh yet
             _ => return Ok(()),
         };
+        #[cfg(test)]
+        let (term, last_line_count) = match self.kind {
+            ProgressDrawTargetKind::TestTerm {
+                ref term,
+                ref mut last_line_count,
+            } => (term, last_line_count),
+            _ => unreachable!(),
+        };
 
         if !draw_state.lines.is_empty() && draw_state.move_cursor {
             term.move_cursor_up(*last_line_count)?;
         } else {
+            #[cfg(not(test))]
             clear_last_lines(term, *last_line_count)?;
+            #[cfg(test)]
+            clear_last_lines_test(term, *last_line_count)?;
         }
 
         let shift = match draw_state.alignment {
@@ -186,6 +200,9 @@ impl ProgressDrawTarget {
             }
             _ => 0,
         };
+        #[cfg(test)]
+        draw_state.draw_to_test_term(term)?;
+        #[cfg(not(test))]
         draw_state.draw_to_term(term)?;
         term.flush()?;
         *last_line_count = draw_state.lines.len() - draw_state.orphan_lines + shift;
@@ -196,6 +213,8 @@ impl ProgressDrawTarget {
     pub(crate) fn disconnect(&self) {
         match self.kind {
             ProgressDrawTargetKind::Term { .. } => {}
+            #[cfg(test)]
+            ProgressDrawTargetKind::TestTerm { .. } => {}
             ProgressDrawTargetKind::Remote { idx, ref state, .. } => {
                 state
                     .write()
@@ -226,6 +245,11 @@ impl ProgressDrawTarget {
 
 #[derive(Debug)]
 enum ProgressDrawTargetKind {
+    #[cfg(test)]
+    TestTerm {
+        term: TestTerm,
+        last_line_count: usize,
+    },
     Term {
         term: Term,
         last_line_count: usize,
@@ -403,6 +427,23 @@ impl ProgressDrawState {
         }
         Ok(())
     }
+
+    #[cfg(test)]
+    pub fn draw_to_test_term(&self, term: &TestTerm) -> io::Result<()> {
+        let len = self.lines.len();
+        for (idx, line) in self.lines.iter().enumerate() {
+            if idx + 1 != len {
+                term.write_line(line)?;
+            } else {
+                // Don't append a '\n' if this is the last line
+                term.write_str(line)?;
+                // Also append a ' ' to keep the original chars count
+                // This is important if the line was meant to fill the entire width
+                term.write_str(" ")?;
+            }
+        }
+        Ok(())
+    }
 }
 
 /// Vertical alignment of a multi progress.
@@ -445,3 +486,84 @@ fn clear_last_lines(term: &Term, n: usize) -> io::Result<()> {
     term.move_cursor_up(n.saturating_sub(1))?;
     Ok(())
 }
+#[cfg(test)]
+fn clear_last_lines_test(term: &TestTerm, n: usize) -> io::Result<()> {
+    term.move_cursor_up(n.saturating_sub(1))?;
+    for i in 0..n {
+        term.clear_line()?;
+        if i + 1 != n {
+            term.move_cursor_down(1)?;
+        }
+    }
+    term.move_cursor_up(n.saturating_sub(1))?;
+    Ok(())
+}
+
+#[cfg(test)]
+#[derive(Debug)]
+pub struct TestTerm {
+    width: usize,
+    buffer: std::sync::Arc<std::sync::Mutex<Vec<u8>>>,
+}
+#[cfg(test)]
+impl TestTerm {
+    pub fn new(width: usize, buffer: std::sync::Arc<std::sync::Mutex<Vec<u8>>>) -> Self {
+        Self { width, buffer }
+    }
+    fn width(&self) -> usize {
+        self.width
+    }
+
+    fn flush(&self) -> Result<(), io::Error> {
+        Ok(())
+    }
+
+    fn write_line(&self, arg: &str) -> Result<(), io::Error> {
+        self.write_str(&format!("{}\n", arg))
+    }
+
+    fn write_str(&self, arg: &str) -> Result<(), io::Error> {
+        use std::io::Write;
+        self.buffer.lock().unwrap().write_all(arg.as_bytes())
+    }
+
+    fn move_cursor_up(&self, _last_line_count: usize) -> Result<(), io::Error> {
+        self.write_str("1") //TODO
+    }
+
+    fn move_cursor_down(&self, _arg: i32) -> Result<(), io::Error> {
+        self.write_str("2") //TODO
+    }
+
+    fn clear_line(&self) -> Result<(), io::Error> {
+        self.write_str("3") //TODO
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::TestTerm;
+    use crate::ProgressBar;
+    use std::sync::{Arc, Mutex};
+
+    #[test]
+    fn snap() {
+        let buffer = Arc::new(Mutex::new(Vec::new()));
+        let pb = ProgressBar::with_draw_target(
+            100,
+            crate::ProgressDrawTarget {
+                kind: super::ProgressDrawTargetKind::TestTerm {
+                    term: TestTerm::new(100, buffer.clone()),
+                    last_line_count: 0,
+                },
+            },
+        );
+        for _ in 0..100 {
+            pb.inc(1);
+        }
+        drop(pb);
+        let result = Arc::try_unwrap(buffer).unwrap().into_inner().unwrap();
+        let snapshot: Vec<u8> = todo!("snapshot");
+        assert_eq!(result, snapshot);
+    }
+}

[djc]

Yeah, a custom draw target sounds good (though probably move those #[cfg(test)] items into tests?).

[sigmaSd]

The problem with moving those to their own module is that I will need to duplicate a lot of stuff, like the draw function needs to be written twice and so on

There is also insta libaray, I'm not really familiar with it, do you think it can work for indicatif?

[djc]

(I meant only the top-level items, that is, TestTerm and clear_last_lines_test().)

I'm not familiar with it either! Could work, but I'd have to see how.

[sigmaSd]

Ah that makes sense

The advantage of insta is that we won't touch indicatif code at all, since its will test the app externally, Its probably a better option.

[djc]

That sounds nice, I just don't know if/how insta can handle terminal machinations.

[sigmaSd]

Yes we'll just have to explore it, I don't know if I'll have time for it though

I saw it used in https://github.com/imsnif/bandwhich and https://github.com/zellij-org/zellij/ and if it can handle such complex tui I imagine indicatif should be a simple case

[chris-laplante]

I took a look at insta and zellij. It appears that insta is just one small piece of the solution that zellij is using for testing. To capture the output of zellij, they launch xterm on a (local?) ssh session. Then it looks like they use insta to compare the output to expected snapshots: https://github.com/zellij-org/zellij/tree/main/src/tests/e2e/snapshots.

Looks like bandwhich is using insta for nearly the same purpose, except I don't see any ssh stuff.

So insta is basically just a pretty assert_eq.

I think the solution @sigmaSd presented in #342 (comment) is the better option. I'd be happy to continue that work if you want.

[djc]

Sure, I can see how a fake draw target could make sense. Thanks for investigating!

[chris-laplante]

@sigmaSd is it cool I make a pull request based on what you posted in #342 (comment)?

[sigmaSd]

@chris-laplante of-course, no need to ask for such permissions, thanks for you work!

[chris-laplante]

Cool, thanks! There is one modification to the approach I'd like to get some feedback on, though, if @djc and @sigmaSd would be kind enough to comment on it. I would like to create a new trait that abstracts both a real Term and a TestTerminal (maybe call it TerminalLike or something like that). This would remove the requirement to write code like this:

#[cfg(not(test))]
clear_last_lines(term, *last_line_count)?;
#[cfg(test)]
clear_last_lines_test(term, *last_line_count)?;

It would be a simple trait that just exposes a few things, e.g. write_line, write_str, clear_last_lines, and width (that's just off the top of my head, I'm sure there are more). What do you think?

[djc]

Yes, I like it!

[sigmaSd]

That's sounds like a good idea

[sigmaSd]

That might require a lot of code change though, I'm not sure bu I guess you can evaluate how much work it needs while trying it

[mostthingsweb]

I'm personally ok with the amount of work if it means cleaner code overall. But yes we'll see what it looks like :)