payload: {"action":"created","issue":{"url":"https://api.github.com/repos/testcontainers/testcontainers-java/issues/1347","repository_url":"https://api.github.com/repos/testcontainers/testcontainers-java","labels_url":"https://api.github.com/repos/testcontainers/testcontainers-java/issues/1347/labels{/name}","comments_url":"https://api.github.com/repos/testcontainers/testcontainers-java/issues/1347/comments","events_url":"https://api.github.com/repos/testcontainers/testcontainers-java/issues/1347/events","html_url":"https://github.com/testcontainers/testcontainers-java/issues/1347","id":426106116,"node_id":"MDU6SXNzdWU0MjYxMDYxMTY=","number":1347,"title":"Proposal: complete removal of TestLifecycleAware","user":{"login":"LMnet","id":2826500,"node_id":"MDQ6VXNlcjI4MjY1MDA=","avatar_url":"https://avatars0.githubusercontent.com/u/2826500?v=4","gravatar_id":"","url":"https://api.github.com/users/LMnet","html_url":"https://github.com/LMnet","followers_url":"https://api.github.com/users/LMnet/followers","following_url":"https://api.github.com/users/LMnet/following{/other_user}","gists_url":"https://api.github.com/users/LMnet/gists{/gist_id}","starred_url":"https://api.github.com/users/LMnet/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/LMnet/subscriptions","organizations_url":"https://api.github.com/users/LMnet/orgs","repos_url":"https://api.github.com/users/LMnet/repos","events_url":"https://api.github.com/users/LMnet/events{/privacy}","received_events_url":"https://api.github.com/users/LMnet/received_events","type":"User","site_admin":false},"labels":[{"id":835896422,"node_id":"MDU6TGFiZWw4MzU4OTY0MjI=","url":"https://api.github.com/repos/testcontainers/testcontainers-java/labels/type/housekeeping","name":"type/housekeeping","color":"d4c5f9","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":10,"created_at":"2019-03-27T17:58:35Z","updated_at":"2019-06-26T16:20:29Z","closed_at":null,"author_association":"NONE","body":"This proposal based on a recent slack discussion: https://testcontainers.slack.com/archives/C9EJ7TVT7/p1553315996086400.\r\n\r\nI propose to delete from the core project `TestLifecycleAware` class.\r\n\r\n# Current state\r\n\r\nAs far as I understand, current project direction looks like this:\r\n1. The core of a project is taken out as a separate project. This core is absolutely agnostic to the concrete test frameworks.\r\n2. All integration will be placed into a few subprojects. These subprojects will depend on core subproject and concrete test frameworks. \r\n\r\nI 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.\r\n\r\nOne 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.\r\n\r\nDisadvantage of the `FailureDetectingExternalResource` was in hard coupling with JUnit 4. This test rule will work only with this test framework.\r\n\r\nAs 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.\r\n\r\nThis 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.\r\n\r\nBut, I think, `Startable` is enough, and we don't need to have `TestLifecycleAware` in the core.\r\n\r\n# Motivation\r\n\r\nTo describe why we can remove `TestLifecycleAware` and don't lose any functionality, I would like to make a digression. \r\n\r\nLet's think about typical developer experience with the test frameworks. For examples I will use JUnit 4. \r\n\r\nIf 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:\r\n\r\n```java\r\npublic class MyTest {\r\n\r\n    @Test\r\n    public void example() {\r\n        System.out.println(\"test\"); // some test code\r\n    }\r\n}\r\n```\r\n\r\nBut 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:\r\n\r\n```java\r\npublic class MyTest {\r\n\r\n    @After\r\n    public void after() {\r\n        System.out.println(\"after\"); // some logic after each test\r\n    }\r\n\r\n    @Test\r\n    public void example() {\r\n        System.out.println(\"test\"); // some test code\r\n    }\r\n}\r\n```\r\n\r\nBut 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: \r\n\r\n```java\r\npublic class MyTest {\r\n\r\n    @AfterClass\r\n    public static void afterClass() {\r\n        System.out.println(\"after class\"); // some logic after all tests\r\n    }\r\n\r\n    @After\r\n    public void after() {\r\n        System.out.println(\"after\"); // some logic after each test\r\n    }\r\n\r\n    @Test\r\n    public void example() {\r\n        System.out.println(\"test\"); // some test code\r\n    }\r\n}\r\n```\r\n\r\nBut 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:\r\n\r\n```java\r\npublic class MyTest {\r\n\r\n    @Rule\r\n    public TestWatcher watcher = new TestWatcher() {\r\n        @Override\r\n        protected void failed(Throwable e, Description description) {\r\n            System.out.println(\"after failed tests\"); // some logic after failed tests\r\n        }\r\n\r\n        @Override\r\n        protected void succeeded(Description description) {\r\n            System.out.println(\"after succeeded tests\"); // some logic after after succeeded tests\r\n        }\r\n    };\r\n\r\n    @Test\r\n    public void example() {\r\n        System.out.println(\"test\"); // some test code\r\n    }\r\n}\r\n```\r\n\r\nWe 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.\r\n\r\nNow, 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:\r\n```java\r\npublic class MyTest {\r\n\r\n    @Rule\r\n    public BrowserWebDriverContainer browser = new BrowserWebDriverContainer();\r\n\r\n    @After\r\n    public void after() {\r\n        System.out.println(browser.getSeleniumAddress()); // some real logic\r\n    }\r\n\r\n    @Test\r\n    public void example() {\r\n        System.out.println(\"test\"); // some test code\r\n    }\r\n}\r\n```\r\nAs 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.\r\n\r\nWith 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.\r\n\r\nNow, 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`.\r\n\r\nSo, 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: \r\n1. If I want to do something before or after the test, I need to place this logic in a method with `@Before/@After` annotation.\r\n2. But If I need to do something with the container, I need to place this logic in the methods inside the container itself.\r\n\r\nFor example:\r\n```java\r\npublic class MyTest {\r\n\r\n    class MyBrowserWebDriverContainer<SELF extends MyBrowserWebDriverContainer<SELF>> extends BrowserWebDriverContainer<SELF> {\r\n\r\n        @Override\r\n        public void afterTest(TestDescription description, Optional<Throwable> throwable) {\r\n            System.out.println(\"example\"); // some real logic\r\n        }\r\n    }\r\n\r\n    @Rule\r\n    public MyBrowserWebDriverContainer browser = new MyBrowserWebDriverContainer();\r\n\r\n    @Test\r\n    public void example() {\r\n        System.out.println(\"test\"); // some test code\r\n    }\r\n}\r\n\r\n```\r\n\r\nIt 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.\r\n\r\nOf 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. \r\n\r\n## Disadvantages of the `TestLifecycleAware`\r\n\r\nLet's sum up all disadvantages: \r\n1. Familiar and recommended by test-framework pattern is broken in case of containers. Testcontainers forces to add an exception to this pattern.\r\n2. The consequence from the first: testcontainers becomes a bit more difficult for the newcomers. They need to get knowledge about this exception.\r\n3. Container with methods from `TestLifecycleAware` impose me behavior from these methods. If I don't need it, I have to override it.\r\n4. There is no way to interact with the multiple containers inside `TestLifecycleAware` methods. So, `TestLifecycleAware` is useful only for some scenarios. \r\n5. There is no guarantee of order between calls of methods of `TestLifecycleAware` on multiple containers. For some scenarios, it could be important.\r\n6. `TestLifecycleAware` itself doesn't solve any problem. All problems already solved on a test-framework side. `TestLifecycleAware` just adds some extra level of abstraction. \r\n\r\n# Alternative approach\r\n\r\nI 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.\r\n\r\nAlso, without `TestLifecycleAware` we could delete `TestDescription`. It needs only for the `TestLifecycleAware`.\r\n\r\n## Advantages of the alternative approach\r\n\r\n1. This approach doesn't break familiar patterns from the test framework. Just use test framework in a way you use it before.\r\n2. Containers don't impose any test behavior. The developer decides, what he want, and when.\r\n3. More freedom for the developer. In the test hooks from the test framework, I can express any logic, with single or multiple containers.\r\n4. 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.\r\n5. 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.\r\n6. 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.\r\n\r\n## Disdvantages of the alternative approach\r\n\r\nI can see these disadvantages:\r\n1. It's a breaking change. Possible solution: deprecation period.\r\n2. 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.\r\n\r\n# Summary\r\n\r\nI 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.\r\n"},"comment":{"url":"https://api.github.com/repos/testcontainers/testcontainers-java/issues/comments/505946208","html_url":"https://github.com/testcontainers/testcontainers-java/issues/1347#issuecomment-505946208","issue_url":"https://api.github.com/repos/testcontainers/testcontainers-java/issues/1347","id":505946208,"node_id":"MDEyOklzc3VlQ29tbWVudDUwNTk0NjIwOA==","user":{"login":"stale[bot]","id":26384082,"node_id":"MDM6Qm90MjYzODQwODI=","avatar_url":"https://avatars3.githubusercontent.com/in/1724?v=4","gravatar_id":"","url":"https://api.github.com/users/stale%5Bbot%5D","html_url":"https://github.com/apps/stale","followers_url":"https://api.github.com/users/stale%5Bbot%5D/followers","following_url":"https://api.github.com/users/stale%5Bbot%5D/following{/other_user}","gists_url":"https://api.github.com/users/stale%5Bbot%5D/gists{/gist_id}","starred_url":"https://api.github.com/users/stale%5Bbot%5D/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stale%5Bbot%5D/subscriptions","organizations_url":"https://api.github.com/users/stale%5Bbot%5D/orgs","repos_url":"https://api.github.com/users/stale%5Bbot%5D/repos","events_url":"https://api.github.com/users/stale%5Bbot%5D/events{/privacy}","received_events_url":"https://api.github.com/users/stale%5Bbot%5D/received_events","type":"Bot","site_admin":false},"created_at":"2019-06-26T16:20:29Z","updated_at":"2019-06-26T16:20:29Z","author_association":"NONE","body":"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.\n"}}
other: {"actor":{"display_login":"stale"}}