Migrate to JUnit 5 and unify used test API #3138

IgnatBeresnev · 2023-08-23T21:10:49Z

Got tired of discussions and creating issues, so I needed switch my brain off and code something monotonous :) This will partially help with #3112 as I will be making some analysis API public, and I will need to write tests for it.

Changes:

JUnit 5 is used across the project, except for examples. JUnit 4 should no longer be visible among dependencies.
kotlin-test is used as much as possible, that includes, but not limited to
- kotlin.test.Test (replaced all org.junit.Test and org.junit.jupiter.api.Test)
- kotlin.test.assertEquals (replaced all org.junit.Assert.assertEquals and org.junit.jupiter.api.Assertions.assertEquals)
- kotlin.test.assertNotNull (replaced all kotlin.requireNotNull()) and similar
- kotlin.test.assertTrue (replaced all kotlin.check(), kotlin.require(), kotlin.assert()) and similar
- kotlin.test.BeforeTest (replaced all org.junit.jupiter.api.BeforeEach) and similar
- kotlin.test.Ignore (replaced all org.junit.jupiter.api.Disabled)
Removed the dependency on assertk
Isolated the usage of Jupiter API, it's now only present in base, templating and integration tests.

Caveats:

Integration tests heavily realy on JUnit's parameterized tests, and their names are reported incorrectly by Gradle (JUnit 5 Dynamic tests not showing with their displayName gradle/gradle#5975). It was fixed in 8.1, but we cannot update to it until Refactor artifact publishing #3132 is implemented. It works and is reported fine in IDEA though, so it's not all bad.
The test setup of our integration tests is far from perfect - they rely on JUnit's API and need kotlin-test-junit5 specificaly. I hope this will be addressed in Integration tests refactoring #2925
Some Jupiter API is still used, I found no replacement for it in kotlin-test:
- org.junit.jupiter.params.ParameterizedTest (along with related classes) - used to run the same test with various parameters
- org.junit.jupiter.api.io.TempDir - used for creating temporary directories to which test data is dumped, could be replaced by our own solution, but I didn't have much time, so it's a task for another day.
- org.junit.jupiter.api.TestFactory - found no direct substitution
- org.junit.jupiter.api.RepeatedTest - found no direct substitution

IgnatBeresnev · 2023-08-24T01:08:34Z

core/test-api/src/main/kotlin/testApi/testRunner/TestRunner.kt

+    private fun withTempDirectory(cleanUpAfterUse: Boolean, block: (tempDirectory: File) -> Unit) {
+        val tempDir = this.createTempDir()
+        try {
+            block(tempDir)
+        } finally {
+            if (cleanUpAfterUse) {
+                tempDir.delete()
+            }
+        }
+    }


Didn't want to add the JUnit dependency only for the temp directories feature, so I implemented it myself.

IgnatBeresnev · 2023-08-24T01:11:03Z

...le/src/integrationTest/kotlin/org/jetbrains/dokka/it/gradle/Android0GradleIntegrationTest.kt

-    companion object {
-        @get:JvmStatic
-        @get:Parameters(name = "{0}")
-        val versions = TestedVersions.ANDROID


The way parameterized tests are set up is very different in JUnit 5 compared to JUnit 4.

In JUnit 4 it was possible to provide parameters for the whole test class, and then pass it using inheritance. In JUnit 5 it's not possible, and the API was changed - now we have to specify parameters for each test individually, and the tests have to be marked with @ParameterizedTest. I made it work, but didn't think much about the beauty of it - the integration tests framework will likely be re-written once we migrate to the new Gradle plugin.

vmishenev

Also, there are plenty of places where assert is used in tests, e.g. JsoupUtils

core/content-matcher-test-utils/src/main/kotlin/matchers/content/ContentMatchersDsl.kt

vmishenev · 2023-08-23T22:20:48Z

plugins/base/src/test/kotlin/expect/AbstractExpectTest.kt

            gitCompare.inputStream.bufferedReader().lines().forEach { logger.info(it) }
-            assertTrue(gitCompare.exitValue() == 0) { "${path.fileName}: outputs don't match" }
+            assertTrue(gitCompare.exitValue() == 0, "${path.fileName}: outputs don't match")


assertEquals

I've fixed the specific places that you outlined since you reviewed them already, but I think such changes fall outside of the scope of this PR, so I won't look for any other similar improvement opportunities on my own

I'm all for correcting and improving all of the asserts, like using assertContains instead of assertTrue(list.contains()), but I think it's best done as a separate change that would be easier to review and will not be blocking anything

plugins/base/src/test/kotlin/expectActuals/ExpectActualsTest.kt

plugins/base/src/test/kotlin/utils/TestUtils.kt

vmishenev · 2023-08-23T22:39:45Z

plugins/base/src/test/kotlin/translators/DefaultDescriptorToDocumentableTranslatorTest.kt

@@ -812,7 +810,7 @@ val soapXml = node("soap-env:Envelope", soapAttrs,
                val testFunction = module.packages.find { it.name == "com.example" }
                    ?.functions
                    ?.single { it.name == "foo" }
-                checkNotNull(testFunction)


By the way, we have special DSL T?.notNull

Indeed, I also noticed it, but I'm personally not a fan of it... It's unclear why it exists, but it for sure breaks the consistency with other asserts, so I'd advocate to get rid of it down the road when we refactor asserts

IgnatBeresnev · 2023-08-29T20:00:27Z

Also, there are plenty of places where assert is used in tests, e.g. JsoupUtils

JsoupUtils is from the public test API (dokka-base-test-utils), which is not in a test source set, so I tried not to touch such places when it comes to assert, require, checkNotNull and so on, to avoid accidentally introducing bugs, as there was no way to know if was used as test assert or to validate inputs. I could look at usages on a case-by-case basis, but it's not the objective here. assertEquals and similar functions were replaced in bulk via Find & Replace, and if it was already used in such modules - it could've only been used for one thing.

I don't see any other usages of assert() in test sources specifically, so if you see any - I'd appreciate it if you could point me to a class, perhaps it didn't get indexed or something

vmishenev · 2023-08-30T02:05:05Z

I don't see any other usages of assert() in test sources specifically, so if you see any - I'd appreciate it if you could point me to a class, perhaps it didn't get indexed or something

I have found

IgnatBeresnev · 2023-08-30T10:02:24Z

I have found

Could you please check that you have the correct branch, and then go to one of the search results, verify that kotlin.assert() is indeed used there, and point me to this specific place? I'm unable to find any, and invalidating IDEA caches doesn't help

There's only a single test class in mathjax.test where the 3 usages from your screenshot have likely been fixed, if we're talking about the same thing. Diff for MathjaxPluginTest. It case it doesn't wanna get loaded due to the size of the PR, here's a screenshot:

screenshot

I'll rebase onto master to resolve merge conflicts now, so the diff link might get corrupted after the force push

vmishenev · 2023-08-30T12:02:50Z

Sorry, my bad. I have not checked out to this branch.
There is only

dokka/plugins/base/src/test/kotlin/markdown/ParserTest.kt

Line 1486 in 0872cfd

assert(

but it is after rebasing.
Everything is ok, you can merge.

IgnatBeresnev · 2023-08-30T12:50:32Z

Oh, good

There is only but it is after rebasing.

Replaced it as well while we're at it :)

Will wait for the unit tests to pass and then merge it, without waiting for integration tests since they've passed just now and nothing significant was changed

IgnatBeresnev requested a review from vmishenev August 23, 2023 21:10

IgnatBeresnev self-assigned this Aug 23, 2023

IgnatBeresnev mentioned this pull request Aug 24, 2023

Enable explicit API mode #3139

Merged

IgnatBeresnev commented Aug 24, 2023

View reviewed changes

vmishenev approved these changes Aug 29, 2023

View reviewed changes

IgnatBeresnev added 3 commits August 30, 2023 12:18

Migrate to JUnit 5 and unify used test API

0872cfd

Remove the usage of Jupiter's assumeTrue

62ed7aa

Address review comments

9e24b10

IgnatBeresnev force-pushed the junit5 branch from 3585e97 to 9e24b10 Compare August 30, 2023 10:24

Replace assert usage after rebasing

60ca368

IgnatBeresnev merged commit c63ea36 into master Aug 30, 2023

IgnatBeresnev deleted the junit5 branch August 30, 2023 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to JUnit 5 and unify used test API #3138

Migrate to JUnit 5 and unify used test API #3138

IgnatBeresnev commented Aug 23, 2023

IgnatBeresnev Aug 24, 2023

IgnatBeresnev Aug 24, 2023

vmishenev left a comment •

edited

Loading

vmishenev Aug 23, 2023

IgnatBeresnev Aug 29, 2023

vmishenev Aug 23, 2023

IgnatBeresnev Aug 29, 2023

IgnatBeresnev commented Aug 29, 2023 •

edited

Loading

vmishenev commented Aug 30, 2023 •

edited

Loading

IgnatBeresnev commented Aug 30, 2023 •

edited

Loading

vmishenev commented Aug 30, 2023

IgnatBeresnev commented Aug 30, 2023

Migrate to JUnit 5 and unify used test API #3138

Migrate to JUnit 5 and unify used test API #3138

Conversation

IgnatBeresnev commented Aug 23, 2023

IgnatBeresnev Aug 24, 2023

Choose a reason for hiding this comment

IgnatBeresnev Aug 24, 2023

Choose a reason for hiding this comment

vmishenev left a comment • edited Loading

Choose a reason for hiding this comment

vmishenev Aug 23, 2023

Choose a reason for hiding this comment

IgnatBeresnev Aug 29, 2023

Choose a reason for hiding this comment

vmishenev Aug 23, 2023

Choose a reason for hiding this comment

IgnatBeresnev Aug 29, 2023

Choose a reason for hiding this comment

IgnatBeresnev commented Aug 29, 2023 • edited Loading

vmishenev commented Aug 30, 2023 • edited Loading

IgnatBeresnev commented Aug 30, 2023 • edited Loading

vmishenev commented Aug 30, 2023

IgnatBeresnev commented Aug 30, 2023

vmishenev left a comment •

edited

Loading

IgnatBeresnev commented Aug 29, 2023 •

edited

Loading

vmishenev commented Aug 30, 2023 •

edited

Loading

IgnatBeresnev commented Aug 30, 2023 •

edited

Loading