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

New API and DSL #78

Merged
merged 2 commits into from Nov 18, 2019

Conversation

@LMnet
Copy link
Contributor

LMnet commented Oct 19, 2019

This pull request is based on these experiments: #54

All changes in this pull request are backward compatible. Old tests and containers will continue to work without changes.

The main motivation was the problem with the mutable nature of the Container. I and many users of testcontainers-scala are facing the problem with this. A typical example: trying to get a mapped port from the unstarted container. The current API allows doing this.

Another problem is lazy stuff in the current API. There is no way to force user to use lazy.

So, I'm trying to create more typesafe API for the scala facade. Here are the main implementation points:

  1. ContainerDef — it's container definition. Its goal is to start a specific docker container with specific parameters.
  2. Container — this is the container itself. Instances of this interface can communicate with the started docker container and, for example, return mappedPort.
  3. To use containers in your test user should use withContainers method inside a test. This is one of the recommended ways from the scalatest library to provide fixtures inside the test body.

All other stuff in the pull request is based on the ideas above.

What exactly done:

  • Introduced ContainerDef.
  • Added new scalatest suite traits. These traits use new containers and ContainerDef. They have a withContainers method, which users should use to use containers in their tests. I'm not sure that the naming is perfect, but this is what I have now:
    1. TestContainerForAll — a single container will start before all tests and stop after all tests.
    2. TestContainerForEach — a single container will start before each test and stop after each test.
    3. TestContainersForAll — multiple containers will start before all tests and stop after all tests.
    4. TestContainersForEach — multiple containers will start before each test and stop after each test.
  • Adapt GenericContainer for the new API. I added a few more constructors to this, and also GenericContainer.Def.
  • Added Stoppable and Andable. This is mostly for the DSL with and. You can find examples in the readme.
  • Added ContainerDef for all containers in the library. Also, I added a few minor refactorings to containers.
  • Added tests for the new functionality.
  • Updated README file.
  • Bumped a version. Also, I noticed, that 0.33.0 doesn't have release notes. I added a TODO badge there.

Some minor things:

  • Renamed OTC... stuff to Java..., for example, OTCGenericContainer to JavaGenericContainer in the codebase. I remember that back in the days when I was just starting to work with the testcontainers-scala this prefix confused me for a few moments when I saw it the first time. Also, I saw at least 1 question about it in the slack channel. I believe that Java prefix is a lot more clear.
  • Marked TestContainerProxy as deprecated and internal. Currently, it contains only deprecated methods and used only for DockerComposeContainer as a common class between DockerComposeContainer and Container. The current class hierarchy in the testcontainers-java is different: the common class between DockerComposeContainer and Container is Startable. It actually makes sense. I think we need to move forward in this direction too.
@LMnet LMnet mentioned this pull request Oct 20, 2019
@LMnet LMnet force-pushed the LMnet:new-api branch 4 times, most recently from 6273d9a to 0a2750b Nov 2, 2019
@LMnet LMnet changed the title WIP: New API and DSL New API and DSL Nov 9, 2019
@LMnet

This comment has been minimized.

Copy link
Contributor Author

LMnet commented Nov 9, 2019

@dimafeng it's ready for the code review

@LMnet LMnet force-pushed the LMnet:new-api branch from 0a2750b to 9a915ed Nov 9, 2019
@dimafeng

This comment has been minimized.

Copy link
Collaborator

dimafeng commented Nov 9, 2019

@LMnet awesome! I'll start reviewing tomorrow

class MysqlSpec extends FlatSpec with TestContainerForAll {
// You need to override `containerDef` with needed container definition
override val containerDef = MySQLContainer.Def()

This comment has been minimized.

Copy link
@dimafeng

dimafeng Nov 11, 2019

Collaborator

I still believe that main container class should be its definition.

override val container = MySQLContainer()

to make it more intuitive

This comment has been minimized.

Copy link
@LMnet

LMnet Nov 12, 2019

Author Contributor

I played with this a bit and I didn't come up with something reasonable enough. Going this way will make it much harder to maintain backward compatibility with the current API. In this pull request, I managed to maintain backward compatibility relatively easy.

Anyway, this is experimental. I suggest to try it and wait for feedback.

This comment has been minimized.

Copy link
@dimafeng

dimafeng Nov 13, 2019

Collaborator

Makes sense. I think I need to play with the code on my local machine to get a better sense of the backward compatibility in this implementation.

Btw, do you know any good way to gather feedback from early adopters?

This comment has been minimized.

Copy link
@LMnet

LMnet Nov 13, 2019

Author Contributor

I definitely will promote these changes in the Russian speaking scala user group in the telegram. I know that some people there are using testcontainers.

Also, recently I spoke about testcontainers and this pull request in the scalalaz podcast. This episode is still not released though. We need to wait for it a few more days.

I hope it would be enough at the start.


If you want to use multiple containers in your test:
```scala
class ExampleSpec extends FlatSpec with TestContainersForAll {

This comment has been minimized.

Copy link
@dimafeng

dimafeng Nov 11, 2019

Collaborator

would it be possible to replace abstract type with a type parameter?

class ExampleSpec extends FlatSpec with TestContainersForAll[MySQLContainer and PostgreSQLContainer]

I think this way it would be less verbose

This comment has been minimized.

Copy link
@LMnet

LMnet Nov 12, 2019

Author Contributor

At first test traits had type parameters, as you suggest. But I changed it to the type member. There are 3 reasons:

  1. You can't refer to a type parameter inside your test body. This is important, because startContainers returns Containers type, and withContainers receives a function with type Containers => Unit.
  2. If you have a lot of containers it would looks weird in my opinion. I have a service with 6 or 7 containers in testcontainers tests, so this is the real situation.
  3. I found that type member usage is a bit more explicit. For example:
    class ExampleSpec extends FlatSpec with TestContainersForAll[MySQLContainer and PostgreSQLContainer] {
      override def startContainers(): Containers = {
        val container1 = MySQLContainer.Def().start ()
        val container2 = PostgreSQLContainer.Def().start ()
        container1 and container2
      }
      
      it should "test" in withContainers { case mysqlContainer and pgContainer =>
        assert(mysqlContainer.jdbcUrl.nonEmpty && pgContainer.jdbcUrl.nonEmpty)
      }
      
    }
    For me, it looks like MySQLContainer and PostgreSQLContainer are lost inside test definition. But here they are a lot more visible and explicit:
    class ExampleSpec extends FlatSpec with TestContainersForAll {
    
      override type Containers = MySQLContainer and PostgreSQLContainer
    
      override def startContainers(): Containers = {
        val container1 = MySQLContainer.Def().start ()
        val container2 = PostgreSQLContainer.Def().start ()
        container1 and container2
      }
      
      it should "test" in withContainers { case mysqlContainer and pgContainer =>
        assert(mysqlContainer.jdbcUrl.nonEmpty && pgContainer.jdbcUrl.nonEmpty)
      }
      
    }
// After that, you need to describe, how you want to start them,
// In this method you can use any intermediate logic.
// You can pass parameters between containers, for example.
override def startContainers(): Containers = {

This comment has been minimized.

Copy link
@dimafeng

dimafeng Nov 11, 2019

Collaborator
  1. is it possible to start the containers under the hood? The order of start can be inherited from the order of returned definitions
  2. is it possible to add default behavior for cases with no additional configuration (like this one in the doc)?

This comment has been minimized.

Copy link
@LMnet

LMnet Nov 12, 2019

Author Contributor

I tried to find a way to do this and I came to the conclusion that there is no need for this default behavior. First of all, you need to pass parameters inside the container's definitions. So, anyway, you need to call ContainerDef constructor. Where will you do this? Even if you do this inside a test body (like in the classic API) you will need the same amount of code.

Let's think about it. Imagine, that we have some default way to start containers in a defined order. I can imagine API like that:

class ExampleSpec extends FlatSpec with TestContainersForAll {

  val containers = MySQLContainer.Def(paramsHere) and PostgreSQLContainer.Def(paramsHere)
  
  // Tests here
}

And it looks exactly like the current API! With all its advantages and disadvantages. The main problem with it — how will you express dependent containers? Looks like you need a code block with the initialization logic:

class ExampleSpec extends FlatSpec with TestContainersForAll {

  val containers = {
    val c1 = Container1.Def()
    val c2 = Container2.Def(c1.someParam)
    c1 and c2
  }
  
  // Tests here
}

But, this code will not work, because c1.someParam will not be accessible, because you can get some parameters only from started containers. So, you need to start them:

class ExampleSpec extends FlatSpec with TestContainersForAll {

  val containers = {
    val c1 = Container1.Def().start()
    val c2 = Container2.Def(c1.someParam).start()
    c1 and c2
  }
  
  // Tests here
}

But you can't create a val inside your test body, because val is eager and will start to initialize with the test constructor. So, if you want to control containers initialization time, you need to make it def:

class ExampleSpec extends FlatSpec with TestContainersForAll {

  def startContainers = {
    // Initialization here
  }
  
  // Tests here
}

But, how will you provide started containers to the user? It looks like you need some function that will know about startup logic. Like withContainers:

class ExampleSpec extends FlatSpec with TestContainersForAll {

  def startContainers = {
    // Initialization here
  }
  
  it should "test" in withContainers { c1 and c2 =>
    // Test body
  }
}

withContainers should receive a function from containers type to a Unit. But what is containers type exactly? If you want to provide reusable traits like TestContainersForAll you need to abstract from this type somehow. And this is exactly what Containers type do. And initialization block — is the startContainers method. And eventually we end up with the same API:

class ExampleSpec extends FlatSpec with TestContainersForAll {

  override type Containers = Container1 and Container2

  override def startContainers(): Containers = {
    val c1 = Container1.Def().start()
    val c2 = Container2.Def(c1.someParam).start
    c1 and c2
  }
  
  it should "test" in withContainers { c1 and c2 =>
    // Test body
  }
}

Also, if you don't have dependent containers, you can write a bit less verbosely:

def startContainers() = Container1.Def().start() and Container2.Def().start()

Looks decent for me.

Actually, the reasoning above is the exact reasoning of this whole API. This is how I end up with this API.

@dimafeng

This comment has been minimized.

Copy link
Collaborator

dimafeng commented Nov 11, 2019

@LMnet it turned out that it's a large PR 😁 I added a couple of comments re api. I think we if we could make it a bit less verbose and more intuitive it will make the library more easy to use without documentation.

@LMnet

This comment has been minimized.

Copy link
Contributor Author

LMnet commented Nov 12, 2019

@dimafeng I added scaladocs with examples to all suite traits and to all important methods. Also, the new API is a lot more compiler friendly. Scala compiler will advise you if you are doing something wrong. I think it would be enough and makes the new API pretty usable without reading README file.

About verbosity — yes, the new API is a bit more verbose. And I didn't find a way to improve this aspect. But, I think more important that it is safer and more compiler friendly, so it's harder to use it in the wrong way. And verbosity is not really bad — just a bit worse than current. This is a current trade off — a bit more verbose but a bit more safe API. And I think safety is more important.

@dimafeng dimafeng merged commit b05e94d into testcontainers:master Nov 18, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.