-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
JUnit 5 extension #887
JUnit 5 extension #887
Conversation
…apted from spock extension
Hey @britter, great that you've contributed the JUnit5 extension, I told you it's not too much work 😉 I'll do the review later this weekend 🙂 |
Hi @britter! Thanks for contributing your vision of 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.
Since I was @'ed, I figured I'd give a quick review. I know little about Testcontainers (except that once they have proper JUnit 5 integration and no longer rely on JUnit 4, I will start using them immediately) and so focused on the Jupiter extension. Straightforward and well implemented. 👍
* {@code @Testcontainers} is a JUnit Jupiter extension to activate automatic | ||
* startup and stop of test containers used in a test case. | ||
* | ||
* <p>The test containers extension finds all fields of typ |
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.
Typo: "typ"
class TestcontainersExtension implements BeforeAllCallback, BeforeEachCallback, AfterAllCallback, AfterEachCallback { | ||
|
||
@Override | ||
public void beforeAll(final ExtensionContext context) throws IllegalAccessException { |
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.
You could reduce code duplication between beforeAll
and afterAll
by having a method forEachSharedContainer(Consumer<Field>)
that does the same thing as this method, but instead of startContainer
calls Consumer::consume
with the field.
} | ||
|
||
@Override | ||
public void beforeEach(final ExtensionContext context) throws IllegalAccessException { |
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.
Same as for beforeAll
.
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
||
@Testcontainers | ||
class ComposeContainerIT { |
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 don't know how Testcontainers in general or these new tests in particular are structured, so my point may be moot, but here it goes: I moved away from using test class names to distinguish unit and integration test. Instead I apply Jupiter tags (e.g. with @Tag("integration")
) and configure the build tool to filter by tags instead of file names. If you're interested in that, let me know - I have more than these two cents to give. 😉
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.
We build Testcontainers using Gradle and currently don't have dedicated source sets for Unit- and Integration-Tests. I would assume if we would retstructure our testing phases, we would use two distinct source sets for Unit- and Integration-Tests, so we can use it for all modules.
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.
Alternatively, you could use tags and define two separate test
tasks.
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 would avoid IT
suffix (a rudiment from Maven)
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 actually just copied over from the Spock tests 🙂
@britter thanks for raising this! I haven't had a chance to review/play with this yet, but re the Codacy warnings: yes, please consider these false positives. There's nothing there that would worry me for test-scoped code. |
|
||
/** | ||
* {@code @Testcontainers} is a JUnit Jupiter extension to activate automatic | ||
* startup and stop of test containers used in a test case. |
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 would omit the 'test' from 'test container' here.
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
||
/** | ||
* This test verifies, that setup and cleanup of containers works correctly. |
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 wonder right know, if this comment is still true for the existing Spock tests structure. Either way, this is kind of hackish and I think we should refactor this test in the Spock module and don't want to duplicate it for this new module :D
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 @bsideup is referring to the inheritance and abstract class tests:
https://github.com/testcontainers/testcontainers-java/pull/636/files#diff-d6b833ecb1659337d0093eab00102635
I'm very happy with the PR, since the integration surface is super slim and the API is the same than our Spock implementation.
One thing to keep in mind is, that VncRecording will not work right now. But we can add the support in a later PR.
I'm doing another iteration. @nicolaiparlog has pointed me to the I'll also adapt the inheritance test case making sure that this use case is also supported! |
After another iteration I came up with a solution that is based on Solution 1 uses static fields to enable shared containers. This way shared containers are bound the the test class. The problem with this approach is, that shared containers can't be used inside nested test cases, because nested test cases must not be declared as static classes and can therefore not have static fields. Solution 2 uses the I currently don't know which solution we should implement... So I'm happy for every feedback. |
I think nested tests is a super convenient and powerful feature, however I'm not sure how many users will actually use them or know about this feature? Is it very prominent in JUnit5? I have no real problem with **Solution 2 ** if we document it accordingly and would prefer it in order to support nested tests. |
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.
Regarding solution 2: There's no way around the instance variable being instantiated twice. You could store the first one in the extension context of the nested test class and replace it using a TestInstancePostProcessor
. However, I would consider that a hack. 😉
Thus, I'm in favor of using static fields and adding documentation that those won't work in nested classes.
* | ||
* <p>The test containers extension finds all fields of typ | ||
* {@link org.testcontainers.lifecycle.Startable} and calls their container | ||
* lifecylce methods. Containers declared as static fields will be shared |
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.
Typo: "lifecylce"
@Override | ||
public void beforeAll(final ExtensionContext context) throws IllegalAccessException { | ||
Class<?> testClass = context.getRequiredTestClass(); | ||
for (final Field field : testClass.getDeclaredFields()) { |
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.
This should take into account fields declared in super classes, shouldn't it? Same for the other methods in this class.
@Override | ||
public void afterEach(final ExtensionContext context) throws IllegalAccessException { | ||
Object testInstance = context.getRequiredTestInstance(); | ||
for (Field field : testInstance.getClass().getDeclaredFields()) { |
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.
Alternatively, you could store the started containers in the ExtensionContext
and retrieve them from there. You could even store CloseableResource
implementations and let Jupiter take care of stopping the containers.
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
|
||
@Testcontainers | ||
class ComposeContainerIT { |
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.
Alternatively, you could use tags and define two separate test
tasks.
This looks amazing - thank you all for contributing and refining this! I only had one minor comment on the docs; this is already a well polished PR by the time I've got to it 😄. I'd like to have a little bit more of a play with this tonight, but I'll be looking forward to us merging and releasing this ASAP. Thank you 🙇 |
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
||
@Testcontainers | ||
class TestcontainersNestedRestaredContainerIT { |
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.
Tiny typo!
class TestcontainersNestedRestaredContainerIT { | |
class TestcontainersNestedRestartedContainerIT { |
Co-Authored-By: britter <beneritter@gmail.com>
store.put(TEST_INSTANCE, testInstance); | ||
|
||
findSharedContainers(testInstance) | ||
.forEach(adapter -> store.getOrComputeIfAbsent(adapter.key, k -> adapter.start())); |
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.
Any reason to use field instead of getter?
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 StoreAdapter
class is a private class inside of TestcontainersExtension
. So all fields are visible from TestcontainersExtension
and there is no gain from an information hiding perspective if we add getters.
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 problem I see is that it is inconsistent with the rest of our code base because we always use getters :)
} | ||
|
||
private static Predicate<Field> isContainer() { | ||
return field -> Startable.class.isAssignableFrom(field.getType()) && AnnotationSupport.isAnnotated(field, Container.class); |
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 would return any field annotated with @Container
and throw and error if it is not Startable
, otherwise it will simply ignore it
I know, this PR is in a late and almost finished state -- but I promised @britter to spike a different way to handle shared containers, which is also thread safe. See https://github.com/junit-team/junit5-samples/compare/singleton-extension for a work-in-progress implementation using a Transferred to this container context, the test class could read like: @ExtendWith(SingletonExtension.class)
class SingletonExtensionTests {
@Test
void test1(@Singleton(MySQL123.class) MySQLContainer shared) {}
@Test
void test2(@Singleton(MySQL123.class) MySQLContainer shared) {}
@Test
void test3(@New(MySQL123.class) MySQLContainer fresh) {}
} The public class MySQL123 implements SingletonExtension.Resource<MySQLContainer> {
private final MySQLContainer container = new MySQLContainer();
@Override
public MySQLContainer get() {
return container;
}
} Edit: refactored the implementation and filed junit-team/junit5-samples#87 as a sample for discussion |
* Small improvements to Jupiter extension code style Follow up to #887 * Only raise exception if annotated field is not Startable
I am trying to use this extension together with From what I see in the JUnit 5 documentation the annotation order matters. Since Java sorts annotations in bytecode alphabetically Spring will always run first unless it is possible to explicitly import Question is: How to ensure testcontainers ordering relative to other extensions with the current extension mechanism? |
Hi @jrehwaldt! |
Thanks. That's exactly what I am currently doing. I got pointed to this new feature and thought I'd give it a shot. Imho, this extension provides test lifecycle support out of the box without copy and pasting start-stop logic in Am I right in assuming the scenario I describe is simply not supported as of now? I believe making |
@jrehwaldt Since this PR is merged, please open a new issue. |
I did in #1017. Thank you. |
This is related to #636 but takes a different approach: instead of upgrading testcontainers core to JUnit 5 APIs, this introduces a new module containing a JUnit 5 extension that uses testcontainers core.
Static fields are shared among test methods (started only once and stopped at the end), while instance fields are started and stopped for every method. This approach has the advantage that it is relatively easy to implement. However, the downside of this is, that shared containers can't be used inside of
@Nested
test cases since these must not be declared as static inner classes and hence can not have static fields.Feedback welcome @kiview, @sormuras, @nicolaiparlog, @marcphilipp