-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Create junit-vintage module #10351
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
Create junit-vintage module #10351
Conversation
FYI: @vlsi and @marcphilipp |
8264c3b
to
3fb74aa
Compare
} | ||
|
||
return ReflectionSupport | ||
.findFields(description.getTestClass(), isTargetedContainer, HierarchyTraversalMode.TOP_DOWN) |
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 what happens if class Subclass extends AbstractClass
and both of them declare @Rule Testcontainers
. It might cause for the containers to discover and initialize twice.
I guess each @Rule
might "manage" own the class it is located (e.g. @Rule
in Subclass
should start/stop only the direct containers in Subclass
, and the rule in AbstractClass
should manage the containers in AbstractClass
plus all its superclasses unless they have one more @Rule Testcontainers
).
An alternative option would be to just fail in such scenarios so the user removes one of the @Rule Testcontainers
.
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 what happens if
class Subclass extends AbstractClass
and both of them declare@Rule Testcontainers
. It might cause for the containers to discover and initialize twice.
Then don't do that 😊
Having large test class hierarchies is an anti-pattern. Just like sharing code via delegation is preferred over sharing code via inheritance in non-test code, using delegation (like Rules) is preferable to large test class hierarchies.
I guess each
@Rule
might "manage" own the class it is located (e.g.@Rule
inSubclass
should start/stop only the direct containers inSubclass
, and the rule inAbstractClass
should manage the containers inAbstractClass
plus all its superclasses unless they have one more@Rule Testcontainers
).
The behavior of this rule mirrors that of the JUnit Jupiter extension, so I'd prefer to keep it as-is to ease migration.
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.
Having large test class hierarchies is an anti-pattern
Testcontainers-java include a lot of test that inherit each other, and you can't prevent everyone from writing such tests.
The behavior of this rule mirrors that of the JUnit Jupiter extension, so I'd prefer to keep it as-is to ease migration
@kcooney , could you please double-check?
As far as I read JUnit5 documentation, I believe JUnit5 does not result in double-start and double-stop of the containers in case users apply @Testcontainers
extension for both subclass and its superclass.
https://junit.org/junit5/docs/current/user-guide/#extensions-registration-inheritance
A specific extension implementation can only be registered once for a given extension context and its parent contexts. Consequently, any attempt to register a duplicate extension implementation will be ignored.
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.
Having large test class hierarchies is an anti-pattern
Testcontainers-java include a lot of test that inherit each other, and you can't prevent everyone from writing such tests.
People who write such tests should understand that they inherit the behavior of Rules defined in the base class.
The behavior of this rule mirrors that of the JUnit Jupiter extension, so I'd prefer to keep it as-is to ease migration
@kcooney , could you please double-check?
I literally copied the Container discovery code from the Jupiter extension code.
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 literally copied the Container discovery code from the Jupiter extension code.
The difference is that JUnit4 would happily execute both rules (one from a base class, another one from a subclass), so it would result in double start and double stop.
JUnit5 would execute extension only once since it registers the extension class only once even if it is declared for both base class and its subclass.
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 literally copied the Container discovery code from the Jupiter extension code.
The difference is that JUnit4 would happily execute both rules (one from a base class, another one from a subclass), so it would result in double start and double stop. JUnit5 would execute extension only once since it registers the extension class only once even if it is declared for both base class and its subclass.
Again, the perfect is the enemy if the good. There is only so much we can do due to the limitations of JUnit4.
People should not use this rule in a class if a base class has the same rule.
The alternative would be requiring that the rule being redefined in every subclass (more boilerplate), and users would need to avoid shadowing the rules defined in the base class.
FWIW, at Google we had internal rules that worked his way (definitions in the base class affecting behavior of annotated classes in subclasses) and I never heard anyone have a problem due to accidentally defining the rule multiple times in the same class hierarchy.
Let's see what the maintainers say.
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.
Again, the perfect is the enemy if the good. There is only so much we can do due to the limitations of JUnit4.
I'm sorry, have you seen a couple of suggested fixes for the problem I listed in the original message?
Approach A: fail the test if the rule is present more than once in the test hierarchy. A straightforward error message would convey the idea that the users should not use the rule multiple times in a single hierarchy.
Approach B: make sure only the first rule in hierarchy wins so the rest becomes a no-op.
In other words, each rule could enumerate all TestcontainerRule
objects within the hierarchy, and execute only in those cases, if the current rule is the first (e.g. it has the minimal hierarchy level, and it is the first based on the field name). The implementation would be trivial, and it would effectively match the behaviour from JUnit5 which registers an extension only once.
Frankly, I do not understand why do you say about "limitations of JUnit4". Both "approach A" and "approach b" are straightforward, they would make the migration easier, I can help with the implementation, and so on.
I assume it would look awkward if I suggest a PR on top of your branch.
modules/junit-vintage/src/main/java/org/testcontainers/junit/vintage/TemporaryNetwork.java
Show resolved
Hide resolved
f2d9987
to
ae0f95f
Compare
b675a38
to
c1081ff
Compare
af9afa5
to
77d45a9
Compare
26171e4
to
d45f926
Compare
d45f926
to
8c66d21
Compare
8c66d21
to
35e5731
Compare
Hi @kcooney, as I previously mention in #10285 (comment) and this demonstrated it again. We will be removing JUnit 4 dependency with no support for it. |
This adds a a module for supporting junit4-style tests. The module contains
two rules:
Testcontainers
- For managing containersTemporaryNetwork
- For managing a networkThese provide a migration path for users of Testcontainers that have
JUnit4-style tests, so that the Testcontainers project can later remove
dependencies on JUnit4 classes in the core Testcontainers code (see #970).
This PR is a work in progress, intended as a starting point for discussions.
Remaining work:
TemporaryNetwork
Testcontainers
to verify behavior of the rule when usedwith
@ClassRule
./.github/ISSUE_TEMPLATE/bug_report.yaml
./.github/ISSUE_TEMPLATE/enhancement.yaml
./.github/ISSUE_TEMPLATE/feature.yaml
./.github/dependabot.yml
./.github/labeler.yml
./docs/modules/