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

Proposal: complete removal of TestLifecycleAware #1347

Closed
LMnet opened this issue Mar 27, 2019 · 12 comments
Closed

Proposal: complete removal of TestLifecycleAware #1347

LMnet opened this issue Mar 27, 2019 · 12 comments

Comments

@LMnet
Copy link

LMnet commented Mar 27, 2019

This proposal based on a recent slack discussion: https://testcontainers.slack.com/archives/C9EJ7TVT7/p1553315996086400.

I propose to delete from the core project TestLifecycleAware class.

Current state

As far as I understand, current project direction looks like this:

  1. The core of a project is taken out as a separate project. This core is absolutely agnostic to the concrete test frameworks.
  2. All integration will be placed into a few subprojects. These subprojects will depend on core subproject and concrete test frameworks.

I fully support this course and I believe that it will simplify the code base and make it more maintainable. This will allow the library to grow faster in the future. In addition, I'm a user and contributor to the testcontainers-scala library, which relies on a testcontainers-java. For the scala facade, the simple and test-framework agnostic core will be a huge win. It will lead to a simpler integration and will give the possibility to delete some non-elegant code from the testcontainers-scala.

One of the steps in this direction was deprecation of a starting, succeeded, failed and finished methods. These methods describe container lifecycle in the tests. They are declared in the FailureDetectingExternalResource class. This is a TestRule for the JUnit 4. All containers are based on this class. When library user wants to add some logic before or after tests, he could use one of those methods.

Disadvantage of the FailureDetectingExternalResource was in hard coupling with JUnit 4. This test rule will work only with this test framework.

As a replacement for the FailureDetectingExternalResource were introduced Startable and TestLifecycleAware interfaces. In Startable there is 2 methods: start and stop, for start and stop logic of a container. In TestLifecycleAware there is beforeTest and afterTest, for the test lifecycle logic.

This new approach is better because it's test-framework agnostic. All test-framework specific logic will be in the integration layer, and this layer will use methods from the Startable and TestLifecycleAware interfaces.

But, I think, Startable is enough, and we don't need to have TestLifecycleAware in the core.

Motivation

To describe why we can remove TestLifecycleAware and don't lose any functionality, I would like to make a digression.

Let's think about typical developer experience with the test frameworks. For examples I will use JUnit 4.

If I want to create a test with JUnit 4, I will create a class with methods, place my test logic in these methods, and mark them with @Test annotation. Like this:

public class MyTest {

    @Test
    public void example() {
        System.out.println("test"); // some test code
    }
}

But it would work only for the simplest cases. Sometimes I need to do something before or after each test. For this JUnit 4 gives me a possibility to mark methods in the test class with the @Before and @After annotations. A code inside these methods will be called before/after each test. For example:

public class MyTest {

    @After
    public void after() {
        System.out.println("after"); // some logic after each test
    }

    @Test
    public void example() {
        System.out.println("test"); // some test code
    }
}

But sometimes I need to do something before or after all tests in the test class. For this case, JUnit 4 gives me @BeforeClass and @AfterClass annotations. For example:

public class MyTest {

    @AfterClass
    public static void afterClass() {
        System.out.println("after class"); // some logic after all tests
    }

    @After
    public void after() {
        System.out.println("after"); // some logic after each test
    }

    @Test
    public void example() {
        System.out.println("test"); // some test code
    }
}

But sometimes it's not enough to do some unconditional code after the test. Sometimes I need to do something only for failed tests, or only for succeeded. For this case, JUnit 4 gives me TestWatcher rule. For example:

public class MyTest {

    @Rule
    public TestWatcher watcher = new TestWatcher() {
        @Override
        protected void failed(Throwable e, Description description) {
            System.out.println("after failed tests"); // some logic after failed tests
        }

        @Override
        protected void succeeded(Description description) {
            System.out.println("after succeeded tests"); // some logic after after succeeded tests
        }
    };

    @Test
    public void example() {
        System.out.println("test"); // some test code
    }
}

We could find similar functionality in other test frameworks too. In JUnit 5 we have @Before/AfterEach, @Before/AfterAll and TestWatcher. In TestNG we have a lot of before/after annotations, including @Before/AfterMethod and @Before/AfterClass. In scalatest, which I use with the testcontainers-scala, I also have similar functionality. And I believe, that almost all test frameworks have some sort of this. Developers are familiar with the patterns from their test framework. These patterns are described in the documentation. They could be googled easily.

Now, let's imagine, that developer decided to use testcontainers for some reason. And, for example, he needs to do something after each test, and he uses JUnit 4. A familiar pattern for the developer is using framework functionality. So, he would use @After annotation for this:

public class MyTest {

    @Rule
    public BrowserWebDriverContainer browser = new BrowserWebDriverContainer();

    @After
    public void after() {
        System.out.println(browser.getSeleniumAddress()); // some real logic
    }

    @Test
    public void example() {
        System.out.println("test"); // some test code
    }
}

As you could see in the example, the developer could place any logic in the after method. He also could somehow interact with the container: call some methods, get some info from it.

With this example, I want to show that containers are not something special from the test framework view and from the developer view. It's just a java object with some methods, which developer could use in the different places in the test class. As I said before, this is a familiar pattern for the developer.

Now, let's return to the TestLifecycleAware. It's supposed that methods from the TestLifecycleAware will be called by the test framework, not a developer. And I think this is the main breaking moment of a TestLifecycleAware.

So, with all other objects (fixtures, counters, instances to test, etc) developer should interact in one way, but with the containers in some other way. Idiomatic logic for testcontainers, in that case, would look like this:

  1. If I want to do something before or after the test, I need to place this logic in a method with @Before/@After annotation.
  2. But If I need to do something with the container, I need to place this logic in the methods inside the container itself.

For example:

public class MyTest {

    class MyBrowserWebDriverContainer<SELF extends MyBrowserWebDriverContainer<SELF>> extends BrowserWebDriverContainer<SELF> {

        @Override
        public void afterTest(TestDescription description, Optional<Throwable> throwable) {
            System.out.println("example"); // some real logic
        }
    }

    @Rule
    public MyBrowserWebDriverContainer browser = new MyBrowserWebDriverContainer();

    @Test
    public void example() {
        System.out.println("test"); // some test code
    }
}

It means that instead of using familiar and unconditional pattern, which is recommended by the test framework, testcontainers force developers to add an exception to these patterns in case of containers.

Of course, I could just not use TestLifecycleAware at all, and use familiar patterns from my test framework. But what If I want to use the container with already implemented methods from the TestLifecycleAware? This container will impose this behavior. And In the case when I want another behavior I will have to create my own container through inheritance and override this behavior.

Disadvantages of the TestLifecycleAware

Let's sum up all disadvantages:

  1. Familiar and recommended by test-framework pattern is broken in case of containers. Testcontainers forces to add an exception to this pattern.
  2. The consequence from the first: testcontainers becomes a bit more difficult for the newcomers. They need to get knowledge about this exception.
  3. Container with methods from TestLifecycleAware impose me behavior from these methods. If I don't need it, I have to override it.
  4. There is no way to interact with the multiple containers inside TestLifecycleAware methods. So, TestLifecycleAware is useful only for some scenarios.
  5. There is no guarantee of order between calls of methods of TestLifecycleAware on multiple containers. For some scenarios, it could be important.
  6. TestLifecycleAware itself doesn't solve any problem. All problems already solved on a test-framework side. TestLifecycleAware just adds some extra level of abstraction.

Alternative approach

I propose to just not use TestLifecycleAware at all. Instead, just use functionality and patterns from your test framework. They already have all the things we need. We don't need to reimplement this in the testcontainers.

Also, without TestLifecycleAware we could delete TestDescription. It needs only for the TestLifecycleAware.

Advantages of the alternative approach

  1. This approach doesn't break familiar patterns from the test framework. Just use test framework in a way you use it before.
  2. Containers don't impose any test behavior. The developer decides, what he want, and when.
  3. More freedom for the developer. In the test hooks from the test framework, I can express any logic, with single or multiple containers.
  4. Core simplification. TestLifecycleAware is not big or complex part of a project, but any deletion from the core is always a good thing for the library.
  5. Integration simplification. With TestLifecycleAware developers from the testcontainers team should think about the integration of TestLifecycleAware with the test framework. It's some extra code, a bit of extra complexity. For example, I'm now working on a new API for a testcontainers-scala. And integration with the TestLifecycleAware takes ~20 lines of code. It's not too much, but all integration is ~100 lines of code. So, TestLifecycleAware is 1/5 of my current integration code. I think it's a noticeable number.
  6. At the current moment, only TestLifecycleAware (excluding deprecated code) somehow tie core with the testing case. Without TestLifecycleAware core module will not have any assumption of a use case. The core will provide containers in the form of java objects. These containers will have some fields and methods. A developer could interact with these containers in a way he wants — containers not assume anything about the use case. It means less coupling with the test scenario, more wide use case for the core. Of course, you could use testcontainers in that way today too. But the existence of TestLifecycleAware in that case looks like a redundant, leaky abstraction.

Disdvantages of the alternative approach

I can see these disadvantages:

  1. It's a breaking change. Possible solution: deprecation period.
  2. If a container is heavily relying on the logic from the TestLifecycleAware methods, and we removed these methods, developers will have to reimplement this logic on their side. This problem may be relevant to the BrowserWebDriverContainer. This is the only container in the library with this logic. Possible solution: improve BrowserWebDriverContainer to give a simple way to reimplement this logic on the user side. Ideally, with the single method call. Another solution, which could be used with the previous one, is to provide functionality from the BrowserWebDriverContainerю.afterTest on the integration level.

Summary

I believe that we just don't need TestLifecycleAware. And if we delete TestLifecycleAware we don't lose anything, and only get some advantages I described above.

@bsideup
Copy link
Member

bsideup commented Mar 27, 2019

I think the whole misunderstanding comes from this block:

  1. If I want to do something before or after the test, I need to place this logic in a method with @Before/@after annotation.
  2. But If I need to do something with the container, I need to place this logic in the methods inside the container itself.

#2 is absolutely incorrect and we never advocate this. The methods exist so that the container's implementation can do something before/after a test, not the end user.

These methods will be executed by the testing framework (e.g. see #1326 ), and this is why we need an interface - to provide a generic set of methods to be executed by the testing frameworks.

@kiview
Copy link
Member

kiview commented Mar 27, 2019

Integration simplification. With TestLifecycleAware developers from the Testcontainers team should think about the integration of TestLifecycleAware with the test framework. It's some extra code, a bit of extra complexity.

I think you are weighing this aspect differently than the Testcontainers team.
It's a good thing that TestLifecycleAware should be considered (and ideally implemented) when writing integrations for other test frameworks. This allows us to specify test related container behavior once and have it directly available in all test framework integrations.

You are right that the only class making use of this feature at the moment is BrowserWebdriverContainer. However this feature is valued highly by many users (taking user feedback in account) and lack of TestLifecycleAware support in test framework integrations (like in the Spock integration) is quickly discovered by users, much to their dismay.
In addition, and as already mentioned in the Slack discussion, we have plans to implement further functionality that will make use of those callbacks, like Database and filesystem snapshots.

I'm not sure if we can come to an agreement, since we definitely put a focus on different aspects here.

dbyron0 referenced this issue in locationlabs/testcontainers-java Mar 27, 2019
Bumps [lombok](https://github.com/rzwitserloot/lombok) from 1.16.6 to 1.18.4.
<details>
<summary>Changelog</summary>

*Sourced from [lombok's changelog](https://github.com/rzwitserloot/lombok/blob/master/doc/changelog.markdown).*

> ### v1.18.4 (October 30th, 2018)
> * PLATFORM: Support for Eclipse Photon. [Issue testcontainers#1831](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1831)
> * PLATFORM: Angular IDE is now recognized by the installer [Issue testcontainers#1830](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1830)
> * PLATFORM: Many improvements for lombok's JDK10/11 support.
> * BREAKING CHANGE: The `[**FieldNameConstants**](https://github.com/FieldNameConstants)` feature has been completely redesigned. [Issue testcontainers#1774](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1774) [FieldNameConstants documentation](https://projectlombok.org/features/experimental/FieldNameConstants)
> * BREAKING CHANGE: Lombok will now always copy specific annotations around (from field to getter, from field to builder 'setter', etcetera): A specific curated list of known annotations where that is the right thing to do (generally, `[**NonNull**](https://github.com/NonNull)` style annotations from various libraries), as well as any annotations you explicitly list in the `lombok.copyableAnnotations` config key in your `lombok.config` file. Also, lombok is more consistent about copying these annotations. (Previous behaviour: Lombok used to copy any annotation whose simple name was `NonNull`, `Nullable`, or `CheckForNull`). [Issue #1570](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1570) and [Issue testcontainers#1634](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1634)
> * FEATURE: Lombok's `[**NonNull**](https://github.com/NonNull)` annotation can now be used on type usages (annotation on type usages has been introduced in JDK 8). `[**Builder**](https://github.com/Builder)`'s `[**Singular**](https://github.com/Singular)` annotation now properly deals with annotations on the generics type on the collection: `[**Singular**](https://github.com/Singular) List<[**NonNull**](https://github.com/NonNull) String> names;` now does the right thing.
> * FEATURE: You can now mix `[**SuperBuilder**](https://github.com/SuperBuilder)` and `toBuilder`, and `toBuilder` no longer throws `NullPointerException` if a `[**Singular**](https://github.com/Singular)`-marked collection field is `null`. [Issue #1324](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1324)
> * FEATURE: delombok now supports module paths via the `--module-path` option, and will automatically add lombok itself to the module path. This should make it possible to delombok your modularized projects. [Issue testcontainers#1848](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1848)
> * FEATURE: You can pass `[**args**](https://github.com/args).txt` to `delombok` to read args from the text file; useful if you have really long classpaths you need to pass to delombok. [Issue testcontainers#1795](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1795)
> * BUGFIX: `[**NoArgsConstructor**](https://github.com/NoArgsConstructor)(force=true)` would try to initialize already initialized final fields in Eclipse. [Issue testcontainers#1829](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1829)
> * BUGFIX: When using lombok to compile modularized (`module-info.java`-style) code, if the module name has dots in it, it wouldn't work. [Issue testcontainers#1808](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1808)
> * BUGFIX: Errors about lombok not reading a module providing `org.mapstruct.ap.spi` when trying to use lombok in jigsaw-mode on JDK 11. [Issue testcontainers#1806](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1806)
> * BUGFIX: Fix NetBeans compile on save. [Issue testcontainers#1770](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1770)
> * BUGFIX: If you manually write your builder class so you can add a few methods of your own, and those methods refer to generated methods, you'd usually run into various bizarre error messages, but only on JDK9/10/11. This one is hard to describe, but we fixed it. [Issue testcontainers#1907](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1907)
> 
> 
> ### v1.18.2 (July 26th, 2018)
> * BUGFIX: mapstruct + lombok in eclipse should hopefully work again. [Issue #1359](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1359) and [mapstruct issue #1159](https://github-redirect.dependabot.com/mapstruct/mapstruct/issues/1159)
> * BUGFIX: Equals and hashCode again exclude transient fields by default. [Issue testcontainers#1724](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1724)
> * BUGFIX: Eclipse 'organize imports' feature (either explicitly, or if automatically triggered on saving via 'save actions') would remove the import for `lombok.var`. [Issue testcontainers#1783](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1783)
> * BUGFIX: Lombok and gradle v4.9 didn't work together; that's been fixed. [Issue testcontainers#1716](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1716) and [gradle-apt-plugin issue #87](https://github-redirect.dependabot.com/tbroyer/gradle-apt-plugin/issues/87)
> * FEATURE: You can now make builders for type hierarchies, using the new (experimental) `[**SuperBuilder**](https://github.com/SuperBuilder)` annotation. Thanks for the contribution, Jan Rieke. [`[**SuperBuilder**](https://github.com/SuperBuilder)` documentation](https://projectlombok.org/features/experimental/SuperBuilder)
> * FEATURE: `[**NoArgsConstructor**](https://github.com/NoArgsConstructor)`, including forcing one with `lombok.config: lombok.noArgsConstructor.extraPrivate=true` now take any defaults set with `[**Builder**](https://github.com/Builder).Default` into account. [Issue #1347](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1347)
> ### v1.18.0 (June 5th, 2018)
> * BREAKING CHANGE: The in 1.16.22 introduced configuration key `lombok.noArgsConstructor.extraPrivate` is now `false` by default. [Issue testcontainers#1708](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1708)
> * BUGFIX: Do not generate a private no-args constructor if that breaks the code. [Issue testcontainers#1703](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1703), [Issue testcontainers#1704](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1704), [Issue testcontainers#1712](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1712)
> * BUGFIX: Using boolean parameters in lombok annotations would fail. [Issue testcontainers#1709](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1709)
> * BUGFIX: Delombok would give an error message. [Issue testcontainers#1705](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1705)
> * BUGFIX: Eclipse java10 var support didn't work if lombok was installed in your eclipse. [Issue testcontainers#1676](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1676)
> * FEATURE: Google's [Flogger (a.k.a. FluentLogger)](https://google.github.io/flogger/) is now available via `[**Flogger**](https://github.com/Flogger)`. [Issue testcontainers#1697](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1697)
> * FEATURE: `[**FieldNameConstants**](https://github.com/FieldNameConstants)` has been extended to support prefixes and suffixes. By default, the generated constants are prefixed with `FIELD_`. [Docs on [**FieldNameConstants**](https://github.com/FieldNameConstants)](https://projectlombok.org/features/experimental/FieldNameConstants).
> 
> ### v1.16.22 "Envious Ferret" (May 29th, 2018)
> * FEATURE: Private no-args constructor for `[**Data**](https://github.com/Data)` and `[**Value**](https://github.com/Value)` to enable deserialization frameworks (like Jackson) to operate out-of-the-box. Use `lombok.noArgsConstructor.extraPrivate = false` to disable this behavior.
> * FEATURE: Methods can now be marked for inclusion in `toString`, `equals`, and `hashCode` generation. There is a new mechanism to mark which fields (and now, methods) are to be included or excluded for the generation of these methods: mark the relevant member with for example `[**ToString**](https://github.com/ToString).Include` or `[**EqualsAndHashCode**](https://github.com/EqualsAndHashCode).Exclude`. [ToString documentation](https://projectlombok.org/features/ToString) [EqualsAndHashCode documentation](https://projectlombok.org/features/EqualsAndHashCode)
> * FEATURE: `[**Getter**](https://github.com/Getter)` and `[**Setter**](https://github.com/Setter)` also allow `onMethod` and `onParam` when put on a type. [Issue testcontainers#1653](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1653) 
> * FEATURE: `[**FieldNameConstants**](https://github.com/FieldNameConstants)` is a new feature that generates string constants for your field names. [Docs on [**FieldNameConstants**](https://github.com/FieldNameConstants)](https://projectlombok.org/features/experimental/FieldNameConstants).
> * PLATFORM: Lombok can be compiled on JDK10, and should run on JDK10. [Issue testcontainers#1693](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1693)
> * PLATFORM: lombok now counts as an _incremental annotation processor_ for gradle. Should speed up your gradle builds considerably! [Issue testcontainers#1580](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1580)
> * PLATFORM: Fix for using lombok together with JDK9+'s new `module-info.java` feature. [Issue #985](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/985)
> * BUGFIX: Solved some issues in eclipse that resulted in error 'A save participant caused problems'. [Issue #879](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/879)
> * BUGFIX: Netbeans on jdk9. [Issue testcontainers#1617](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1617)
> * BUGFIX: Netbeans < 9. [Issue #1555](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1555)
> * PROMOTION: `var` has been promoted from experimental to the main package with no changes. The 'old' experimental one is still around but is deprecated, and is an alias for the new main package one. [var documentation](https://projectlombok.org/features/var.html).
> * OLD-CRUFT: `lombok.experimental.Builder` and `lombok.experimental.Value` are deprecated remnants of when these features were still in experimental. They are now removed entirely. If your project is dependent on an older version of lombok which still has those; fret not, lombok still processes these annotations. It just no longer includes them in the jar.
> 
> ### v1.16.20 (January 9th, 2018)
> * PLATFORM: Better support for jdk9 in the new IntelliJ, Netbeans and for Gradle.
> * BREAKING CHANGE: _lombok config_ key `lombok.addJavaxGeneratedAnnotation` now defaults to `false` instead of true. Oracle broke this annotation with the release of JDK9, necessitating this breaking change.
></table> ... (truncated)
</details>
<details>
<summary>Commits</summary>

- [`8451c88`](projectlombok/lombok@8451c88) pre-release version bump
- [`e960f48`](projectlombok/lombok@e960f48) cleaning up the changelog in preparation for a release.
- [`9b06018`](projectlombok/lombok@9b06018) [fixes testcontainers#1907] This one is hard to describe; due to builder being a bit overze...
- [`f5b1069`](projectlombok/lombok@f5b1069) add jdk11 to docker builds
- [`0336537`](projectlombok/lombok@0336537) fixing the tests added in the previous commits by janrieke to match alternati...
- [`d8de0e3`](projectlombok/lombok@d8de0e3) Merge branch 'wildcardsSingularFix' of git://github.com/janrieke/lombok into ...
- [`eca219e`](projectlombok/lombok@eca219e) eliminate ‘you are using private API’ warnings by streamlining all reflective...
- [`182cb0c`](projectlombok/lombok@182cb0c) [java-11] up dependency on lombok.patcher, including asm7
- [`c02263a`](projectlombok/lombok@c02263a) Merge pull request [testcontainers#1923](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1923) from abargnesi/fix-maven-issue-management
- [`4cf36b2`](projectlombok/lombok@4cf36b2) Merge pull request [testcontainers#1917](https://github-redirect.dependabot.com/rzwitserloot/lombok/issues/1917) from kkocel/master
- Additional commits viewable in [compare view](projectlombok/lombok@v1.16.6...v1.18.4)
</details>
<br />

[![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=org.projectlombok:lombok&package-manager=maven&previous-version=1.16.6&new-version=1.18.4)](https://dependabot.com/compatibility-score.html?dependency-name=org.projectlombok:lombok&package-manager=maven&previous-version=1.16.6&new-version=1.18.4)

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

**Note:** This repo was added to Dependabot recently, so you'll receive a maximum of 5 PRs for your first few update runs. Once an update run creates fewer than 5 PRs we'll remove that limit.

You can always request more updates by clicking `Bump now` in your [Dependabot dashboard](https://app.dependabot.com).

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme

Additionally, you can set the following in the `.dependabot/config.yml` file in this repo:
- Update frequency (including time of day and day of week)
- Automerge options (never/patch/minor, and dev/runtime dependencies)
- Pull request limits (per update run and/or open at any time)
- Out-of-range updates (receive only lockfile updates, if desired)
- Security updates (receive only security updates, if desired)

Finally, you can contact us by mentioning @dependabot.

</details>
@LMnet
Copy link
Author

LMnet commented Mar 28, 2019

#2 is absolutely incorrect and we never advocate this.

Even if there is no explicit recommendation about it, the existence of 2 different places for logic before/after test could confuse newcomers:

  • Why there is a beforeTest and afterTest in the containers when I already have hooks from my test framework?
  • In which cases should I use methods from the TestLifecycleAware? And in which cases should I use hooks from my test framework?
  • What If I don't want to use behavior from methods from the TestLifecycleAware?

The methods exist so that the container's implementation can do something before/after a test, not the end user.

Why containers should contains this logic inside? It's not a container logic, it's a test logic. A test logic should be placed inside tests, or inside some test helpers. A developer should be able to have full control under his tests. No need to impose some test behavior to the developer.

These methods will be executed by the testing framework (e.g. see #1326 ), and this is why we need an interface - to provide a generic set of methods to be executed by the testing frameworks.

If containers will just provide methods with the same logic as we have now in the TestLifecycleAware methods, the developer will still be able to use this logic. But this logic will not be imposed on a developer. As I said before, this is a test logic, not container logic. A developer should control it, not a container.

I think you are weighing this aspect differently than the Testcontainers team.

Yes, it's not a big deal to add this integration. But it worth mentioning.

You are right that the only class making use of this feature at the moment is BrowserWebdriverContainer.

I don't think that this is important. Number of integrations with the TestLifecycleAware does not affect on my arguments from the proposal

However this feature is valued highly by many users (taking user feedback in account) and lack of TestLifecycleAware support in test framework integrations (like in the Spock integration) is quickly discovered by users, much to their dismay.

I'm not sure about it. As I already mentioned, TestLifecycleAware doesn't provide any new or unique functionality. Developers can use functionality from their test frameworks to solve the same problems.

In addition, and as already mentioned in the Slack discussion, we have plans to implement further functionality that will make use of those callbacks, like Database and filesystem snapshots.

That's great. Just provide this functionality as a methods without assumptions about caller use cases. Let's take database snapshots for example. To give this functionality you can add a method like saveShapshot(file). And that's it. Developers will be able to use in inside tests. Or after tests. Or before tests. It's fully under developer control.

I'm not sure if we can come to an agreement, since we definitely put a focus on different aspects here.

That's why I created this proposal. I believe that even if we have different views or use cases we can still come to the right decision.

I think you are concentrating too much on a current situation and historical reasons. Of course, testcontainers has features to do some stuff before/after tests for a while. This features is familiar and feels usable. But just imagine that testcontainers doen't have any kind of this functionality. And all other features are in the library. Would you add something like TestLifecycleAware in the library in this case? I don't think so. TestLifecycleAware doesn't provide any unique functionality and doesn't solve any real issue. You can solve all problems that TestLifecycleAware trying to solve without it.

That's the main reason why I propose to delete TestLifecycleAware. It's just redundant. And deletion of TestLifecycleAware in the current library state will give us a few advantages I mentioned before.

@bsideup
Copy link
Member

bsideup commented Mar 28, 2019

the existence of 2 different places for logic before/after test could confuse newcomers

I think you're speculating. Newcomers will never know about that interface. We have it, so that the testing frameworks can integrate with Testcontainers. If a newcomer writes an integration, he is considered an advanced user.

Why there is a beforeTest and afterTest in the containers when I already have hooks from my test framework?

Because they serve different purposes. You, as a user of your test framework, use the hooks to do your stuff. Your test framework, if integrated with Testcontainers, uses beforeTest/afterTest to match your expectations (e.g. @Rule in JUnit 4 or @ExtendWith() + @Container in JUnit 5).
beforeTest/afterTest should not be used in the tests' code.

In which cases should I use methods from the TestLifecycleAware? And in which cases should I use hooks from my test framework?

Never, as a user. As mentioned already, you use them only then you integrate with a testing framework.

What If I don't want to use behavior from methods from the TestLifecycleAware?

Then you don't use the rules / @Container annotation, etc. As a user, you don't know about TestLifecycleAware. You only know about your testing framework, and that the @Rule marked field, for instance, will be controlled by JUnit. You don't want it to be controlled - you remove framework's ways of controlling it.

Why containers should contains this logic inside? It's not a container logic, it's a test logic.
No need to impose some test behavior to the developer.

Take a look at the project's name. It will give you a hint ;)

I'm not sure about it

We are. Testcontainers is the result of many years of development, community communication and feedback listening. We would not become the most popular Docker testing library out there without it. Just think about it.

Just provide this functionality as a methods without assumptions about caller use cases. Let's take database snapshots for example. To give this functionality you can add a method like saveShapshot(file). And that's it. Developers will be able to use in inside tests. Or after tests. Or before tests. It's fully under developer control.

One of the most beloved aspect of Testcontainers is that you create a container, annotate it with @Rule, for instance, and it works. With the recording or any other feature. That is how most of the users are using it.
If you are an advanced user and want to control everything, the methods are there (e.g. .start(), .stop() or saveShapshot(file) if/when we add it). Just we make it easier for the ones who want to solve their testing problem instead of controlling everything.

I believe that even if we have different views or use cases we can still come to the right decision.
I think you are concentrating too much on a current situation and historical reasons
TestLifecycleAware doesn't provide any unique functionality and doesn't solve any real issue

I suggest you to think about it differently. We have our view on the problem because we were solving a problem. Meanwhile, you're trying to push your idea, with your only argument is "it doesn't provide any unique functionality". If you really want to help the project, interview a few QA people about their experience with the browser container and how it "magically works without them doing anything because they are not hardcore developers".

give us a few advantages I mentioned before

That's a speculation. "Less code" is never an advantage per se, as long as we're (as a team) ready to maintain it.

Also, when you say "us" - what does it mean to you?

@rnorth
Copy link
Member

rnorth commented Mar 28, 2019

I fully agree with @bsideup and @kiview here.

A lot of what Testcontainers is for is to stop developers having to write/extend boilerplate classes. If we were being strict about giving full control to developers and having no test framework/lifecycle coupling, we wouldn't have @Rule, for example. Instead, we took an opinionated position on having various elements of test framework integration that we think saves people effort.

The trouble with opinionated design decisions is that they tend to have disadvantages, and tend to face disagreement at some time. I think that this is the case here, but unfortunately I don't see a way to find a middle ground. I'm sorry, but we might have to 'agree to disagree'.

FWIW, If/when we get around to creating a non-testing container library, with Testcontainers becoming a testing-related library on top of it, this interface is likely to move and/or change. I'm afraid that now is a bit too soon to know exactly what we'll do with it though.

BTW I do really appreciate that you've put a lot of time and thought into this proposal, and been civil throughout on a topic you feel strongly about. Thank you for this.

@LMnet
Copy link
Author

LMnet commented Mar 28, 2019

I think you're speculating. Newcomers will never know about that interface. We have it, so that the testing frameworks can integrate with Testcontainers. If a newcomer writes an integration, he is considered an advanced user.

I'm talking about developers who saw TestLifecycleAware for the first time. I think they could be surprised by this.

Your test framework, if integrated with Testcontainers, uses beforeTest/afterTest to match your expectations

I'm not sure about what expectations you are talking about. Could you give an example?

beforeTest/afterTest should not be used in the tests' code.

Maybe I was not 100% clear, but I never proposed this. I'm talking about logic inside these methods. This logic should be available for the developers without TestLifecycleAware too.

Never, as a user. As mentioned already, you use them only then you integrate with a testing framework.

You are talking about the usage part. But I was talking about the implementation of containers. In which situation should I put the logic after test in the container, and in which in the test-framework hook? This is the confusing part in my opinion.

Then you don't use the rules / @container annotation, etc.

But in that case, I will lose start/stop functionality too, which is always useful.

We have our view on the problem because we were solving a problem.

About which problem you are talking about?

interview a few QA people about their experience with the browser container and how it "magically works without them doing anything because they are not hardcore developers".

Well, yes, I don't have enough side look about this. But for me, there is no difference between doing something inside the test or after the test. If QA people can call methods on the container inside the test body, they can do the same after the test, using the test-framework feature.

"Less code" is never an advantage per se, as long as we're (as a team) ready to maintain it.

This is a bit philosophical, but I do not agree with your opinion. Less code is always better than any code. You could have bugs in this code. You should evolve this code. But it's not really important. When I'm talking about "few advantages" I, first of all, mean less opinionated core.

Also, when you say "us" - what does it mean to you?

It's contextual. I will try to be more clear.

If we were being strict about giving full control to developers and having no test framework/lifecycle coupling, we wouldn't have @rule, for example.

I think integration with test frameworks could use only start/stop. And it's enough for most cases.

Instead, we took an opinionated position on having various elements of test framework integration that we think saves people effort.

As far as I know, at the current moment, only Startable and TestLifecycleAware are taking part in the integration. The Startable is, in any case, is necessary for all scenarios with containers. But in my opinion TestLifecycleAware is not.


So, your position is that testcontainers provides docker containers which are useful mostly for tests. And also, testcontainers provides some default behavior inside the tests with the methods from TestLifecycleAware. In the case of BrowserWebDriverContainer it's a behavior of saving videos after a test. In the case of DB containers, it could be saving snapshots, as was already mentioned by @kiview. Am I right?

@kiview
Copy link
Member

kiview commented Mar 28, 2019

@LMnet I respect the time and the thought you put into this discussion, but TBH, this whole argument seems like the ultimate example in bikeshedding and I therefore won't go on participating in it any further.

We all have limited capacities for supporting and developing the project and this is not the hill I plan to die on. It's all about prioritization and pragmatism in the face of limited resources and if you look at the open issues and feature requests, I'd assume you agree, there are more valuable things to spend energy on. Not mentioning the hassle for our users when pushing a breaking change.

Are there any concrete problems you are facing in your developer life because of TestLifecycleAware?

@LMnet
Copy link
Author

LMnet commented Mar 28, 2019

I want to give some prehistory. I'm working on a new Scala API in the testcontainers-scala. Current Scala API uses starting, succeeded, failed and finished methods. These methods are deprecated. Instead of them, there is a new approach with the TestLifecycleAware. But in the new Scala API, I wanted to make the core module of testcontainers-scala as generic as possible. TestLifecycleAware is a hard coupling with the test scenario. I started to think about it and discussed it. And as more I thought about it, the more I convinced that TestLifecycleAware is a redundant functionality.

Without TestLifecycleAware I could achieve my goal to create a generic core. On top of this core, I could create a more opinionated module with some test logic. On top of this module, I could create a bunch of integrations with test frameworks. I believe that this is the right direction for the library API. And for testcontainers-java too. But to split one module (current state) to 2+n modules in the testcontainers-java library authors needs to spend pretty much time. This is not an option for now.

But if you ignore deprecated stuff and see on the current state of a testcontainers-java you could see, that the only obstacle on the way of the generic core module is TestLifecycleAware. And even without this, removing of TestLifecycleAware could give some advantages to the testcontainers-java in my opinion.

That's why I created this proposal. I didn't mention testcontainers-scala a lot because it's an internal course of a testcontainers-scala and this course should not affect testcontainers-java. I tried to use arguments specific only for the java library.

I can accept and understand that testcontainers-java is not ready now for this separation to 2+n modules. But as I already said, we now only one step away from the generic core. If testcontainers-java will continue its opinionated course, it potentially means more test-specific functionality (like more containers with methods from the TestLifecycleAware). And it means that in future we, potentially, will not have a possibility to split the library to the 2+n modules without huge breaking changes.

@bsideup
Copy link
Member

bsideup commented Mar 28, 2019

I wanted to make the core module of testcontainers-scala as generic as possible.
Without TestLifecycleAware I could achieve my goal to create a generic core. On top of this core, I could create a more opinionated module with some test logic. On top of this module, I could create a bunch of integrations with test frameworks.

I don't think you should be doing it in testcontainers-scala. That library is just a wrapper (or a Scala language binding) around the main one.

We do plan to create "containercore" library), but it will take time and it is quite pointless to achieve it in a separate library given the main project is still heavily test-oriented (not just the lifecycle, but also the cleanup at JVM exit, for example, or random names).

But as I already said, we now only one step away from the generic core

Sorry, but you're missing the bigger picture. Please leave it to the Testcontainers team to decide.

@LMnet
Copy link
Author

LMnet commented Mar 28, 2019

Sorry, but you're missing the bigger picture.

It's absolutely true and I could miss some parts.

I don't think you should be doing it in testcontainers-scala.

This is not a final decision atm. But it looks like Scala API could progress a lot faster because it's a lot smaller. That's why I think I could achieve this in the next few releases.

We do plan to create "containercore" library), but it will take time and it is quite pointless to achieve it in a separate library given the main project is still heavily test-oriented (not just the lifecycle, but also the cleanup at JVM exit, for example, or random names).

Well, from the messages in this discussion it looks like this is pretty long plans. But if this is a current long-term strategy, I want to help these plans come true earlier.

@stale
Copy link

stale bot commented Jun 26, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

@stale stale bot added the stale label Jun 26, 2019
@stale
Copy link

stale bot commented Jul 10, 2019

This issue has been automatically closed due to inactivity. We apologise if this is still an active problem for you, and would ask you to re-open the issue if this is the case.

@stale stale bot closed this as completed Jul 10, 2019
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

4 participants