-
Notifications
You must be signed in to change notification settings - Fork 260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Truth seems to import JUnit 4, even on JUnit 5 #333
Comments
Unfortunately, the parts of Truth that depend on JUnit 4 are basically all "all of them," since we throw There are at least a couple approaches we could take here:
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. |
Throwing |
opentest4j 1.0.0 was released on September 10th. |
Cool, thanks. |
Since #40 is merged, we can close this now. |
Oops, I meant cashapp/misk#40 😁 |
Oh, thanks for the link:
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. |
Isn't there a way to search & replace "en masse" across the codebase? |
We can do search and replace. My fears are:
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 |
Oh, and we should also check whether IDEs have begun supporting the opentest4j |
just have a look what the gradle guys do with their codebase: gradle/gradle#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 ;) |
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. |
This might include eliminating Alternatively, it might include concluding that |
Another thing that |
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 ( |
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:
|
Oh, and I meant to add that, as part of opentest4j support, we'd also want to look for the opentest4j |
Sounds good to me @cpovirk! 👍 |
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. |
… real this time. This should *really* let users exclude the JUnit 4 dependency -- again, as long as they don't use Expect, ExpectFailure, or TruthJUnit (i.e., assume()). This change makes the previously failing test procedure described in #333 (comment) pass. RELNOTES=Made it possible for users to exclude our JUnit 4 dependency and still use standard Truth assertions -- *really* this time. (JUnit 4 is still required for some advanced features, like `Expect`, `ExpectFailure`, and `TruthJUnit.assume()`.) PiperOrigin-RevId: 353318639
… real this time. This should *really* let users exclude the JUnit 4 dependency -- again, as long as they don't use Expect, ExpectFailure, or TruthJUnit (i.e., assume()). This change makes the previously failing test procedure described in #333 (comment) pass. RELNOTES=Made it possible for users to exclude our JUnit 4 dependency and still use standard Truth assertions -- *really* this time. (JUnit 4 is still required for some advanced features, like `Expect`, `ExpectFailure`, and `TruthJUnit.assume()`.) PiperOrigin-RevId: 353318639
… real this time. This should *really* let users exclude the JUnit 4 dependency -- again, as long as they don't use Expect, ExpectFailure, or TruthJUnit (i.e., assume()). This change makes the previously failing test procedure described in #333 (comment) pass. RELNOTES=Made it possible for users to exclude our JUnit 4 dependency and still use standard Truth assertions -- *really* this time. (JUnit 4 is still required for some advanced features, like `Expect`, `ExpectFailure`, and `TruthJUnit.assume()`.) PiperOrigin-RevId: 353416036
OK, 1.1.2 contains the fix (and another fix that I noticed I needed along the way...). Sorry for all the trouble. It's still conceivable that something will go wrong in some case ( Please continue to let us know of new problems that the JUnit dependency causes. The ones I'm aware of:
|
1.1.2 looks good 👍 @cpovirk. No more broken window spam when utilizing truth in projects with jupiter and excluding junit. (Work with stolsvik, so ack from us) |
@cpovirk the |
And that's, uh, expected: https://github.com/google/truth/releases/tag/release_1_1_2
|
🤦 missed that, thanks! |
We don't have a clear plan for JUnit 5 at this point :( I want to say that we ought to be able to extract the purely static version of ExpectFailure into a class with no JUnit dependency, but I'd have to confirm. And ideally, we'd want to get at least the beginnings of a long-term plan before copying things around. In the meantime, copying and pasting the methods from ExpectFailure into your own codebase doesn't require too much duplication, so it may be a workable option (depending on licensing and other such questions). |
Related: Nicer way to support soft assertions (Expect.create junit4 Rule) in JUnit5? #893 (patch available) |
@cpovirk is this still on hold? |
Truth still declares a build dependency on JUnit: Lines 21 to 24 in ceecbd4
As of Truth 1.1.2, you can exclude that dependency, and Truth will work at runtime. (It just won't throw a That's as far as we're likely to take this for a long while, unfortunately. |
If that's the case would it make sense to document how to use Truth with JUnit 5 with detail on what features are lost to that exclusion and how to get them back in JUnit 5? I really like how detailed https://truth.dev/comparison is, would love to see a similar page for JUnit 5. |
It would :( Truth is very much on the back burner. I do hope to change that someday. Offhand, I think you lose only |
According to the junit 5 codebase, it looks like they use the |
…yReflection`. This makes it possible to write a config that instructs Proguard to keep that constructor if it keeps the class. I'm hoping that I don't need to explicitly instruct it to keep the class, since the class name appears as a literal argument to `Class.forName`. (I could just tell it to keep it "just to be safe," but then maybe we'd see problems with users who don't actually use Truth but end up with it at runtime anyway because it's somewhere in the transitive deps and we asked Proguard to keep it.) (The reason we're using reflection in the first place is to support running without JUnit 4 on the classpath. See #333.) RELNOTES=n/a PiperOrigin-RevId: 462420279
…yReflection`. This makes it possible to write a config that instructs Proguard to keep that constructor if it keeps the class. I'm hoping that I don't need to explicitly instruct it to keep the class, since the class name appears as a literal argument to `Class.forName`. (I could just tell it to keep it "just to be safe," but then maybe we'd see problems with users who don't actually use Truth but end up with it at runtime anyway because it's somewhere in the transitive deps and we asked Proguard to keep it.) (The reason we're using reflection in the first place is to support running without JUnit 4 on the classpath. See #333.) RELNOTES=n/a PiperOrigin-RevId: 462826082
RELNOTES=Speed up the first `GoogleLogger` construction. PiperOrigin-RevId: 586132513
fixes #2728, attempt to remove Junit 4 support after migration. Other than POM dependency migrate, changes include: - package name changes - Junit 5 syntax upgrades, e.g. `@Before` --> `@BeforeEach`, Replace assertion methods - remove public modifier on tests and test classes. - Refactor JUnit 4 TemporaryFolder `@Rule` in [ITGdch.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-6ae7755a0b038e1a2febae2d27e36c762f6751b8c7db577421667069399884b4) to JUnit 5 `@TempDir` - Replace `@Test(timeout = 15000L)` in [ITClientShutdown.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-70d1df57471178a7a63302f82e4a4855ffbbd642ea67d92d501bd1f7008957ca) with `@Timeout(15)` - Update `@RunWith(Parameterized.class)` test in [ITHttpAnnotation.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-03d420650ecc9fe78ad4887761043c4fdceaa978f464ce30cfc4ed5f8be9b64d) to `@ParameterizedTest` with `@MethodSource("data")` ~~Note: #2737 creates a new test class with JUnit4 syntax. Depending on merging order, I will either update in this pr, or #2737.~~ Updated. Due to truth library depending on junit 4 ([see issue](google/truth#333)), junit 4 cannot be completely removed, or will encounter `java.lang.ClassNotFoundException: org.junit.runner.notification.RunListener` running tests with maven surefire. To keep things cleaner, excluding the implicitly junit brought in from truth and `junit-vintage-engine`. We could also do the reverse, and make a comment if that's prefered. --------- Co-authored-by: Burke Davison <[email protected]> Co-authored-by: Lawrence Qiu <[email protected]>
fixes #2728, attempt to remove Junit 4 support after migration. Other than POM dependency migrate, changes include: - package name changes - Junit 5 syntax upgrades, e.g. `@Before` --> `@BeforeEach`, Replace assertion methods - remove public modifier on tests and test classes. - Refactor JUnit 4 TemporaryFolder `@Rule` in [ITGdch.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-6ae7755a0b038e1a2febae2d27e36c762f6751b8c7db577421667069399884b4) to JUnit 5 `@TempDir` - Replace `@Test(timeout = 15000L)` in [ITClientShutdown.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-70d1df57471178a7a63302f82e4a4855ffbbd642ea67d92d501bd1f7008957ca) with `@Timeout(15)` - Update `@RunWith(Parameterized.class)` test in [ITHttpAnnotation.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-03d420650ecc9fe78ad4887761043c4fdceaa978f464ce30cfc4ed5f8be9b64d) to `@ParameterizedTest` with `@MethodSource("data")` ~~Note: #2737 creates a new test class with JUnit4 syntax. Depending on merging order, I will either update in this pr, or #2737.~~ Updated. Due to truth library depending on junit 4 ([see issue](google/truth#333)), junit 4 cannot be completely removed, or will encounter `java.lang.ClassNotFoundException: org.junit.runner.notification.RunListener` running tests with maven surefire. To keep things cleaner, excluding the implicitly junit brought in from truth and `junit-vintage-engine`. We could also do the reverse, and make a comment if that's prefered. --------- Co-authored-by: Burke Davison <[email protected]> Co-authored-by: Lawrence Qiu <[email protected]>
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?
The text was updated successfully, but these errors were encountered: