Truth seems to import JUnit 4, even on JUnit 5

[jbduncan]

It occurs to me that the core/ subproject's pom.xml imports JUnit 4 as a non-test dependency. I think this is fine for projects that use JUnit 4 and Truth together for testing, but I'm a little concerned that for projects using JUnit 5 it will cause JUnit 4 to be unnecessarily downloaded and depended upon.

Are there any plans to move any parts of Truth which depend on JUnit 4 APIs into their own extensions?

[cpovirk]

Unfortunately, the parts of Truth that depend on JUnit 4 are basically all "all of them," since we throw ComparisonFailure.

There are at least a couple approaches we could take here:

• Throw ComparisonFailure conditionally only if it's available at runtime.
• Adopt the opentest4j types once they're finalized.

Overall, I'm not too concerned about a dependency on JUnit 4 in Truth, since it's a test framework. But if it's causing people trouble, we should reconsider.

[dimo414]

Throwing ComparisonFailure only if it's already on the classpath would be a silent regression for users not using JUnit4, so (as you say, assuming it's not causing real trouble) I think waiting for opentest4j makes the most sense.

[kashike]

opentest4j 1.0.0 was released on September 10th.

[cpovirk]

Cool, thanks.

[Androbin]

Since #40 is merged, we can close this now.

[jbduncan]

@Androbin Sorry, I'm unclear on how #40 is related to this issue. Would you kindly clarify things for me?

[Androbin]

Oops, I meant https://github.com/square/misk/pull/40 😁

[cpovirk]

Oh, thanks for the link:

Until this is resolved, we should be careful to use the @org.junit.jupiter.api.Test annotation and not @org.junit.Test by mistake.

That's a good downside for us to keep in mind. Inside Google, I think we're pretty much resigned to having JUnit 4 on our classpath, and people haven't started using JUnit 5 much. But externally, things are different.

This is still something we could consider doing someday. It's just not at the top of the list currently.

[Androbin]

Isn't there a way to search & replace "en masse" across the codebase? Or is this simply not feasible?

[cpovirk]

We can do search and replace. My fears are:

• Some people will be catching particular exception types, and their code will break. They ideally wouldn't be doing that, but we still have to clean them up inside Google, and others will have to fix it outside Google :\ Of course, the sooner we make the change, the better, so that the fallout is smaller.

• We'd need to get the opentest4j exceptions working under GWT and Android [update: all the way back to Ice Cream Sandwich] -- which is probably straightforward but sometimes turns out not to be.

• The opentest4j annotations still look possibly not officially finalized -- or maybe the docs just haven't been updated yet, since they're used in JUnit 5?

None of these are necessarily huge problems; there just didn't seem to be a big upside to eliminating the JUnit 4 dependency. The release of JUnit 5 may change that, and it calls attention to a larger problem with multiple @Test annotations.

[cpovirk]

Oh, and we should also check whether IDEs have begun supporting the opentest4j AssertionFailedError. I gather that they probably have, but I'd want to be sure. (e.g., I found this comment suggesting that IntelliJ has problems, but it seems hard to believe. I'd still want to verify.)

[childnode]

@Androbin

Isn't there a way to search & replace "en masse" across the codebase? Or is this simply not feasible?

just have a look what the gradle guys do with their codebase: https://github.com/gradle/gradle/pull/4116 << tl;dr: auto converting and running junit 4 tests with junit 5 platform both vintage and converted https://github.com/gradle/gradle/pull/4116/files#diff-a5945fda2451f7bab0e91c578e8e7f2eR49 ff

BUT: this is just done for the gradle tests itself and not intended to be used for other projects neither as production code

THIS truth one has other impacts, but if you ask me: just deprecate Junit 4 support and branch for Junit 5 ;)

[cpovirk]

We should at least consider migrating to the new extensions before 1.0. My guess is that we'll ultimately not consider this to be a true 1.0 blocker, but I'll mark it for consideration.

[cpovirk]

This might include eliminating TruthJUnit in favor of something on Truth itself.

Alternatively, it might include concluding that TruthJUnit is a weird name for a class that contains assume -- and then doing something about that.

[cpovirk]

Another thing that opentest4j offers is MultipleFailuresError, which would be nice for Expect.

[ptahchiev]

This issue is blocking me to migrate to JUnit5 :( I use google-compile testing framework which brings truth as transitive dependency and you guys have hard-coded junit4 classes in there (JUnitComparisonFailure)

[cpovirk]

My current thinking is that we won't fix this for 1.0 (which we're going to try to make our final release with breaking changes) because we can still theoretically fix it afterward by:

• changing our calls to FailureStrategy.fail to pass an AssertionFailedError (a subtype of AssertionError)
• making JUnit 4 an optional dependency that users can add if they want to use TruthJUnit I still hope to have a look at all that someday, but it's not imminent.

[cpovirk]

Oh, and I meant to add that, as part of opentest4j support, we'd also want to look for the opentest4j IncompleteExecutionException (and subtypes) in Expect.

[jbduncan]

Sounds good to me @cpovirk! 👍

[ptahchiev]

Any updates on this? I'd really like to see you guys support JUnit5 so we can move the google-compile-testing to JUnit5 too.

[stolsvik]

We use truth in some of our projects along with JUnit 5 (Jupiter), and due to this hard JUnit 4 dep we get an annoying "no [junit 4] tests found" message, in red (IntelliJ!), over 4 lines for each test run we do. It is a broken window, thorn in my side that really is putting me off this combo.

Why can't you instead throw AssertionError with a message or somesuch if JUnit 4 is not on the classpath?

[cpovirk]

Thanks. It's helpful to hear of a concrete problem this is causing -- but of course it's sad to hear that such a problem exists :(

Since your goal isn't to gain features from the new opentest4j types but rather just to work without JUnit 4 on the classpath, I suspect that we can hack something up. I will try to give it a shot in the next week or so, though no 100% guarantee on that.

(I wonder if IntelliJ could also be changed to avoid issuing this error. If you've done any digging into their forums or issue tracker, I'd be happy to follow along with anything you've found. But I certainly don't want to pressure you to spend more time on this than you have already.)

[cpovirk]

@stolsvik, please try version 1.1.1 and let me know if that resolves the problem you are encountering.

[tbroyer]

Fwiw, I use with success:

    testImplementation("com.google.truth:truth:1.1") {
        // See https://github.com/google/truth/issues/333
        exclude(group = "junit", module = "junit")
    }
    testRuntimeOnly("junit:junit:4.13") {
        // See https://github.com/google/truth/issues/333
        because("Truth needs it")
    }
    testImplementation("org.junit.jupiter:junit-jupiter-api:5.7.0")
    testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:5.7.0")

https://github.com/tbroyer/gradle-nullaway-plugin/blob/b87e1f5f0a782944717a4bd1f594fd8566f4ff5c/build.gradle.kts#L61-L70

My tests are in Kotlin and not Java, but IntelliJ IDEA (2020.3.1) doesn't complain. I'm also using TruthJUnit.assume() with no problem as JUnit 4 types are not exposed in the API (if Truth were built with Gradle, it'd be a perfect case for implementation vs api, where Gradle would generate a <scope>runtime</scope> in the published POM).

As an exercise, I tried upgrading to Truth 1.1.1 and removing the testRuntimeOnly("junit:junit:4.13"), after commenting out all my usages of TruthJUnit but that fails right from the Truth clinit:

java.lang.NoClassDefFoundError: org/junit/ComparisonFailure
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)
	at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
	at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:174)
	at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:800)
	at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:698)
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:621)
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:579)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
	at java.base/java.lang.ClassLoader.defineClass1(Native Method)
	at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1017)
	at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:174)
	at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:800)
	at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:698)
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:621)
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:579)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
	at com.google.common.truth.StandardSubjectBuilder.forCustomFailureStrategy(StandardSubjectBuilder.java:55)
	at com.google.common.truth.Truth.<clinit>(Truth.java:85)
	at net.ltgt.gradle.nullaway.AbstractPluginIntegrationTest.compilation succeeds(AbstractPluginIntegrationTest.kt:85)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
[…]
Caused by: java.lang.ClassNotFoundException: org.junit.ComparisonFailure
	at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:581)
	at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:178)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:522)
	... 111 more

[cpovirk]

Thanks, that is odd. The only mention of org.junit.ComparisonFailure in that release should be here: https://github.com/google/truth/blob/release_1_1_1/core/src/main/java/com/google/common/truth/Platform.java#L156

And the only reference to that should be here: https://github.com/google/truth/blob/release_1_1_1/core/src/main/java/com/google/common/truth/ComparisonFailureWithFacts.java#L30

And the only reference to that should be the guarded one here: https://github.com/google/truth/blob/release_1_1_1/core/src/main/java/com/google/common/truth/FailureMetadata.java#L190

And the (Google-internal) test I added, which fails without my changes, passes with them. So something interesting is going on.... I've cloned your repo, and I'm investigating.

[cpovirk]

I can confirm that I can reproduce it in your project. For my reference, my debugging steps so far:

$ git co b87e1f5

$ vi build.gradle.kts
... update to 1.1.1 and remove testRuntimeOnly junit dep ...

$ ./gradlew dependencies --write-locks

$ vi $(grep -rl TruthJUnit src)
... remove references ...

$ vi build.gradle.kts
... edit `test` to contain `jvmArgs("-verbose:class")` ...

$ ANDROID_SDK_ROOT=$HOME/android-sdk-linux ./gradlew --no-build-cache cleanTest test --tests '*AndroidIntegrationTest*missing annotated packages option*' &> /tmp/everything

[tbroyer]

I pushed my changes to a branch: https://github.com/tbroyer/gradle-nullaway-plugin/tree/truth-1.1.1 And the build indeed fails: https://github.com/tbroyer/gradle-nullaway-plugin/runs/1750932768

(edit: updated branch and link to build run, as I had forgotten to push the test changes)

[cpovirk]

Thanks.

Even -XX:+TraceClassLoading and -XX:+TraceClassResolution do not clear things up for me. But then I had never even heard of those until just now, so I don't necessarily know how to interpret them :)

[edit: nor -XX:+TraceExceptions]

I have dumped all the bytecode in the Truth jar, and the only references to ComparisonFailure are the expected ones from Platform.PlatformComparisonFailure.

Platform.PlatformComparisonFailure is in turn referenced from ComparisonFailureWithFacts, also as expected. But it also shows up in the bytecode for Platform (in InnerClasses), and maybe that's biting us?

[cpovirk]

Perhaps more likely: The try-catch approach in FailureMetadata.failEqualityCheck (which is indeed the only reference to ComparisonFailureWithFacts) is not good enough. I seem to recall that Java is allowed to load classes sooner than you might hope:

Errors detected during linkage are thrown at a point in the program where some action is taken by the program that might, directly or indirectly, require linkage to the class or interface involved in the error.

Probably we should use reflection. There's always the chance that that will break something else, but "luckily" we do at least some of that already: https://github.com/google/truth/blob/release_1_1_1/core/src/main/java/com/google/common/truth/Platform.java#L98

[cpovirk]

Backing up a bit as I wait for some tests to run:

Thanks for your workaround above: It's probably the best current solution for people willing to go to the trouble to set it up, since it lets Truth continue to throw a true ComparisonFailure, which can be displayed more nicely by IDEs and other tools.

Still, I'm working on the reflective approach so that people who want to simply exclude JUnit without worrying about re-including it at runtime can get by with that simpler approach.