-
Notifications
You must be signed in to change notification settings - Fork 38
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
Extend support for JUnit 5 via the new roborazzi-junit5 artifact #355
base: main
Are you sure you want to change the base?
Extend support for JUnit 5 via the new roborazzi-junit5 artifact #355
Conversation
a3245f5
to
a0ccf40
Compare
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.
Thanks for your contribution.
Overall, I want to introduce these changes, and I was able to understand them from the pull request description. I do have a few questions, but they are minor.
Could you take a look at the CI failure?
@@ -0,0 +1 @@ | |||
com.github.takahirom.roborazzi.junit5.RoborazziExtension |
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.
Question: Will the RoborazziExtension be automatically installed in all users' projects by this file for every test? Can users disable it?
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.
Yes, but only if they enable the automatic detection of extensions, either via Option B or C listed above. If they don't do either, then the extension would not be installed and they would have to use @ExtendWith(RoborazziExtension::class)
for every test
* from different class loaders, bridging the gap between test definition and their execution. | ||
*/ | ||
internal object CurrentTestInfo { | ||
private val concurrentRef = ThreadLocal<TestInfo>() |
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 wasn't aware that we needed to use ThreadLocal. I'm a bit concerned that we are currently using object for RoborazziContext, and if we switch to ThreadLocal, it might break some tests that use a different thread. I think this issue isn't directly related to this PR, but we should consider it later.
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.
It's a bit of a premature optimization at this point, because we haven't figured out parallel test execution for Robolectric yet (and I suppose that limitation extends to Roborazzi). But in the future, it's theoretically possible for multiple tests to run concurrently on different threads, and every test should be able to store its own TestInfo
here
} | ||
|
||
override fun afterEach(context: ExtensionContext) { | ||
val isConcurrent = requireNotNull(context.executionMode) == ExecutionMode.CONCURRENT |
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.
It may be challenging, but could we implement tests for ExecutionMode.CONCURRENT? 👀
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.
Yeah it should work, let me think about this!
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 added a concurrent unit test battery for CurrentTestInfo
in 4f07c4b
@RepeatedTest(3) | ||
fun repeatedTest(info: RepetitionInfo) { | ||
when (info.currentRepetition) { | ||
1 -> runTest("repeatedTest.png") |
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.
👍
Thank you for the swift review! 🙇♂️ I will address the CI failure shortly and consider extending the unit tests. |
0fd2f85
to
0d8c86e
Compare
@takahirom お待たせしました。We should be good to go with another run of the workflows. I reinstated the basic JUnit 5 detection of the stack trace strategy in the latest commit 0d8c86e, since apparently Kotlin Test defaults to using JUnit 5 behind the scenes for Compose Desktop and Multiplatform environments in some cases. With that, I was able to run all tests locally fine, including the ones in the sample projects. 🤞 |
Looks great. I should have mentioned this earlier, but I would like to add some tests and documentation.
|
Absolutely, sounds like a solid plan. I'll need a bit of time for this, but I'll let you know once done. |
@takahirom I'm thinking about the |
@mannodermaus |
Seems like we have a compatibility issue because the Robolectric JUnit 5 extension is built against Java 17, while Roborazzi uses the more conversative Java 11. 🤔 AGP has had 17 as the minimum version since 8.0, would this be something you'd be willing to raise? Roborazzi could keep its targetCompatibility at 11, but use a higher-level toolchain for compilation. |
…rallel execution Fire 100 tests at the same time and check that all references are set correctly
Turns out that kotlin.test defaults to JUnit 5 in certain applications (e.g. Compose Desktop or Multiplatform) and the existing strategy does pick up its method names correctly. Add a comment explaining the context
This requires the introduction of the third-party Robolectric extension and the Gradle plugin for JUnit 5 with Android.
828ee19
to
0015851
Compare
@takahirom I wrote a doc section on JUnit 5 in the latest commit (first time using Writerside; pretty neat!) and also updated the Robolectric extension to the latest version. It's now targeting Java 11, so there shouldn't be any compatibility issues - let's just hope that the integration tests finally work. |
@@ -0,0 +1,87 @@ | |||
# JUnit 5 support |
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 think this is easy-to-understand documentation! Thanks!
This PR introduces
roborazzi-junit5
, a newly proposed artifact for extending the capabilities of this fantastic library with the newest version of JUnit. It refactors how Roborazzi's file name generation works and adds a second detection algorithm specifically for tests using the JUnit Jupiter API from JUnit 5.Background
Historically, Robolectric-based tests have not been supported by JUnit 5 because of incompatibilities with the way it injects custom class loading into the execution environment. However, very recently there have been some fantastic achievements in this space, finally allowing JUnit 5 to deal with Robolectric tests via a third-party extension. The hope is to integrate this extension into the mainline Robolectric library in the future. With this extension in place, it's already possible to use the current Roborazzi version for ordinary test cases, but only if you specify the file path manually for every usage of
captureRoboImage
.Motivation
The default behavior of
captureRoboImage
delegates the final file name of the captured image to theDefaultFileNameGenerator
. This class can infer the file name by looking at the stacktrace of the Robolectric thread and finding the element annotated with@org.junit.Test
. There is also some basic detection for the JUnit 5 annotation (@org.junit.jupiter.api.Test
), but the check doesn't cover all of the possible cases and fails when used together with the extension I mentioned earlier. Consider the following test class as an example:Approach
This PR refactors the stacktrace detection code and makes it an implementation of the new
TestNameExtractionStrategy
interface inside ofroborazzi-core
. By default, Roborazzi will always use this implementation for generating file names, just like before. Additionally, it can detect ifroborazzi-junit5
is on the classpath, and if this is the case, it also adds the newJUnit5TestNameExtractionStrategy
to itself. You see, the issue with JUnit 5 stacktraces is that they don't always contain the exact test method in them, so you cannot find them by their annotation. Furthermore, there are a whole bunch of annotations that generate tests at runtime, and those cases don't have any annotations in the trace and therefore cannot be detected with the current way:@ParameterizedTest
@RepeatedTest
@TestFactory
@TestTemplate
The extraction strategy for JUnit 5 that I added to
roborazzi-junit5
is tailored to detecting the names of these tests. It reads the currently executed test method from a shared storage calledCurrentTestInfo
, which is updated from the other side by a JUnit 5 extension that keeps the storage up-to-date at all times. Since both sides are attached to different class loaders, there is a fair bit of ugly trickery involved to send this information across their boundaries. 😵 Essentially, the extraction strategy has to look up the sharedCurrentTestInfo
via reflection, otherwise it cannot see the other class loader's static data. I'm using lazy references as much as possible to minimize the runtime performance impact of this.The following diagram illustrates how the classes in the new artifact work together:
Setup
With this new artifact, consumers can use Roborazzi in their Robolectric tests with JUnit 5. The proposed setup would look like this:
Step 1: Add the new dependency next to the main Roborazzi library
dependencies { testImplementation("org.junit.jupiter:junit-jupiter-api:x.xx.x") testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:x.xx.x") testImplementation("io.github.takahirom.roborazzi:roborazzi:x.xx.x") + testImplementation("io.github.takahirom.roborazzi:roborazzi-junit5:x.xx.x") }
Step 2: Enable Roborazzi's JUnit 5 extension
Any of the following options will work, and only one of them needs to be followed. I personally prefer option C.
Option A: Add the Roborazzi extension to each test class
Option B: Enable autodetection of extensions globally via properties file
// in src/test/junit-platform.properties + junit.jupiter.extensions.autodetection.enabled=true (no change to MyJUnit5RoborazziTest necessary)
Option C: Enable autodetection of extensions via android-junit5's Gradle DSL
Conclusion
Apologies for dropping this wall of text unprompted! Please let me know your thoughts. 🙇♂️ This PR may be incomplete, as I have not yet looked into the publishing part of this new artifact. If there are any other files that need updating, let me know.