-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: move junit4 integration to org.testcontainers:junit4 module #10285
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
Conversation
|
Could anybody please approve the workflows so we could understand the scope of the failures? |
|
I've added I'm not sure regarding I'm not sure regarding |
8a67ea8 to
b909dd3
Compare
kcooney
left a comment
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.
Note to maintainers: this is in alignment with what the JUnit team suggested in junit-team/junit4#1720
| @Rule | ||
| public GenericContainer redis = new GenericContainer(DockerImageName.parse("redis:6-alpine")) | ||
| .withExposedPorts(6379); | ||
| public TestcontainersRule<GenericContainer<?>> redis = new TestcontainersRule<>( |
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 suggest naming this field redisRule (or, possibly, redisContainerRule).
This test is apparently used in the documentation, and having a more intention-revealing name could reduce confusion.
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.
Previously the fields were not named "...rule", and this is a "best effort to keep running with outdated junit4", so I would really prefer to refrain from renaming lots of fields.
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.
Preciously the fields had a different type.
I'm only suggesting updating this one file, since it is referenced by the documentation.
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'm only suggesting updating this one file, since it is referenced by the documentation.
For reference, there are a lot of test files displayed in the documentation: many modules have documentation and they refer to the test code (see https://java.testcontainers.org/modules/consul/ , https://java.testcontainers.org/modules/mockserver/ )
|
Thank you for working on this, @vlsi I am not a maintainer or a user of Testcontainers (I used to be a maintainer of JUnit4, which is how I discovered this PR) so take this with a grain of salt. I suggest breaking this PR into two. The first PR would include:
The above would be in a bug-fix release of Reasoning: Projects that are still using JUnit4 tend to be a) large codebases with shared components/infra tied to JUnit4, b) owned by teams with limited time for maintenance, or c) both. Often upgrading a library is only done when there is a critical need (ex: upgrading library x requires updating library y). In these cases, it's much easier if libraries introduce a replacement API in a separate release from one that removes existing functionality. |
| /** | ||
| * Integrates Testcontainers with JUnit4 lifecycle. | ||
| */ | ||
| public class TestcontainersRule<T extends AutoCloseable> extends ExternalResource { |
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 recommend making this class as final (see Effective Java 3rd Edition Item 19).
|
@kcooney , thank you for the review, and I mostly agree with it, yet I would like to hear from project maintainers first as I do not like spending time and waiting for 4+years to know the PR gets rejected. I agree it might be worth going with deprecation cycle, however, the deprecation does not help much if the deprecated elements got dropped in the subsequent release. At the same time, people could add something like In the best case, JUnit could lift the rule field validation, so testcotainers could add The request of dropping junit4 from testcotainers has been missing an answer for more than 6-7 years, so I would prefer to have at least something merged rather than having 10 PRs dying in vain. |
717b6dd to
00aa777
Compare
Previously, testcontainers declared an explicit dependency on JUnit4, and this change decouples Testcontainers from JUnit4. Note: JUnit4 is in maintenance mode, so it does not sound right to force such a dependency. The users who still need to use JUnit4 could use the following: * Add org.testcontainers:junit4 dependency * Add TestcontainersRule as follows: @rule TestcontainersRule<GenericContainer> = new TestcontainersRule(...) Fixes testcontainers#970
Waiting to hear from the Testcontainers project maintainers before making any large changes is perfectly reasonable. The comment me that you are replying to was targeting the maintainers; sorry if that wasn't clear.
Sure it does. Projects that use Testcontainers with JUnit4 coud first upgrade to the release that contains the new Rule, then incrementally update their tests to use the Rule. They could then upgrade to the next major/minor release of Testcontainers once their tests pass with the new dependency. Not all organizations can easily update their entire codebase in on commit.
The timeframe between deprecation and removal is, of course, up to the Testcontainers mainteners. I personally wouldn't suggest waiting five years in this specific case Very short periods between deprecation and removal is certainly not ideal, but in its current state this PR would b a breaking change. Breaking changes without a smooth migration path are also not ideal.
People could do that (and I've had to do similar shenanigans to upgrade third-party libraries), but that approach would require projects to update all of their tests twice to avoid having to maintain their own version of Assuming the Testcontainers team avoids including breaking changes in bug fix releases, I'm guessing the changes to existing classes in this PR would be included in a major or minor release. That's why I suggested introducing the new Rule in a bug fix release. Let's see what the Testcontainers maintainers think.
I don't think it is helpful for us to rehash that discussion here. Interested parties can read the thread for issue 1720 in the JUnit4 repo.
In any case, as I said a few days ago, the approach you have in this PR is in alignment with the one suggested by the JUnit team four years ago. I believe it's a good solution to the problem, and I appreciate you working on it. Hopefully a Testcontainers maintainer will respond soon. |
| @ClassRule | ||
| public Db2Container db2 = new Db2Container() | ||
| .acceptLicense(); | ||
| public TestcontainersRule<Db2Container> db2 = new TestcontainersRule<>( |
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.
While you are touching this file, I suggest changing this field to be static (fields annotated with @ClassRule must be static; the code here would result in a test failure).
| The `@Rule` annotation tells JUnit to notify this field about various events in the test lifecycle. | ||
| In this case, our rule object is a Testcontainers `GenericContainer`, configured to use a specific Redis image from Docker Hub, and configured to expose a port. | ||
| Wrap the containers with `new TestContainersRule(...)` so the containers start and stop, according to the test lifecycle. | ||
| In this case, our rule object is not `static`, so the container will start and stop with every test. |
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.
| In this case, our rule object is not `static`, so the container will start and stop with every test. | |
| Since the `redis` field is annotated with `@Rule` the container will start and stop with every test. |
|
Hi @vlsi, thanks for your contribution. In the future, please make sure to share your idea about how to tackle some issues. We understand and appreciate the effort involved in making such changes, and we value your time as a contributor. We’ve been intentionally avoiding breaking changes, even regarding the JUnit 4 dependency. I’ve explored a few approaches myself in the past, and it’s becoming clear to me that we should plan to drop it in the coming months. |
|
@eddumelendez, great to hear from maintainers. Could you please clarify? Do you mean you plan to drop junit4 dependency without providing any workaround for the users? |
|
@eddumelendez perhaps you or one of the other maintainers can comment on #970 to let people know what your current thinking on the approach is? |
Fixes #970
The idea is to drop JUnit 4 classes and interfaces from the core
testcontainersmodule, and move the integration to:junit4module.It turns out only a couple of modules used
@Rulepreviously:coreconsulazureSee: