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

Better mechanism to define singleton containers #1441

Closed
aguibert opened this issue Apr 29, 2019 · 21 comments
Closed

Better mechanism to define singleton containers #1441

aguibert opened this issue Apr 29, 2019 · 21 comments

Comments

@aguibert
Copy link
Contributor

Currently singleton containers are kind of hacky, requiring manual container start in a static init block of an extended class.

Singleton containers are useful, because it allows container startup cost to be amortized across multiple test classes for a faster overall runtime.

The current approach has 2 primary limitations:

  1. requires a certain test class to be extended
  2. requires container lifecycle to be manually managed by the user

Proposed solution

Add a new SharedContainerConfiguration marker interface, with a complementary @SharedContainerConfig(Class<? extends SharedContainerConfiguration>) annotation which may be used on a test class. Shared container configuration would be processed in org.testcontainers.junit.jupiter.TestcontainersExtension.beforeAll(ExtensionContext), and would start any containers that are not already started.

Example usage:

public class MySharedContainerConfig implements SharedContainerConfiguration {
    @Container
    public static GenericContainer<?> foo = new GenericContainer<>();
}
@Testcontainers
@SharedContainerConfig(AppContainerConfig.class)
public class MyTestA {

   @Test 
   public void testA() { ... }
}

@Testcontainers
@SharedContainerConfig(AppContainerConfig.class)
public class MyTestB {

   @Test 
   public void testB() { ... }
}

The call flow would be as follows:

  1. GenericContainer foo is started
  2. TestA beforeClass (no-op in this case)
  3. testA runs
  4. TestB beforeClass (no-op in this case)
  5. testB runs
@aguibert
Copy link
Contributor Author

aguibert commented May 1, 2019

I'm willing to take a crack at implementing this feature, but before I spend the time implementing it I wanted to discuss the idea to make sure it would be acceptable if implemented.

@kiview
Copy link
Member

kiview commented May 2, 2019

Hi @aguibert,
thanks for the suggestion. I also think it's a good idea to discuss this beforehand.

In your example, how would I be able to access the container foo from inside the test?

@sdaschner
Copy link

Hi folks, I think that is a very much needed approach.

How about solving the need for accessing a container with adding the possibility to inject the @SharedContainerConfig into a test class, as well.

I.e. having a second approach, if the access is required:

@Testcontainers
public class MyTestA {

   @SharedContainerConfig
   private AppContainerConfig testScenario;

   @Test 
   public void testA() {
      testScenario.foo ...
   }
}

@kiview
Copy link
Member

kiview commented May 2, 2019

That's a nice idea, I think I like this more.

So whatever the actual implementation, we would still ultimately bind the life cycle to the JVM process, correct?

Now the last thing I'm wondering about, if this should be a test framework specific implementation, or if we want the API independently (probably good enough like we have it now with the static block).

@aguibert
Copy link
Contributor Author

aguibert commented May 2, 2019

In your example, how would I be able to access the container foo from inside the test?

Since the foo field in MySharedContainerConfig is static, and the lifecylce is wider than the scope of the test, the test can simply access the public static field using normal Java static field access:

@Testcontainers
@SharedContainerConfig(AppContainerConfig.class)
public class MyTestA {

   @Test 
   public void testA() {
     int port = AppContainerConfig.app.getFirstMappedPort();
     // etc...
  }
}

The limitation here is that pretty much everything in the shared config class ought to be static. Conceptually, statics make more sense to me when we consider the lifecycle is JVM-wide. I think the advantage to what @sdaschner suggested is that we get an instance of the object, and can access instance fields. However, I don't think instance fields should be necessary for a JVM-wide shared config?

So whatever the actual implementation, we would still ultimately bind the life cycle to the JVM process, correct?

Correct

Now the last thing I'm wondering about, if this should be a test framework specific implementation, or if we want the API independently (probably good enough like we have it now with the static block).

This is really only doable with JUnit Jupiter afaik. If people are still on JUnit 4 we could or something else, they can use the current approach with the static block.

@sdaschner
Copy link

sdaschner commented May 2, 2019

@aguibert Oh, only now I realized that your field was actually static. I'd actually rather avoid a static field and make the "singletonness" part of the testcontainers definition. Especially, if you're in a more complex test scenario you'd need to access the container config from other components and making that explicit (rather than accessing the public static field from somewhere) is cleaner, IMO.

So, I'd go for a non-static @Container field definition approach, very similar to what you would normally define in a test class.

This is really only doable with JUnit Jupiter afaik. If people are still on JUnit 4 we could or something else, they can use the current approach with the static block.

I'm 99% sure the same is possible in JUnit 4, at least with a custom runner and reflective lookup. But yes, as a last resort, one can always fallback to the static block & abstract class.

@bsideup
Copy link
Member

bsideup commented May 2, 2019

Doesn't this give the same result, framework agnostic?

public enum MyContainers {
    INSTANCE;

    public KafkaContainer kafka = new KafkaContainer();

    public GenericContainer whatever = ...;

    public MyContainers() {
        Stream.of(kafka, whatever).parallel().start();
    }
}

// ...
new KafkaClient(MyContainers.INSTANCE.kafka.getBootstrapServers());

@aguibert
Copy link
Contributor Author

aguibert commented May 2, 2019

So, I'd go for a non-static @container field definition approach, very similar to what you would normally define in a test class.

Having a non-static @Container field already has meaning for Testcontianers though -- it means that the container will be restarted on every single @Test method. If you define your @Container field as static, then it lives for the entire duration of the class. I suppose we could always re-define the meaning of static vs. non-static in the context of this new marker interface, but I think we should have a concrete benefit of doing so before we make it different.

I'm 99% sure the same is possible in JUnit 4, at least with a custom runner and reflective lookup. But yes, as a last resort, one can always fallback to the static block & abstract class.

It would be possible with a custom JUnit 4 runner, but I don't consider that an option because a test class can only define 1 custom runner at most. I don't think this mechanism is valuable enough for it to be the one customer runner that people choose.


@bsideup that's an interesting idea. Can you provide a bit more context though? Do the containers in MyContainers get initialized/started on static init of the test class that's using them? Or lazily on first-access of MyContainers.INSTANCE?

@bsideup
Copy link
Member

bsideup commented May 2, 2019

@aguibert according to the spec, the enum gets initialized on the first access.
Enum's semantics guarantee the singleton behaviour, and it can be used from anywhere in the tests :)

@sdaschner
Copy link

@bsideup That approach certainly works, as well as the one that is currently documented (singleton containers), however, I'd rather have another, cleaner integration into the tool. TBH, implementing singletons using enums is also not the cleanest solution :-) WDYT?

Also, we might have multiple SharedContainerConfigs, that are not globally shared, but only where they are defined and being used.

@sdaschner
Copy link

@aguibert I see, yes, I actually thought about defining the semantics of having a method annotated with @Container to be singleton, or rather, shared, once it's defined in that marker interface, just thinking of "cleaner" interfaces only consisting of non-static methods... But that's probably an instance of bike shedding, I guess :-)

@kiview
Copy link
Member

kiview commented May 6, 2019

TBH, implementing singletons using enums is also not the cleanest solution :-) WDYT?

Why do you think it's not clean? I think for the JVM life cycle use case, this would be elegant, since we don't need any framework integration code.

Also, we might have multiple SharedContainerConfigs, that are not globally shared, but only where they are defined and being used.

One could use package-private Enum classes, depending on the organization of the test classes.

I think if we argue about something like SharedContainerConfig, it would be more useful to think about Jupiter specific things that could add unique value, e.g. shared containers per test suite (once they will be implemented, see junit-team/junit5#744).

@aguibert
Copy link
Contributor Author

aguibert commented May 6, 2019

I don't mind using an enum for a singleton, but a few things I don't like about any vanilla singleton solution (enums or otherwise) is:

  1. Requires user to manually call start() in all cases. This can be super useful in some cases where container start ordering matters, but isn't always needed. Also, if users don't realize the parallel() trick they could take a big performance hit.
  2. The container configuration seems like an important class-level configuration aspect of the test class. Displaying this prominently on a class-level annotation seems like an appropriate way to advertise that information, as opposed to burying it in a test case.
  3. Requires direct access of the singleton at some point in the test. With current Testcontainers code this is fine, but I am working on a Testcontainers extension that abstracts away the need to access container objects (in most cases). I realize this use case is specific to my extension, so my backup option is to implement it in my extension instead. But I'd prefer to contribute it to the core Testcontainers project so others can benefit from it too.

One benefit of using an annotation is that we could bake common user options into it, such as:

@SharedContainerConfig(parallelStart = true|false, // default=true
                       restartPolicy = NEVER|PER_CLASS|PER_TEST) // default=NEVER

If we were to do this, it would indeed seem Jupiter specific. But is that really a concern? It seems that Jupiter is the future direction of automated java tests for sure.

@bsideup
Copy link
Member

bsideup commented May 7, 2019

Also, if users don't realize the parallel() trick they could take a big performance hit.

JUnit always start them sequentially, the parallel trick is an advantage of this approach

The container configuration seems like an important class-level configuration aspect of the test class. Displaying this prominently on a class-level annotation seems like an appropriate way to advertise that information, as opposed to burying it in a test case.

But if you're going to share the configuration (including the SharedContainerConfig), you're moving it to a separate class anyways

Requires direct access of the singleton at some point in the test.

On the other hard, I can say that this is an advantage too :) "I can access my containers from anywhere, just with Java code, without injecting anything, integrating with the test frameworks or knowing of some annotation" :)

@SharedContainerConfig(parallelStart = true|false, // default=true

We can't have it like this, some containers depend on another. At least until we introduce #1404

But is that really a concern? It seems that Jupiter is the future direction of automated java tests for sure.

While JUnit Platform is indeed popular, there are still many users of alternative testing frameworks, including: JUnit 4, Spock, ScalaTests or whatever they call it, TestNG, etc.
So I would say this is a concern for us.

@kiview
Copy link
Member

kiview commented May 7, 2019

@aguibert Just to get some context (and because I'm interested in it 🙂), on what kind of extension are you working on?

With current Testcontainers code this is fine, but I am working on a Testcontainers extension that abstracts away the need to access container objects (in most cases). I realize this use case is specific to my extension, so my backup option is to implement it in my extension instead.

@aguibert
Copy link
Contributor Author

aguibert commented May 7, 2019

JUnit always start them sequentially, the parallel trick is an advantage of this approach

By this approach do you mean the shared container config, or the singleton approach? Both could take advantage of parallel start. My point was that the shared container config could automatically do the parallel start, whereas with the enum/singleton approach the user needs to specify parallel.

But if you're going to share the configuration (including the SharedContainerConfig), you're moving it to a separate class anyways

Yes, both solutions move the container config to a separate class. What I was trying to get at here is that a class-level anno displays that information prominently to the developer.

Requires direct access of the singleton at some point in the test.

On the other hard, I can say that this is an advantage too :) "I can access my containers from anywhere, just with Java code, without injecting anything, integrating with the test frameworks or knowing of some annotation" :)

I think allowing direct access would be the ideal advantage. Requiring access when it is not needed seems restrictive.

@bsideup I do see your points though, and I think you've provides sufficient argument against this idea where I can close out this issue.


@aguibert Just to get some context (and because I'm interested in it 🙂), on what kind of extension are you working on?

The basic idea I'm pursuing is an extension that leverages testcontainers/docker for the application under test, as opposed to just using testcontainers/docker for external resources. In the JavaEE world, I think we can use Docker+startup checks to eliminate a lot of the tedious things about starting/configuring traditional JavaEE app servers. This essentially turns things into black-box testing, but with the help of REST client proxies (among other technologies) we can make this more of a grey-box test. A few weeks back I published a blog post with my initial concept: https://openliberty.io/blog/2019/03/27/integration-testing-with-testcontainers.html

@aguibert aguibert closed this as completed May 7, 2019
@bsideup
Copy link
Member

bsideup commented May 8, 2019

@aguibert I hope I was not too pushy ☺️
I'm not strongly against the idea, but I would always prefer to not have an integration (and annotations) where a language feature can be used instead.

You have a valid concern that not everyone is aware of the parallel trick, thanks!
I think it makes sense to add it to the docs, e.g. to the "Singleton container pattern" section 👍

Thanks for your proposal! It brought some good discussions and reminded us of a few things like #1404 :)

Feel free to reopen it if you discover more downsides of the enum/shared class/static block approach!

@aguibert
Copy link
Contributor Author

aguibert commented May 8, 2019

@bsideup not at all! I appreciate your willingness to discuss and entertain suggestions from random users like myself.

#1404 looks interesting. I've added a new comment/suggestion over there. Overall, I really like field-based declaration approach of containers, but the problems is that when you want to orchestrate the containers the current field-based approach breaks down and we have to do these other solutions that seem more like "workarounds" than proper solutions -- even though I can't come up with any technical reasons why they can't be considered "proper solutions" 😉. I guess it's because the rest of the Testcontainers developer experience is really clean, but this aspect sticks out as an "ugly" spot

@membersound
Copy link

membersound commented Feb 3, 2020

So as this issue is closed, what was the conclusion? Is there any way creating a shared container beside adding a static block inside an abstract class, and dropping @Testcontainers and @Container annotations by starting the container manually?

Why not simply introducing a @Testcontainers(shared = true) property?

@bsideup
Copy link
Member

bsideup commented Feb 3, 2020

Why not simply introducing

Because it is not simple :)

What's wrong with static + calling .start(), once again? BTW you don't need an abstract class, it's up to you how to share it

@membersound
Copy link

Nothing wrong, it could just be more elegant regarding the usual spring annotation-based config style. Nonetheless it works fine of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants