Skip to content
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

Make TestcontainersExtension public #5285

Merged
merged 2 commits into from
Aug 16, 2022
Merged

Conversation

hmatt1
Copy link
Contributor

@hmatt1 hmatt1 commented Apr 26, 2022

Proposed fix for #2045

When we use multiple JUnit5 extensions, the way to order them is via the @ExtendsWith({FirstExtension.class, SecondExtension.class}) annotation.
But TestcontainersExtension is not public so we can only use the @testcontainers annotion and we are not able to order the extension.

See the following documentation on JUnit5: https://junit.org/junit5/docs/current/user-guide/#extensions-registration-declarative

Proposed fix for testcontainers#2045

When we use multiple JUnit5 extensions, the way to order them is via the @ExtendsWith({FirstExtension.class, SecondExtension.class}) annotation.
But TestcontainersExtension is not public so we can only use the @testcontainers annotion and we are not able to order the extension.

See the following documentation on JUnit5: https://junit.org/junit5/docs/current/user-guide/#extensions-registration-declarative
@hmatt1 hmatt1 requested a review from a team as a code owner July 5, 2022 05:37
@kiview
Copy link
Member

kiview commented Jul 6, 2022

Hi @hmatt1, sorry for not reacting sooner. Can you give me an example of a list of extensions, where the order with regards to the TestcontainersExtension matters?

Looking around different 3rd party Jupiter extensions, (like junit-pioneer), I get the feeling that having the Extension class package-private is the idiomatic way to do things.

@hmatt1
Copy link
Contributor Author

hmatt1 commented Jul 6, 2022

@kiview It is mentioned in the issue that one user wanted to do some registry authentication config: #2045 (comment)

The issue also mentions

Some microservices frameworks have extension to start the application for integration testing, whe using test containers, we need to be sure that the testcontainers extension run first to start the containers before the apps starts.

I'm not familiar with the specific extensions the users are referring to. However, in the docs I linked to, they provide this example of defining a custom composed annotation that combines multiple extensions in a reusable way.

@Target({ ElementType.TYPE, ElementType.METHOD })
@Retention(RetentionPolicy.RUNTIME)
@ExtendWith({ DatabaseExtension.class, WebServerExtension.class })
public @interface DatabaseAndWebServerExtension {
}

This is another use case that isn't supported when the TestcontainersExtension class is package-private. This is something I wanted to take advantage of in my project, since we have a few extensions we are using across our tests and would like to define them in one place.

@kiview
Copy link
Member

kiview commented Jul 6, 2022

Thanks for providing more info @hmatt1. I was talking with @sormuras from the JUnit team and he generally recommended to not go for public as the default visibility modifier and that extensions should strive to be independent.

I can imagine that there might be concrete cases where this ordering is the only way to make it work, but I wanted to know the concrete case, to understand if there might be a better approach (similar to what e.g. Spring provides with DynamicPropertySource).

@hmatt1
Copy link
Contributor Author

hmatt1 commented Jul 7, 2022

@kiview Is the use case around creating a composed annotation that includes the TestContainers extension still valid? My understanding is that it is not possible if the extension isn't public.

@kiview kiview added this to the next milestone Aug 16, 2022
@kiview kiview merged commit 9275bc5 into testcontainers:master Aug 16, 2022
@kiview
Copy link
Member

kiview commented Aug 16, 2022

@hmatt1 Thanks for providing the PR, after some consideration and thanks to the feedback of the community, we decided to make the extension public 🙂

@hmatt1 hmatt1 deleted the patch-1 branch August 16, 2022 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants