Opentest4j or use diff views for string comparison

[robstoll]

I was thinking to use opentest4j to improve String comparisons so that one gets support in the IDE to see a diff view.
However, I am not sure if opentest4j is a good fit as it is meant for jvm only. Maybe it is better if we deliver a diff hint in case the subject is a CharSequence. For instance:

expect("Hello my name is Robert and I am the author of Atrium")
  .toBe("hello my name is Robert and I am the author of ATrium")

Should result in something like

expect: Hello my name is Robert and I am the author of Atrium
* to be: hello my name is Robert and I am the author of ATrium
  >> diff: 
    @@ -1 +1 @@
    [-Hello-]{+hello+} my name is Robert and I am the author of [-Atrium-]{+ATrium+}

I guess this is especially helpful if someone compares long texts such as file contents (e.g. two json).

[jGleitz]

Can we do both? I would like to have the diff view in IntelliJ so opentest4j would be great if we are on the JVM. Your diff view would be helpful on other platforms.

[robstoll]

might be possible. I am not yet sure how well we can map to opentest4j exceptions as Atrium is more expressive, but we will see.

[robstoll]

As mentioned by @igorakkerman the diff makes probably not sense if the String is to short (TBD what short means). I agree for text reporting and to a console which does not support colours.
In case of a coloured console (#27) I would suggest to always use a diff view but not as shown above but by highlighting the diff with another colour (red, blue whatever)

[igorakkerman]

I'd say: just don't. It bothers more than it helps, really.

A viable alternative for seeing the differences would be an IDE plugin that would show the differences in a popup, when requested. IntelliJ IDEA does that already for some Java frameworks.

[jGleitz]

@igorakkerman that is what adopting opentest4j is about. They are currently discussing providing, diffs, too.

I would like to make a WIP PR that makes atrium adopt opentest4j. We can then see what that will result in and discuss there.

[robstoll]

This will most likely require a breaking change as the super-class of AtriumError will need to change, thus assigned it to 1.0.0

[robstoll]

I made a quick test with opentest4j. As it seems intellij does not bother whether the exception is one of opentest4j's classes or not but it searches for a particular pattern in the error message. For instance, intellij will provide a diff view for the following

throw AtriumError.create("expected: <1> but was: <2>", coreFactory.newNoOpAtriumErrorAdjuster())

The prototyp is in the branch: https://github.com/robstoll/atrium/tree/%23339-avoid-duplicate-for-jvm-opentest4j
commit robstoll/atrium@47f3310

[robstoll]

Found a stackoverflow issue:
https://stackoverflow.com/questions/10934743/formatting-output-so-that-intellij-idea-shows-diffs-for-two-texts

with a link to some patterns which were in use (would be interesting to see the current patterns):
https://github.com/JetBrains/intellij-community/blob/d2ef69f/plugins/junit_rt/src/com/intellij/junit4/ExpectedPatterns.java
(master: https://github.com/JetBrains/intellij-community/blob/master/plugins/junit_rt/src/com/intellij/junit4/ExpectedPatterns.java)

and https://github.com/JetBrains/intellij-community/blob/0e2aa4030ee763c9b0c828f0b5119f4cdcc66f35/java/java-runtime/src/com/intellij/rt/execution/testFrameworks/AbstractExpectedPatterns.java

The good thing, we probably don't have to bother about opentest4j (one dependency less 👍) on the other hand it seems a bit random and we need to see if we actually want to use the patterns or not (surely not changing the whole reporting only for that but maybe as additional hint).

[jGleitz]

That’s interesting. I was playing around with the Opentest4j exceptions and was not able to merry them easily to atrium’s existing exception infrastructure.

If JetBrains keeps a list of patterns they search for, maybe we can open a ticket to have our pattern added? We would then need to guarantee that this pattern stays stable, though.

[vlsi]

What do you think of a reporter that collects all the failed expectations in known to idea format?

In other words, generate something like

Group text 1
Expected value

Group text 2
Expected value 2

Then it could be compared via idea's diff.

See also https://youtrack.jetbrains.com/issue/IDEA-313769

Yet another alternative is to call idea diff file1 file2, so it spawns a diff window. The drawback is there's no button to show the diff again, however, there might be a command to copy-paste. Another drawbacks is one does not probably want more than one diff window to spawn automatically.

https://github.com/approvals/ApprovalTests.Java/blob/master/approvaltests/docs/Reporters.md#supported-diff-tools have implemented launcers for many diff tools, however, they are not exracted as a seperate module/dependency

[robstoll]

What do you think of a reporter that collects all the failed expectations in known to idea format?

that's what I was thinking as well, using a format where intellij provides a diff link. I would like to discuss whether we include it into the regular error message or if we repeat it at the end.

in error message could be something like:

I expected subject: "long string..."
* to equal: "another long string..."
  (d) expected: <long string...> but was <another long string>

currently my plan would be:

1. collect all toEqual like expectations and show debug failure hint in the error message as shown above
2. show it only, if envVar CI is not set
3. don't show the full representation ("long string...") if CI is not set, only show full in the debug hint.
4. currently we restrict the number of characters per representation to 10'000. I would change this, so that the limit only applies if CI is set
5. implement a git like diff which we include in the error message
6. allow to modify the behaviour via ReportOptions

Thoughts?

[vlsi]

Thoughts?

Sounds like a good plan to me.

don't show the full representation ("long string...") if CI is not set, only show full in the debug hint.

Just in case, IDEA supports at most one "diff link" (I was not able to make it display several links), so incline we need to build a single debug hint for all equal-like comparisons.

show it only, if envVar CI is not set

I think IDEA injects a class or something, so we can show "diff for IDEA" only in case we know there's IDEA.

don't show the full representation ("long string...") if CI is not set, only show full in the debug hint.

We could probably omit the diff in case it exceeds 50% of the full length.

[vlsi]

Opentest4j allows throwing "comparison failed" exceptions without displaying too much information on the screen, so Opentest4j seems to be a reasonable way to achieve "click to see diff".

However, it looks like Opentest4j exception is not properly displayed when the test is executed via Gradle runner: https://youtrack.jetbrains.com/issue/IDEA-275065/Click-to-see-diference-does-not-appear-with-org.opentest4j.AssertionFailedError-when-running-tests-with-Gradle

[robstoll]

Just in case, IDEA supports at most one "diff link" (I was not able to make it display several links), so incline we need to build a single debug hint for all equal-like comparisons.

That's bad to hear, maybe we can trick it by using different patterns? let's see, in the worst case we make one big debug string but in this case I would put it at the end, after the error message as such.

think IDEA injects a class or something, so we can show "diff for IDEA" only in case we know there's IDEA.

My guess is that also other IDEs have this diff functionslity and also parse the exception message for expect<> but was <> therefore I would (per default) output it as long as env CI is not set.

We could probably omit the diff in case it exceeds 50% of the full length.

I don't understand what you mean, could you elaborate please?

so Opentest4j seems to be a reasonable way to achieve "click to see diff".

Yes, at least for the JVM platform. As far as I remember they also have an exceptions which gathers multiple other exceptions so that IDEA shows several click to see diff.
However, opentest4J is only available on the jvm platform, I would like to have a solution which also works for JS and potentially other platforms in the future. We would need to try out if intellij shows also the diffs in case we fake the opentest4J exceptions on other platforms. If this works, then this might be a good solution.
But, if I recall correctly, the exceptions do not allow to have a custom message and the report would suffer quite a bit (this would be a no go IMO). I don't remember if the classes are final or if we could override this behaviour. Also, AtriumError currently includes a workaround for a behaviour of IDEA. Usually IDEA shows the error message twice which is quite cumbersome IMO. We work around it so that the error message is shown only once.

[vlsi]

That's bad to hear, maybe we can trick it by using different patterns?

I studied the code around, and it looks currently the behaviour is:

1. Try extracting expected and actual from well-known exception classes
2. If no luck, try extracting expected and actual from the exception message

At all times, it extracts a single "diff" only.

My guess is that also other IDEs have this diff functionslity and also parse the exception message

Frankly speaking, I do not know if the other IDEs use regexp parsing for JUnit patterns
The issue with printing "expected:<...> got:<...>" is the message would be very big, and if that is printed for every failed test it might quickly become impractical, especially, if the runner makes no parsing.

I don't understand what you mean, could you elaborate please?

I consider diff as an option to shrink and pin-point the difference.
Suppose the input is 100KiB, and the output is 100KiB while every line is completely different. For instance, suppose old output was YAML, and the new output is XML.
Then the classical diff would produce 200KiB of garbage :)
So I think "diff" should give up at some point and produce something like "ok, the output has dramatically changed, here are the first lines, however, N of them were modified as well".
Of course, we might try several normalizations before giving up. For instance, try ignoring whitespace.

However, opentest4J is only available on the jvm platform

Right you are.

As far as I remember they also have an exceptions which gathers multiple other exceptions so that IDEA shows several click to see diff.

There's org.opentest4j.MultipleFailuresError, however, I can't make IDEA to display several "diff" links even when using IDEA test runner.

I would like to have a solution which also works for JS and potentially other platforms in the future

Frankly speaking, I do not know what are the solutions for other platforms.
It looks like having a common stdout microformat might work: https://youtrack.jetbrains.com/issue/IDEA-313769/Support-click-to-see-difference-for-file-based-comparisons
In other words, having <editor-diff ...> in the standard output might work as a cross-platform way of asking IDEs to display diffs

[robstoll]

I consider diff as an option to shrink and pin-point the difference.

I see it the same way and now I understand what you mean, makes sense. 👍

There's org.opentest4j.MultipleFailuresError, however, I can't make IDEA to display several "diff" links even when using IDEA test runner.

that's bad news, I wonder if I recall wrong or if it's a bug. Anyway, I tend to go down the opentest4j approach (if we can subclass reasonably) where we should be able to offer one click to see difference link which usually appears after the exception message, isn't it?
I'll figure out something for the other platforms. JVM has clearly a higher prio and is the target which is used most often by far. If we structure the debug hint well, then I guess it shouldn't be a problem that we show multiple failed expectations within the same diff.

Frankly speaking, I do not know if the other IDEs use regexp parsing for JUnit patterns
The issue with printing "expected:<...> got:<...>" is the message would be very big, and if that is printed for every failed test it might quickly become impractical, especially, if the runner makes no parsing.

I see your point. I think if we use opentest4j then it is fair if we assume the IDE supports a diff feature based on it and we don't need to print the pattern at all.
And I reconsidered the check for CI, I think it is better to determine if the current runner (including its context) supports the diff feature. you mentioned IDEA injects a class, can you point to the corresponding code please?
In case they inject it as well when using the gradle runner, then I would show the git like diff (as the click link is not shown) until the above mentioned bug is fixed.

[robstoll]

for gradle we could alternatively also provide an own TestFailureMapper

[robstoll]

took a close look how we could provide an own TestFailureMapper but it doesn't seem to possible for now, the implementation uses a hard-coded list of mappers:
https://github.com/gradle/gradle/blob/master/platforms/jvm/testing-junit-platform/src/main/java/org/gradle/api/internal/tasks/testing/junitplatform/JUnitPlatformTestExecutionListener.java#L71

[robstoll]

@dsvoronin thanks for reminding me about the missing diff functionality in Atrium via robstoll/atrium#1797. I would like to gather some feedback before implementing something. Maybe you can share your ideas. @VEZE I saw your thumbs up, so maybe you can give your feedback as well. (of course any other feedback is welcome as well)

I see three main problems:

1. it is only possible to diff one value
2. we cannot place the <Click to see difference> at an arbitrary place
3. it is mainly suited for simple expect(...).toEqual()

Personally I don't think we will end up with a nice solution if we try to come up with a diff for something the following current output because oft he above mentioned problems:

I expected subject: [1, 2, 2, 4]        (java.util.Arrays.ArrayList <1234789>)
◆ ▶ size: 4        (kotlin.Int <1234789>)
    ◾ to equal: 2        (kotlin.Int <1234789>)
◆ to contain only, in order: 
  ✔ ▶ element 0: 1        (kotlin.Int <1234789>)
      ◾ to equal: 3        (kotlin.Int <1234789>)
  ✘ ▶ element 1: 2        (kotlin.Int <1234789>)
      ◾ to be less than: 2        (kotlin.Int <1234789>)
    ❗❗ additional elements detected: 
       ⚬ element 2: 2        (kotlin.Int <1234789>)
       ⚬ element 3: 4        (kotlin.Int <1234789>)

I therefore suggest, we focus on simple expect(...).toEqual() cases for now (as a next step we could support things like:

expect(person) {
  firstName.toEqual("Dmitriy")
  lastName.toEqual("Voronin")
}

)

Question 1
Do you agree or do you actually have use cases where you would like to have a diff in a different scenario?

---

Currently Intellij uses the following format in the output in case expected and actual fit on one line

Expected :hello
Actual   :world
<Click to see difference>

org.opentest4j.AssertionFailedError: expected: <hello> but was: <world>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)

We can influence what is written after Expected:, Actual: and the exception message but we cannot influence the order.
This is a pity as even for simple things like:

expect("hello").toEqual("world")

We end up with something like

Expected :hello
Actual   :world
<Click to see difference>

I expected subject : "hello"
◆ to equal         : "world"

at com.example.Test.foo(com.example.Test.kt:151)
....

Which is too noisy IMO. Therefore my suggestion would be that we always append something to expected and actual so that we end up with the following error message in the console:

I expected subject : "hello"
◆ to equal         : "world"

at com.example.Test.foo(com.example.Test.kt:151)
....

And the diff would look then as follows (so my suggestion would be to append \n====== diff powered by Atrium):

Question 2
What do you think about this idea?

---

I see the intellij diff view as quick-win, long term it would be nicer IMO if the diff would be included in the error message directly, i.e. something like

Question 3
Do you also think this would be more beneficial long term?

---

If you have any other suggestions, please raise your voice 🙂

[dsvoronin]

@robstoll

Question 1

Most common scenario for me is diff between data classes, or long enough strings to be hard to spot difference in console. That would fit the "simple one value" scenario.

Simple blue-ish diff highlighting in Idea helps a lot here.

Sometimes i prefer to diff data classes instead of using feature extraction, which could help here too. It's just easier to maintain mostly.

Question 2

Small diffs usually simple to spot, big data classes/strings would require soft-wrapping, big window, so i would say it could be helpful in some cases, but not completely covers Idea diff

Question 3

No idea, really. Could it make things worse, for example for people with non-standard color schemas in terminal?

Should I already see aligned expected/actual : ? For latest version it's not I believe. This alone would be enough for some cases.

[robstoll]

@dsvoronin thanks for the feedback 🙂 👍

Atrium v1.2.0 does not include the alignment yet but it's implemented on the proof branch:
https://github.com/robstoll/atrium/blob/feature/proof-based-reporter/README.md#ex-first

This branch also includes the <Click to see difference>-link for simple cases. I could built a nightly version if you are interested to try it out already. It's not complete yet though... but feedback would be welcome

[Nikita-Guchakov]

1. Well, I've checked behavior for soft assertions in IDEA for assertj/assertk and diff works fine for multiple failures now.
   Don't see wether assertj throws MultipleFailuresError in the output, but assertk do.

val subject = "1"
        SoftAssertions.assertSoftly{
            it.assertThat(subject)
                .isEqualTo("2")
                .isEqualTo("30")
        }

[Nikita-Guchakov]

Do you agree or do you actually have use cases where you would like to have a diff in a different scenario?

diff for assertEquals scenarios is fine! Though it should work for things like string, collections, arrays, maps, objects, numbers.
I really think it's ok to check other assertion libs behavior. Assertj for example throws java.lang.AssertionError for many cases, where doesn't seem suitable, instead of opentest4j.

---

I've checked up your discussion above and really glad that you go with opentest4j solution, cause

1. other libs've done same, and when we speak about using some "common ground", like assertion spec, it's nice choice to implement same and contribute with proposals to spec, I believe it will do great for community.
   For example your discussion what exception should be thrown in case of contains assertion is a good question for their spec.
2. IDEA is not the only IDE for jvm, I haven't check VCS, but if they have/or will implement some handling for assertion errors, they do/will cover opentest4j too, so your implementation will work automatically for all existed/future IDEs that supports spec:)

[Nikita-Guchakov]

I see the intellij diff view as quick-win, long term it would be nicer IMO if the diff would be included in the error message directly

As for me it's not a problem to use "click to show diff", it allows to navigate between not matching parts,
but I use it for long lines/objects.
For small output it would be nice to keep it simple.
As for contrary example Atrium's expected:<"3[0]"> but was:<"3[]"> is a noisy for my taste, my brain doesn't take it as smoothly as if it were expected:<"30"> but was:<"3"> :) But if it will be configurable per user (like envs or some~/$XDG_CONFIG_HOME/.atrium.yaml - then ok

[robstoll]

@Nikita-Guchakov thanks for the feedback

I have come across use cases where the diff view of intellij was helpful even for short text due to hidden chars (e.g. different line ending, different spaces, background/foreground colour etc.).
As long as we don't have an equivalent feature (making invisible chars visible) the <click to see difference> link might always be helpful and I would include it in every case -- I don't intend to spend too much effort in rebuilding this logic if intellij already has it.

As for now I see the coloured diff as addition which might be especially helpful in CI and on non-JVM platforms (e.g. when running JS tests). Of course, colours only help if the terminal supports them, where windows is an enemy 😆

I don't think MultipleFailuresError will be a good fit for Atrium as provided out of the box as it splits the error message into multiple parts (which kind of goes against the reporting strategy of atrium). But having the possibility for mulitple <click to see difference> links is of course nice. I will check later (i.e. in 1.4.0) if we can misuse it somewhat.

[robstoll]

@Nikita-Guchakov @dsvoronin and others reading this, I have released v1.3.0-alpha-1 which includes the click to see difference link:
https://github.com/robstoll/atrium/releases/tag/v1.3.0-alpha-1

Please try it out and give feedback, especially if you should find bugs or if something seems odd in reporting. Thanks