testcontainers-java icon indicating copy to clipboard operation
testcontainers-java copied to clipboard

Support capturing separate screen recording files specific to each test method when reusing a single instance of BrowserWebDriverContainer for multiple test methods

Open erdi opened this issue 4 years ago • 4 comments

Starting a new instance of a browser for each browser test is costly. That's why Geb has a WebDriver instance caching mechanism which allows to save time by using a single instance of the driver and hence the browser across all of the tests. The browser state (cookies and web storage) is also managed to prevent it bleeding from one test to another.

BrowserWebDriverContainer comes with a very neat support for recording the screen of the driven browser. Unfortunately it seems to be implemented in a way which assumes that both the browser and vnc recording containers will only be used for a single test method and then discarded. In geb/issues#630 we've been discussing with @leonard84 that it would be good to make it easy to use the screen recording support provided by Testcontainers from Geb. To be able to achieve that we would need to make it possible to capture separate screen recordings for multiple tests using a single BrowserWebDriverContainer instance. I could probably come up with some hacks or workaround for this problem but I believe that in the interest of users who might be familiar with BrowserWebDriverContainer and myself who would need to then maintain such hack/workaround it's best if that's actually added to Testcontainers.

I would like to propose adding a mode to BrowserWebDriverContainer which would restart the vnc container every time afterTest() is called on it. I'm willing to work on a PR with the necessary changes but before I do I would like to ask if such a contribution has a chance of being accepted. I can see this being somewhat against the design philosophy of Testcontainers where GenericContainer implements org.junit.rules.TestRule in a way where the container is started prior to the statement (single test case or the whole class depending on the rule annotation used) being executed and stopped afterwards. Therefore I'm asking this question upfront to avoid wasting my time and effort if such a change is unlikely to be accepted.

erdi avatar Apr 14 '21 20:04 erdi

Hey @erdi,

Have you considered doing something like this? https://github.com/testcontainers/testcontainers-java/issues/320#issuecomment-361310904

bsideup avatar Apr 15 '21 20:04 bsideup

@bsideup while that works I don't think it is a very elegant solution, maybe it is the best for junit4, but newer frameworks allow better extensions. IMHO it would make sense to give the BrowserWebDriverContainer the necessary functionality to start a fresh recording. Instead of forcing every user to wire it up again.

leonard84 avatar Apr 15 '21 20:04 leonard84

I think adding this behavior to afterTest() of BrowserWebDriverContainer should indeed work without breaking any existing functionality. The actual implementation could also be similar to https://github.com/testcontainers/testcontainers-java/issues/320#issuecomment-361310904 (e.g. just restart the VncContainer part).

kiview avatar Jul 02 '21 15:07 kiview

I have been running into the same issue and acknowledge that #320 kind of can solve the issue. However, this requires that quite some stuff needs to be reimplemented like org.testcontainers.containers.BrowserWebDriverContainer#retainRecordingIfNeeded to get proper handling of recordingMode, vncRecordingDirectory, etc. and retain the same functions as with the current implementation.

How about having either the vncRecordingContainer as protected property so afterTest can handle all of that or a method like

reinitializeVncRecorderContainer() {
  vncRecorderContainer.stop();
  vncRecordingContainer =
                new VncRecordingContainer(this)
                    .withVncPassword(DEFAULT_PASSWORD)
                    .withVncPort(VNC_PORT)
                    .withVideoFormat(recordingFormat);
  vncRecorderContainer.start();
}

I think this is just a bit of refactoring that won't break any existing solutions.

MartinAhrer avatar May 13 '24 08:05 MartinAhrer