Automatically use StrComparison for comparing strings #92
Conversation
There was a problem hiding this comment.
Hi, thanks for using and contributing to the project!
This certainly makes the multiline string diffing case more ergonomic for the user. I have a couple of thoughts surrounding this though:
- Could you expand on your use case a bit? i.e. where it's not suitable to simply
use pretty_assertions::assert_str_eq? I can think of a couple, but interested to know what you're actually running into in the wild. - In this PR there's no way to recover the existing behaviour for the user, if they desire it - the new format can't be opted out of. We could fix this by introducing a new macro
assert_debug_eq, which is an exact copy of the existingassert_eq. It would then be the job ofassert_eqto pick which representation to use, by some internal heuristic. - This is a breaking change in the output format, so it's questionable whether this necessitates a v2 release (open to opinions about this)
- This moves the crate away from the current behaviour of using the
Debugrepresentation in all cases. This isn't something I'm opposed to, but I'd like a semver strategy so we can say, add a specific output forVec<u8>in future, without needing major version bumps each time. - (edit): You mentioned possibly introspecting the strings to see if they contain
\nchars - I don't think this is a good idea, so very happy without it
I think overall, I need to write up some guarantees for the crate around semver, and decide whether that includes exact diff output. I'll take a look around at similar dev-tooling style crates and see what guarantees they provide.
| "#)] | ||
| fn fails_str() { | ||
| ::pretty_assertions::assert_eq!("foo\nbar", "foo\nbaz"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Thanks for adding these - I also added a couple more tests to cover the String/AsRef cases. Could you push these onto your branch, or give me access to do so?
commit 872b7233da10a4c69ca5cc1fd744e95fbb5af3aa
Author: Tom Milligan <tom@reinfer.io>
Date: Wed Mar 9 07:55:13 2022 +0000
[review] add more macro tests
diff --git a/pretty_assertions/tests/macros.rs b/pretty_assertions/tests/macros.rs
index bbbe8de..fbb5e84 100644
--- a/pretty_assertions/tests/macros.rs
+++ b/pretty_assertions/tests/macros.rs
@@ -32,23 +32,23 @@ mod assert_str_eq {
::pretty_assertions::assert_str_eq!(s0, s1);
}
- #[test]
- fn passes_as_ref_types() {
- #[derive(PartialEq)]
- struct MyString(String);
+ #[derive(PartialEq)]
+ struct MyString(String);
- impl AsRef<str> for MyString {
- fn as_ref(&self) -> &str {
- &self.0
- }
+ impl AsRef<str> for MyString {
+ fn as_ref(&self) -> &str {
+ &self.0
}
+ }
- impl PartialEq<String> for MyString {
- fn eq(&self, other: &String) -> bool {
- &self.0 == other
- }
+ impl PartialEq<String> for MyString {
+ fn eq(&self, other: &String) -> bool {
+ &self.0 == other
}
+ }
+ #[test]
+ fn passes_as_ref_types() {
let s0 = MyString("foo".to_string());
let s1 = "foo".to_string();
::pretty_assertions::assert_str_eq!(s0, s1);
@@ -62,10 +62,38 @@ mod assert_str_eq {
�[31m<ba�[0m�[1;48;5;52;31mr�[0m
�[32m>ba�[0m�[1;48;5;22;32mz�[0m
+"#)]
+ fn fails_as_ref_types() {
+ let s0 = MyString("foo\nbar".to_string());
+ let s1 = "foo\nbaz".to_string();
+ ::pretty_assertions::assert_str_eq!(s0, s1);
+ }
+
+ #[test]
+ #[should_panic(expected = r#"assertion failed: `(left == right)`
+
+�[1mDiff�[0m �[31m< left�[0m / �[32mright >�[0m :
+ foo
+�[31m<ba�[0m�[1;48;5;52;31mr�[0m
+�[32m>ba�[0m�[1;48;5;22;32mz�[0m
+
"#)]
fn fails_foo() {
::pretty_assertions::assert_str_eq!("foo\nbar", "foo\nbaz");
}
+
+ #[test]
+ #[should_panic(expected = r#"assertion failed: `(left == right)`
+
+�[1mDiff�[0m �[31m< left�[0m / �[32mright >�[0m :
+ foo
+�[31m<ba�[0m�[1;48;5;52;31mr�[0m
+�[32m>ba�[0m�[1;48;5;22;32mz�[0m
+
+"#)]
+ fn fails_string() {
+ ::pretty_assertions::assert_eq!("foo\nbar".to_string(), "foo\nbaz".to_string());
+ }
}
#[allow(clippy::eq_op)]There was a problem hiding this comment.
Great—I've committed this change (except I moved your new fails_string from mod assert_str_eq to mod assert_eq, since it is testing assert_eq!).
I don't use this crate, but I was reviewing someone else's code where they started replacing pretty_assertions::assert_eq -> assert_str_eq and it seemed like a waste of time, compared to producing the most helpful message possible whenever any of this crate's macros is used.
That's up to whether you feel enough of your users would want that to justify building it into this crate's API. From the use case that I am familiar with, I am skeptical that it would be useful.
This sounds like an extreme stance. I don't understand why it would be valuable to your users that the output remain bitwise identical. Rustc definitely doesn't guarantee that for its diagnostics, and the standard library does not guarantee that for its Debug impls, so it's already not the case for uses of pretty-assertions in general. |
|
I'm the one whose code he was reviewing ;), and I agree that the
I, too, have a hard time imagining a case in which I would rather see a bunch of
I concur. From what I understand, the primary (only?) consumer of this output is the human who is running the tests, so this is more like a UI tweak than a breaking change. A breaking change would be if a given usage of |
|
Thanks both for your comments - I agree that I can't think of a good case for preserving the existing behaviour over a more readable one. I find @jkeljo's point about I will also update the readme with some notes about the guarantees this crate provides about the output changing moving forwards between versions. |
|
Released in v1.2.0 |
This PR makes
assert_eq!(left, right)automatically behave likeassert_str_eq!(left, right)when both sides are str/String.We could alternatively do this only if one or both strings contains
\nif you prefer, as that's when the old behavior ofassert_eq!is definitely worse; #24.