-
Notifications
You must be signed in to change notification settings - Fork 399
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
Migrate to JUnit 5 and unify used test API #3138
Conversation
private fun withTempDirectory(cleanUpAfterUse: Boolean, block: (tempDirectory: File) -> Unit) { | ||
val tempDir = this.createTempDir() | ||
try { | ||
block(tempDir) | ||
} finally { | ||
if (cleanUpAfterUse) { | ||
tempDir.delete() | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't want to add the JUnit dependency only for the temp directories feature, so I implemented it myself.
companion object { | ||
@get:JvmStatic | ||
@get:Parameters(name = "{0}") | ||
val versions = TestedVersions.ANDROID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
core/content-matcher-test-utils/src/main/kotlin/matchers/content/ContentMatchersDsl.kt
Outdated
Show resolved
Hide resolved
core/content-matcher-test-utils/src/main/kotlin/matchers/content/ContentMatchersDsl.kt
Outdated
Show resolved
Hide resolved
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") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertEquals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, we have special DSL T?.notNull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
I don't see any other usages of |
Could you please check that you have the correct branch, and then go to one of the search results, verify that There's only a single test class in I'll rebase onto master to resolve merge conflicts now, so the diff link might get corrupted after the force push |
Sorry, my bad. I have not checked out to this branch.
Everything is ok, you can merge. |
Oh, good
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 |
Fixes #2924
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:
kotlin-test
is used as much as possible, that includes, but not limited tokotlin.test.Test
(replaced allorg.junit.Test
andorg.junit.jupiter.api.Test
)kotlin.test.assertEquals
(replaced allorg.junit.Assert.assertEquals
andorg.junit.jupiter.api.Assertions.assertEquals
)kotlin.test.assertNotNull
(replaced allkotlin.requireNotNull()
) and similarkotlin.test.assertTrue
(replaced allkotlin.check()
,kotlin.require()
,kotlin.assert()
) and similarkotlin.test.BeforeTest
(replaced allorg.junit.jupiter.api.BeforeEach
) and similarkotlin.test.Ignore
(replaced allorg.junit.jupiter.api.Disabled
)assertk
base
,templating
and integration tests.Caveats:
kotlin-test-junit5
specificaly. I hope this will be addressed in Integration tests refactoring #2925org.junit.jupiter.params.ParameterizedTest
(along with related classes) - used to run the same test with various parametersorg.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 substitutionorg.junit.jupiter.api.RepeatedTest
- found no direct substitution