refactor(buffer): deprecate assert_buffer_eq! in favor of assert_eq!

[EdJoPaTo]

• Simplify assert_buffer_eq! logic.
• Deprecate assert_buffer_eq!.
• Introduce TestBackend::assert_buffer_lines.

Also simplify many tests involving buffer comparisons.

For the deprecation, just use assert_eq instead of assert_buffer_eq:

-assert_buffer_eq!(actual, expected);
+assert_eq!(actual, expected);

---

I noticed assert_buffer_eq! creating no test coverage reports and looked into this macro. First I simplified it. Then I noticed a bunch of assert_eq!(buffer, …) and other indirect usages of this macro (like TestBackend::assert_buffer).

The good thing here is that it's mainly used in tests so not many changes to the library code.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.3%. Comparing base (eb281df) to head (f2b3194).

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #1007    +/-   ##
======================================
  Coverage   94.2%   94.3%            
======================================
  Files         61      61            
  Lines      14587   14768   +181     
======================================
+ Hits       13751   13932   +181     
  Misses       836     836            

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joshka]

LGTM

• I'd suggest fixing the "needless pass by value" lints rather than allowing them, but this is a small thing in comparison to the larger change.

I wonder how possible it would be to accept Into for the expected and then make it possible to just pass a bare Iterator / IntoIterator of Into values.

E.g.

buf.assert_eq(&Buffer::with_lines(vec!["  foo", " bar "]));

Becomes:

buf.assert_eq(["  foo", " bar "]);
// or perhaps it's meaningful or necessary to disambiguate:
buf.assert_lines([...]);

[EdJoPaTo]

I wonder how possible it would be to accept Into for the expected and then make it possible to just pass a bare Iterator / IntoIterator of Into values.

I am not sure if I like that idea. The idea of assert_eq is to make sure it's exactly what you expect it should be. Into can easily end up in mistakes as it's similar but not exact. I think this assertion should focus on being precise rather than quick to write. And when its really needed .into() isn't that far away either.

[joshka]

I wonder how possible it would be to accept Into for the expected and then make it possible to just pass a bare Iterator / IntoIterator of Into values.

I am not sure if I like that idea. The idea of assert_eq is to make sure it's exactly what you expect it should be. Into can easily end up in mistakes as it's similar but not exact. I think this assertion should focus on being precise rather than quick to write. And when its really needed .into() isn't that far away either.

My rationale on this is that we have a huge amount of tests that call that Buffer::with_lines() method. The method really only exists for testing, and effectively is a point of repetition that could make the whole testing approach smaller and neater.

A well written test IMHO is one that can be grokked as correct just by looking at it. Such tests are an exercise in removing all details that are unnecessary to prove the desired assertion. Smaller tests are often better, so long as there's no (unreasonable) complexity added by DRYing up the tests. If there's a large scale repetition like this, then it's really worth considering how things look when the repetition it's removed.

I'd like to get some consensus from the other @ratatui-org/maintainers on whether to DRY the tests up or not before merging this, and make this as a single change rather than adding this new method and then removing or changing it later.

Perhaps an alternative test approach to consider could be something like:

assert_eq!(actual.lines(), [
    "hello world!".into(),
    "goodbye worl".into(),
]);

P.s. I didn't realize the lint I asked you to fix would be affect such a large amount of code - my apologies.

[EdJoPaTo]

I like that we are discussing alternatives to improve the usage of this!

---

Writing Buffer::assert_lines would be fairly simple:

impl Buffer {
    /// Asserts that this `Buffer` equals the expected lines
    pub fn assert_lines<'l, I>(&self, expected: I)
    where
        I: IntoIterator,
        I::Item: Into<Line<'l>>,
    {
        self.assert_eq(&Self::with_lines(expected));
    }
}

The usage is very similar:

-one.assert_eq(&Buffer::with_lines(["11", "11", "22", "22"]));
+one.assert_lines(["11", "11", "22", "22"]);

When the expected Buffer has more complicated styles this currently is written as let mut expected = Buffer::with_lines(…); expected.set_style(…);. These would need to use .assert_eq instead of .assert_lines which could create confusion as this differs. Also, many tests are using Buffer::empty or Buffer::with_lines in other places too.
I think there should be one way of achieving this instead of multiple ways that might create more confusion. Especially as this shortcut is only saving 18 characters. Often its on its own line anyway.

assert_eq<B: Into<Buffer>>(&self, expected: B) would also require the expected Buffer to be moved which is probably fine given that this is test code without performance relevance.
But I still think Into makes this code less clear as it does things without the user noticing it. Using .into() would be more clear.

Also side note: Into can sneak in stuff like clone() which is not obvious from looking at the code. For example EdJoPaTo/tui-rs-tree-widget@b4d0c30 took a reference and into cloned it while the caller dropped the value later resulting in a superfluous clone on every render which was invisible from looking at the code. Therefore, I like to be more explicit instead of using into in order to not introduce hidden performance relevant mistakes.

---

Your assert_eq!(lines() example seems more annoying to write than the current implementation. Looking at my example above it would need to look like this:

-one.assert_eq(&Buffer::with_lines(["11", "11", "22", "22"]));
+assert_eq!(one.lines(), [
+    "11".into(),
+    "11".into(),
+    "22".into(),
+    "22".into(),
+]);

And this still does not highlight the exact diffs like the current implementation does with Buffer::diff.
Which got me thinking if Buffer::diff would be a good way of doing this (assert_eq!(actual.diff(expected), []);), but the output is less useful in that case as it does not display the Buffer Debug output. Also, it does not notice different area sizes.

---

I think the only real alternative is the usage of assert_eq!(). As a downside we would lose the diff output:

 ---- buffer::assert::tests::assert_buffer_eq_panics_on_unequal_area stdout ----
 thread 'buffer::assert::tests::assert_buffer_eq_panics_on_unequal_area' panicked at src/buffer/assert.rs:70:9:
-buffer areas not equal
-expected:  Buffer {
+assertion `left == right` failed
+  left: Buffer {
     area: Rect { x: 0, y: 0, width: 6, height: 1 },
     content: [
         "      ",
     ],
     styles: [
         x: 0, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
     ]
 }
-actual:    Buffer {
+ right: Buffer {
     area: Rect { x: 0, y: 0, width: 5, height: 1 },
     content: [
         "     ",
     ],
     styles: [
         x: 0, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
     ]
 }

 ---- buffer::assert::tests::assert_buffer_eq_panics_on_unequal_style stdout ----
 thread 'buffer::assert::tests::assert_buffer_eq_panics_on_unequal_style' panicked at src/buffer/assert.rs:78:9:
-buffer contents not equal
-expected: Buffer {
+assertion `left == right` failed
+  left: Buffer {
     area: Rect { x: 0, y: 0, width: 5, height: 1 },
     content: [
         "     ",
     ],
     styles: [
         x: 0, y: 0, fg: Red, bg: Reset, underline: Reset, modifier: NONE,
         x: 1, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
     ]
 }
-actual: Buffer {
+ right: Buffer {
     area: Rect { x: 0, y: 0, width: 5, height: 1 },
     content: [
         "     ",
     ],
     styles: [
         x: 0, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
     ]
 }
-diff:
-0: at (0, 0)
-  expected: Cell { symbol: " ", fg: Red, bg: Reset, underline_color: Reset, modifier: NONE, skip: false }
-  actual:   Cell { symbol: " ", fg: Reset, bg: Reset, underline_color: Reset, modifier: NONE, skip: false }

This is fine for smaller Buffer comparisons but gets annoying for bigger diffs. For example the Map widget tests have hundreds of Cells. Finding a single difference in there is annoying.

---

So… I would stick with the Buffer::assert_eq function. assert_eq!() would also do the job but is less informative.

[joshka]

It feels odd to me that test code is hanging off the Buffer.

I think this might feel more appropriate as a standalone function to be imported rather than a method in the Buffer implementation. I haven't ever seen other places where the test code ends up as a method, and a function to be imported seems like a reasonable compromise. Thoughts?

To be clear, I'm suggesting:

fn assert_eq(left: Buffer, right: Buffer) {}

or assert_buffer_eq. Either seems fine, assert_eq is shorter, which makes tests easier to read, but could be mistaken for assert_eq! easily.

Another approach to keeping the assert_eq out of the public could be

trait AssertEq {
   fn assert_eq(&self, other: &Self);
}
impl AssertEq for Buffer { ...}

I really like that this removes 600 LoC. That's a bunch less to think about.

I'd really like to simplify the assertions more - perhaps fn assert_buffer_lines() would be appropriate - no magic, just a standard shortcut for what is currently (with this change) buf.assert_eq(&Buffer::with_lines([...])

By keeping this out of the buffer and as a separate function, it makes it obvious where other similar functions like that would end up (rather than bloating Buffer's namespace).

[EdJoPaTo]

It feels odd to me that test code is hanging off the Buffer.

I think this might feel more appropriate as a standalone function to be imported rather than a method in the Buffer implementation. I haven't ever seen other places where the test code ends up as a method, and a function to be imported seems like a reasonable compromise. Thoughts?

Buffer::assert_eq(actual, expected) is possible with this too.

I think having this existing on the impl Buffer is the perfect place for it as it makes it more clear where its relevant: on the Buffer.

It could be explicitly not on self, but I'm not sure why as its kinda clear to me what is happening in that case as it is used on a buffer.

bloating Buffer's namespace

Like prelude is bloating the users namespace which results in uncertainty where something is coming from and what is used? 🔥

Another approach to keeping the assert_eq out of the public could be [trait]

I don't think this should be kept out of public code as people either don't use Buffer directly (just passing it around where its needed) or use it when creating widgets, and it's really helpful for testing your code works so it should be discoverable. Maybe add to the doc comment that this is helpful for test cases and add an example for that in there?

I'd really like to simplify the assertions more - perhaps fn assert_buffer_lines() would be appropriate - no magic, just a standard shortcut for what is currently (with this change) buf.assert_eq(&Buffer::with_lines([...])

#1007 (comment)

[joshka]

Buffer::assert_eq(actual, expected) is possible with this too.

I think this idea works great. It's super clear what it's doing, while still being similar enough to the existing assert_eq! macro to drop it in without much effort when the macro doesn't hit the spot. If you're also ok with it, let's settle on that.

Like prelude is bloating the users namespace which results in uncertainty where something is coming from and what is used? 🔥

This only bloats namespaces for users that opt in to using the prelude. I'd assume your apps / widgets would be marked #[safe_from_prelude_bloat_today] ;P

#1007 (comment)

Perhaps a followup PR for that change is in order - let's get this over the line and I'll slap one together.

[EdJoPaTo]

#1007 (comment)

Perhaps a followup PR for that change is in order - let's get this over the line and I'll slap one together.

Just to make sure I understand you correctly there. I preferred not to add this as this isn’t much shorter and less clear what is happening. Your follow up PR still wants to add it?

[EdJoPaTo]

Just a reminder to ground this idea, the reason assert_buffer_eq exists is purely because the diff between buffers is difficult to visually inspect when there's an error, and the output is difficult to copy and paste from to update a broken test in a TDD approach. At steady state, this is entirely unnecessary, it's only when developing a new test or fixing a broken one when this matters.

I think this is an argument against using a more generic trait and going with a simple implementation. This shouldn't lead people to creating more things like this. This is a workaround around the assert_eq!() being hard to understand for Buffers. Ideally it does not exist for buffer nor other things so there shouldn't be a generic.

Going with pretty_assertions::assert_eq might also be an alternative approach interesting to look into. assert_eq! is hard to read when colours are involved. When its only the content that is different it's relatively easy to spot. Especially when test cases are small representations and don't use larger buffers to make their point.
Trying to get it look the same in source code might also work against getting smaller diffs… Not sure what I think of that yet.

[EdJoPaTo]

funny realisation: self.assert_lines(lines) is even shorter and would require bigger example buffers to ensure the formatting in the sources stays similar to rendering…

[EdJoPaTo]

Thought about adding Buffer::assert_eq(&self, other: AsRef<Self>) but that only removes one & per line. its not that significant and it results in less clear code that the input is only taken by reference. Also it requires a dummy impl AsRef<Self> for Buffer taking up 5 lines… don't think it's really worth it in comparison to just one &.

[joshka]

Thought about adding Buffer::assert_eq(&self, other: AsRef<Self>) but that only removes one & per line. its not that significant and it results in less clear code that the input is only taken by reference. Also it requires a dummy impl AsRef<Self> for Buffer taking up 5 lines… don't think it's really worth it in comparison to just one &.

As it's >180 refs, I think that might be worth including. Is there any downside to that impl?

assert_eq! is hard to read when colours are involved. When its only the content that is different it's relatively easy to spot

Interestingly if you do give pretty_assertions an ansi string, it will render it as colors (though it will overwrite the colors with its red/green diff colors):

use pretty_assertions::assert_eq;
#[test]
fn to_string() {
    let mut buf = AnsiStringBuffer::new(5, 2);
    buf.render_ref(&Line::styled("Hello", Color::Blue), Rect::new(0, 0, 5, 1));
    buf.render_ref(&Line::styled("World", Color::Green), Rect::new(0, 1, 5, 1));
    assert_eq!(
        buf.to_string(),
        "\u{1b}[34;49mHello\n\u{1b}[32;49mMars \u{1b}[0m"
    );
}

Experimenting with a style diff:

The main hassle with this output it doesn't easily lend itself to being copied back into the test.

When writing test code that interacts with the buffer (widgets etc.), my usual (Test after Dev) flow is:

1. write a broken test that asserts that the buffer is an empty buffer
2. look at the output of the test and confirm that it looks correct
3. copy the output of the test directly into the test

This leads to a pretty fast and easy approach to going from nothing to tested. And the effectiveness of this is proven out by the increase in the quantity and quality of unit tests in Ratatui even as we've added a significant amount of code.

For interest sake, I ran coverage (cargo llvm-cov --open --all-targets) on v0.19.0 (tui-rs) and main.

Before we started improving this:

Current situation:

Summary, we added ~10k LoC, and increased the test coverage by ~10k LoC.
This number is probably higher as the coverage bump we get from this change is pretty big.

[EdJoPaTo]

When writing test code that interacts with the buffer (widgets etc.), my usual (Test after Dev) flow is:

1. write a broken test that asserts that the buffer is an empty buffer
2. look at the output of the test and confirm that it looks correct
3. copy the output of the test directly into the test

This leads to a pretty fast and easy approach to going from nothing to tested. And the effectiveness of this is proven out by the increase in the quantity and quality of unit tests in Ratatui even as we've added a significant amount of code.

This also works with the native assert_eq!() as the output is from Debug, not from the assert_buffer_eq.

So maybe the best approach is to remove the dedicated buffer assert stuff all together?

Adding a #[rustfmt::skip] exactly at the wrapping thing prevents it from happening while the rest of the code is still formatted:

#[rustfmt::skip]
let expected = Buffer::with_lines([
    " a",
    " b",
    " c",
]);

let expected = Buffer::with_lines([" a", " b", " c"]);

This way the buffer content can get way smaller resulting in smaller Debug outputs and therefore easier to spot differences for test cases. Then the need for a special macro for this is even less important.

If that still requires a diff as it's still complicated to spot the difference then a (temporary?) dbg!(actual.diff(expected)) is not that far away.

That would remove the somewhat weird custom assert_eq!() implementation for the buffer making use of standard Rust while addressing the pain point of bigger diffs due to having bigger than necessary buffers in code due to code formatting prevention.

[joshka]

To make that work, the debug format also needs to be much less verbose (but in a way that makes it easy to identify differences).

For the most part, the visual inspection approach applies to multi-line output in the buffers. I'm not opposed to rustfmt::skip, I used the approach of longer lines before I knew that existed.

If that still requires a diff as it's still complicated to spot the difference then a (temporary?) dbg!(actual.diff(expected)) is not that far away.

Makes sense I guess.

Perhaps give it a try and see how it looks with just assert_eq!

[joshka]

Great work in wrangling all the tests into a really consistent form and making them into rstests when appropriate. I went through the entire PR with a pretty detailed view, and only found the comments previously raised to get answers to / fixes.

I think this is ready to merge, but I'm 50/50 on whether to wait til after 0.26.3 to merge (so we can get out the unicode line bug fix).
Pros: (less carrying cost of large PR, less future PR conflicts)
Cons: (potential breakage so close to releasing a patch release)

Your call on whether to merge this.

[EdJoPaTo]

I think this is ready to merge, but I'm 50/50 on whether to wait til after 0.26.3 to merge (so we can get out the unicode line bug fix).

Pros: (less carrying cost of large PR, less future PR conflicts)

Future PR conflicts is not really an argument as stuff isn't branched of release tags, they are branched of from the default branch.

Cons: (potential breakage so close to releasing a patch release)

This changes mostly tests. Only the macro itself is a non-test change. And it fixes #1043.

I could see why removing the deprecation might be a good idea until after the patch release, but it's really just a deprecation. #[allow(deprecation)] is an easy workaround. Nothing is panicking on runtime without any heads up.

Your call on whether to merge this.

Therefore, I would go for merging this (once the open points are all resolved).

[EdJoPaTo]

Any improvements for the first message (above the line) as a commit message? When not, this should be merged.

[joshka]

Any improvements for the first message (above the line) as a commit message? When not, this should be merged.

LGTM