#2784 - adding Myers based diff to rendered string equality assertion failures #2923
Conversation
|
@adamgfraser I haven't ironed all the kinks yet but this is an example of diff rendering I currently have: test("multiline strings not equal") {
assert("foo bar\nbazz buzzard")(Assertion.equalTo("bar foo\nbazard"))
}, |
|
@dkarlinsky Wow! This looks amazing. Will review in detail tomorrow. |
|
@adamgfraser I still have some issues / open questions on how to format multiline diffs, especially when CR/LF char is the only char added/removed... |
There was a problem hiding this comment.
This looks really good. A couple of things to think about:
- When should we do string diffing? Sometimes I have been annoyed with other test frameworks where I do something like
assert("bar")(equalTo("baz"))and get back this detailed diff message where given the size of the strings it is obvious that they are not the same and the diff just clutters things up. Options might be diffing only strings over a certain size, or diffing only when a certain combinator is usedassertEqualString. Counterargument would be we should do the same thing for everything. - Can we push more logic out of the
DefaultTestReporter? There is a lot going on in that file right now. Would be nice to make it as much a "consumer' of the diffing functionality as possible.
test/shared/src/main/scala-2.x/zio/test/AssertionVariants.scala
Outdated
Show resolved
Hide resolved
| (actual, expected) match { | ||
| case (left: Array[_], right: Array[_]) => left.sameElements[Any](right) | ||
| case (left, right) => left == right | ||
| } | ||
| } | ||
| assertion.withExpected(Some(expected)) |
There was a problem hiding this comment.
Do we want to add the expectation for every equality assertion or only those involving strings? This gets a little to the "when do we want to diff?" question.
There was a problem hiding this comment.
@adamgfraser see my latest commits.
It is still string-only by default, but you can customise it by passing a different Diffing to equalTo
| @@ -22,10 +22,11 @@ import scala.io.AnsiColor | |||
|
|
|||
There was a problem hiding this comment.
It would be nice to push more of the logic in this file into the diff package so the test reporter is really just handing over an actual and expected value and getting back what it should render.
There was a problem hiding this comment.
I would love to split DefaultTestReporter into to parts, where one translates test results into a simple (ish) domain model representing notions like assertion failures and diffs but without any markup stuff like lines, offsets and ANSI colors.
This would be useful for JUnitRunner and IDE integration @hmemcpy is working on, I think.
And rendering with colors and offsets would be done by a different class.
But I think this should probably be a separate PR
There was a problem hiding this comment.
I did move FailureRenderer, MessageMarkup and DiffRenderer out into their own files.
|
@dkarlinsky Would love to see this one make it in! You think you'll be able to get back to it sometime soon? |
6ea4b2c to
ffdd4cc
Compare
|
@jdegoes yes, have pushed bunch of commits addressing @adamgfraser 's comments. |
|
@dkarlinsky This looks great! It is a big PR and I'm sure there is more follow up we can do, especially around the |
|
@adamgfraser we could make the default |
|
This is great! I started a PR #2975 where I separated the My main need in that PR is to introduce the ability to support a custom renderer via command line arguments, so I can implement one externally for the IntelliJ test runner (without having to reimplement the command line runner logic). |
|
@dkarlinsky That sounds reasonable to me. I don't really have a strong opinion here, I just think it is something we should consider. There is value in having uniformity but at least personally I have been annoyed at times when I am comparing simple strings and get a whole diff from other test frameworks, especially when it is part of a larger test where the difference is the obvious part. @jdegoes What do you think? |
|
@adamgfraser even if the diff is redundant due to the strings being short, you still also get the message: |
|
@dkarlinsky Can you resolve the conflict and then we will merge? @jdegoes Had an idea that we could look at the percent of differences to the total length of the string, so if the strings are close we show the diff, if they are nothing alike we just show the two strings. That seems pretty good to me but we can follow up on that later. |
* The diffing is now determined by the assertion (as a function); * Improved multiline rendering.
…r.scala (dotty compat)
f46d002 to
47da27b
Compare
No description provided.