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

Could val container by lazy by default? #81

Closed
aesteve opened this issue Nov 15, 2019 · 5 comments · Fixed by #82
Closed

Could val container by lazy by default? #81

aesteve opened this issue Nov 15, 2019 · 5 comments · Fixed by #82

Comments

@aesteve
Copy link
Contributor

aesteve commented Nov 15, 2019

Hello, and thanks for your great work! Using it on a daily basis, works like a charm.

Here's the (mini) problem I ran into.

I have a set of standard test that are run through the test (and thus build task) => they're pure unit tests or PBT, they don't need testcontainers (the faster they run the faster the feedback-loop...)

I have a set of integrationTest using testcontainers-scala. They're quite heavy-on-ressources (docker-compose, etc.). They're simply here to assert that "in a real environment, things should work". They're run through a dedicated task, not every build.

I chose to use an annotation for these integration test classes so that I can exclude them through ScalaTest tags (I'm using scalatest-gradle-plugin). But even when excluded, they're still instanciated, and thus val container = /* ... */ is still executed (checking Docker version etc.).

This is not a real problem for me, since everywhere test are run, there's a docker daemon, but it could be problematic if not.

The very simple fix for me was to declare:

@IntegrationTest
class SomeIntegrationTestNeedingTestContainers extends FeatureSpec with ForAllTestContainers {
  override lazy val container: DockerComposeContainer =  /* declaration */ 
}

This seems to work perfectly fine and achieve what I need it to achieve.
I was just wondering if other people ran into this issue? And if yes, could it be "lazy" by default?

wdyt?

@LMnet
Copy link
Contributor

LMnet commented Nov 15, 2019

It could be def by default. With this, you will achieve an effect you want. And overall it is a good practice to not have vals in the traits. You can create a pull request with this change.

Also, in the near future, we will release a new API in which you will not have the issue you described.

@aesteve
Copy link
Contributor Author

aesteve commented Nov 15, 2019

Yes obviously def works too. I did not propose this since it could be a big breaking change for end-users (having to re-write all their override val to override def should we create a PR with this? Sounds like a "breaking change" (even though it's better I agree.

We also could argue that the container shouldn't be re-created every time the method is called? (hence really being a lazy val and not def) <= not sure about this.

@LMnet
Copy link
Contributor

LMnet commented Nov 15, 2019

It will not break user code — you can override def with val.

@aesteve
Copy link
Contributor Author

aesteve commented Nov 15, 2019

Thanks, I forgot about that. Sorry to not submit a PR, but I'm leaving for 3 weeks with no internet access. I'll address this once I'm back.

Thanks for your explanations!

aesteve pushed a commit to aesteve/testcontainers-scala that referenced this issue Nov 15, 2019
@aesteve
Copy link
Contributor Author

aesteve commented Nov 15, 2019

Got a few minutes to file a PR.

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

Successfully merging a pull request may close this issue.

2 participants