Improved IDE experience for JUnit5 - visual comparison failure

[kriegfrj]

When JUnit5+Mockito is used in modern IDE (IDEA, Eclipse) we now show "visual" comparison failure pop-up for certain Mockito exceptions (such as ArgumentsAreDifferent).

Fixes #1663.

[codecov-io]

Codecov Report

Merging #1667 into release/2.x will decrease coverage by 0.01%.
The diff coverage is 91.3%.

@@                Coverage Diff                @@
##             release/2.x    #1667      +/-   ##
=================================================
- Coverage          87.36%   87.34%   -0.02%     
  Complexity          2442     2442              
=================================================
  Files                301      302       +1     
  Lines               6293     6307      +14     
  Branches             787      788       +1     
=================================================
+ Hits                5498     5509      +11     
- Misses               600      603       +3     
  Partials             195      195

Impacted Files	Coverage Δ	Complexity Δ
...a/org/mockito/internal/junit/ExceptionFactory.java	100% <100%> (ø)	3 <3> (-2)	⬇️
...exceptions/verification/ArgumentsAreDifferent.java	80% <100%> (+13.33%)	2 <1> (+1)	⬆️
...verification/opentest4j/ArgumentsAreDifferent.java	77.77% <77.77%> (ø)	2 <2> (?)
...ions/verification/junit/ArgumentsAreDifferent.java	66.66% <0%> (-11.12%)	1% <0%> (-1%)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a323b81...42f802e. Read the comment docs.

[TimvdLippe]

Do you have any instructions on how I can test this PR to verify the difference in IDE behavior?

[kriegfrj]

Do you have any instructions on how I can test this PR to verify the difference in IDE behavior?

• Create a simple JUnit 5 test that fails with ArgumentsAreDifferent:

package mytest;

import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.verify;

import org.junit.jupiter.api.Test;

public class Test {
    @SuppressWarnings("unchecked")
    @Test
    public void test() {
        Comparable<Object> c = (Comparable<Object>)mock(Comparable.class);
        c.compareTo("test");
        
        verify(c).compareTo("somethingElse");
    }
}

• Make sure that the snapshot version of Mockito is on your classpath.
• Make sure JUnit 5 is on the test classpath. If you're using Mockito's workspace, you can do this with the following dependency declarations:
  • testCompile libraries.junitJupiterApi
  • testRuntime libraries.junitJupiterEngine
• Run the test in the IDE using the JUnit 5 runner.
• In Eclipse, you should get the following:
  
  I'm not sure how it looks in IDEA or Android Studio, but I'm sure it's similar.

[kriegfrj]

This implementation is not perfect. Under Eclipse, if you're using the JUnit 4 but happen to have OpenTest on the classpath, then you'll use the visual diff capability again. This is because the JUnit 4 runner in Eclipse doesn't try and catch/handle org.opentest4j.AssertionFailedError. I'm not sure of the behaviour in IDEA.

The only way around that would be for ExceptionFactory.createArgumentsAreDifferent() to examine the caller stack to see if it has been invoked from a JUnit 4 or a JUnit 5 context, and throw the most appropriate exception. This would complicate the implementation and make it run slower too. It would be better, IMO, if the JUnit 4 runner in the IDEs were made aware of the OpenTest4J exception and could handle it too.

[mockitoguy]

This implementation is not perfect. Under Eclipse, if you're using the JUnit 4 but happen to have OpenTest on the classpath, then you'll use the visual diff capability again.

Why do you call this behavior imperfect?

[mockitoguy]

This looks great, thank you!!!

Can you do 2 more checks for us?

• does it work with IDEA, too?
• can you build the project and inspect all pom files (they should be somewhere under "build" dirs) and ensure that we don't ship the new dependency?

[kriegfrj]

This implementation is not perfect. Under Eclipse, if you're using the JUnit 4 but happen to have OpenTest on the classpath, then you'll use the visual diff capability again.

Why do you call this behavior imperfect?

Sorry, it was a typo. It should have read "... then you'll lose the visual diff capability again." Perfect behaviour, in my mind, would be that it determined which runner was actually running (as opposed to merely being on the classpath), and throw the exception that was appropriate for that runner. But I think this is an edge case that probably isn't worth the extra complexity. Not perfect, but good enough.

[kriegfrj]

This looks great, thank you!!!

Can you do 2 more checks for us?

• does it work with IDEA, too?

This will be difficult for me to do, as I don't have IDEA installed. But I can't see why it wouldn't work

• can you build the project and inspect all pom files (they should be somewhere under "build" dirs) and ensure that we don't ship the new dependency?

Will do. I did note that I need to make sure that an optional Import-Package is added to the MANIFEST.MF too.

[mockitoguy]

This will be difficult for me to do, as I don't have IDEA installed.

Grab it from JetBrains site, IDEA community/open source edition is free and easy to install

[mockitoguy]

Sorry, it was a typo.

Got it! Thank you for confirming. Yes, the ideal behavior is that we get clean visual diff for all IDEs. Let's move on. It seems that having OpenTest4J + JUnit4 on classpath is an edge case. Also, I would hope that Eclipse will catch up and start handling OpenTest4J exceptions.

@TimvdLippe, this is an interesting argument for our discussion about adding hard dependency on OpenTest4J. If we add such dependency today, then Eclipse + JUnit4 + Mockito (with OpenTest4J) combo loses visual diff feature (unless we write code to detect the runner - a fragile complexity better avoided). Just an FYI. I don't mind to restart the discussion.

Thanks guys!!!

[TimvdLippe]

If we add such dependency today, then Eclipse + JUnit4 + Mockito (with OpenTest4J) combo loses visual diff feature (unless we write code to detect the runner - a fragile complexity better avoided).

I am absolutely fine with not adding the hard-dependency 😄

[kriegfrj]

Got it! Thank you for confirming. Yes, the ideal behavior is that we get clean visual diff for all IDEs. Let's move on.

👍

It seems that having OpenTest4J + JUnit4 on classpath is an edge case. Also, I would hope that Eclipse will catch up and start handling OpenTest4J exceptions.

Just to clarify (in case it wasn't clear): The problem is not merely having both OpenTest4J and JUnit4 on the class path. JUnit 5 and OpenTest4J have been deliberately designed so that they can happily co-exist with JUnit 4 on the same classpath.

The problem (in Eclipse at least) is if you mix the JUnit 4 runner with OpenTest4J exceptions (which this current PR will do if you have OpenTest4J on the classpath) - the visual diff will not work, because Eclipse's implementation of the JUnit 4 runner special-cases the JUnit 3 & 4 ComparisonFailure, but not the OpenTest4J AssertionFailedError (conversely, Eclipse's JUnit 5 runner handles only AssertionFailedError and not ComparisonFailure). If you use the JUnit 5 runner in Eclipse the visual diff will work, even if JUnit 4 is on the classpath.

@TimvdLippe, this is an interesting argument for our discussion about adding hard dependency on OpenTest4J. If we add such dependency today, then Eclipse + JUnit4 + Mockito (with OpenTest4J) combo loses visual diff feature (unless we write code to detect the runner - a fragile complexity better avoided). Just an FYI. I don't mind to restart the discussion.

Actually, the issue of losing the visual diff doesn't have anything to do with whether the dependency on OpenTest4J is hard or soft. It has more to do with how the decision is made about which exception to throw. The above problem would still occur even if we made the dependency on OpenTest4J a hard one, unless we did the same stack trace trick (ie, check which runner invoked us and throw the appropriate exception).

Thanks guys!!!

Welcome! Honoured to be able to meaningfully contribute to such a popular project!

[kriegfrj]

IDEA verification:

Note the "click to see difference" link in the error output, and the visual diff.

[TimvdLippe]

Could you maybe add a new subproject which has some sample tests with instructions on how to inspect the diff? That would ease future development so that we can check if regressed on that. Thanks for all the hard work, good progress!

[kriegfrj]

• can you build the project and inspect all pom files (they should be somewhere under "build" dirs) and ensure that we don't ship the new dependency?

Will do. I did note that I need to make sure that an optional Import-Package is added to the MANIFEST.MF too.

I did this - I can confirm that opentest4j does not appear as a dependency in any of the generated pom files.

I also added the manifest change: org.opentest4j is now an optional import to the OSGi manifest so that it would work properly in an OSGi environment.

[kriegfrj]

Could you maybe add a new subproject which has some sample tests with instructions on how to inspect the diff? That would ease future development so that we can check if regressed on that. Thanks for all the hard work, good progress!

I'll sleep on it, but to be honest my initial thoughts are that I feel this is overreaching a little. I think the Mockito tests should focus on the fact that it throws AssertionFailedError correctly, which I think they already do. Added to which I'm not sure I have the time.

Perhaps this could be the subject of a separate issue/PR which I can get to if I find time, or else someone else can take it on.

[mockitoguy]

Could you maybe add a new subproject which has some sample tests with instructions on how to inspect the diff? That would ease future development so that we can check if regressed on that. Thanks for all the hard work, good progress!

I suggest we move on. We won't be able to automatically test how IDEs integrate with xUnit runners. Manual tests will be forgotten. We never had tests for this feature and it did not regress. Eventual regression will not be a blocker. If we encounter one, then we can prioritize a solution.

[mockitoguy]

It is ready merge. @TimvdLippe, OK?

[TimvdLippe]

I didn't count it as a blocker, more as a future improvement. I am okay with merging as-is.