Add "VncRecordingContainer" #526

bsideup · 2017-12-17T20:40:57Z

Network-based
Attachable
supports remote Docker instances (doesn't use the volumes)

…tion of VncRecordingSidekickContainer

bsideup · 2017-12-17T20:46:16Z

core/src/main/java/org/testcontainers/containers/Network.java

@@ -16,6 +16,8 @@

 public interface Network extends AutoCloseable, TestRule {

+    Network SHARED = newNetwork();


since "network alias" functionality requires Network to be set, I created a SHARED network for different use cases (to replace the links)

Cool, we might be able to use it for a more modern Docker-Compose implementation as well?

bsideup · 2017-12-17T20:46:54Z

core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java

+
+    private int vncPort = 5900;
+
+    private int frameRate = 30;


thought it's nice to have it configurable :) Some people do crazy stuff during their tests :D

bsideup · 2017-12-17T20:47:09Z

core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java

+                        };
+
+                        try (
+                                Closeable __ = dockerClient.logContainerCmd(containerId)


__ == unused

Starting with Java 9, underscores won't be legal variable names (Java Language Changes for Java SE 9) (okay, maybe double underscore is fine 😜 )

bsideup · 2017-12-17T20:48:31Z

core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java

+        this.targetNetworkAlias = targetNetworkAlias;
+        withNetwork(network);
+
+        waitingFor(new AbstractWaitStrategy() {


Our standard LogWaitStrategy didn't work here and also requires a Regexp while here contains is more than enough. Also, in debug mode vncrec.py produces a lot of output, makes sense to do as little as possible here

Perhaps we could/should ship a log wait strategy that is based on contains anyway. It seems to me that LogWaitStrategy is used for contains semantics more often than for a full blown regex.

Not saying we have to do that now, but keen for your thoughts.

I see your point not needing a Regex, but why didn't LogWaitStrategy work?

bsideup · 2017-12-17T20:51:07Z

core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java

+    }
+
+    @SneakyThrows
+    public InputStream streamRecord() {


the whole point is having "volume-less" recorder because:

for some reason, on Mac, files were incomplete (read - empty) once a recording is flushed to the FS ( See VNC recorder for selenium container creates empty video files #466 )

allows Docker to be on another host. Not-so-critical for the integration testing, but I've received some feedback about CI setups for functional/e2e testing, people really run their Docker hosts "somewhere else"

2 is definitely an awesome reason to do this 👍

bsideup · 2017-12-17T20:52:43Z

modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java

        }

        driver = Unreliables.retryUntilSuccess(30, TimeUnit.SECONDS,
-                Timeouts.getWithTimeout(10, TimeUnit.SECONDS,
+                Timeouts.getWithTimeout(1, TimeUnit.SECONDS,


Not directly related to this PR, but sometimes it fails to init, and this code waits for 10s while 1s is enough

bsideup · 2017-12-17T20:54:55Z

...s/selenium/src/test/java/org/testcontainers/junit/ChromeRecordingWebDriverContainerTest.java

-/**
- *
- */
+@RunWith(Enclosed.class)


had to replace with Enclosed - was using this class for testing, but it was starting 2 containers every time, while only one of them is used to actually run the test

Sounds sensible.

rnorth · 2017-12-18T13:39:08Z

core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java

+
+    private int frameRate = 30;
+
+    public VncRecordingContainer(@NonNull GenericContainer<?> targetContainer) throws IllegalStateException {


Do we need to explicitly declare IllegalStateException as thrown?

no, nice catch, a leftover from my previous experiments :)

rnorth · 2017-12-18T13:50:12Z

core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java

+                targetContainer.getNetwork(),
+                targetContainer.getNetworkAliases().stream()
+                        .findFirst()
+                        .orElseThrow(() -> new IllegalStateException("Target container must have a network alias"))


Maybe we could make this less likely to happen by always creating a default (random?) alias for a container whenever you join it to a network?

Does this already happen / would it cause problems if we did it?

if container already started, we can't modify its aliases I think

actually, we CAN alias during connect:
https://docs.docker.com/engine/reference/commandline/network_connect/#description

I'll check if it's possible with docker-java, will make users' life much easier

rnorth

This is really good - thanks. I have a few comments/questions but nothing that I think would get in the way of merging.

kiview · 2017-12-18T18:48:26Z

core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java

+        this.targetNetworkAlias = targetNetworkAlias;
+        withNetwork(network);
+
+        waitingFor(new AbstractWaitStrategy() {


I see your point not needing a Regex, but why didn't LogWaitStrategy work?

kiview · 2017-12-18T18:52:31Z

core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java

+                        };
+
+                        try (
+                                Closeable __ = dockerClient.logContainerCmd(containerId)


Starting with Java 9, underscores won't be legal variable names (Java Language Changes for Java SE 9) (okay, maybe double underscore is fine 😜 )

kiview · 2017-12-18T18:58:28Z

modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java

    }

    private void stopAndRetainRecordingForDescriptionAndSuccessState(Description description, boolean succeeded) {
-        File recordingFile = recordingFileFactory.recordingFileForTest(vncRecordingDirectory, description, succeeded);
-        LOGGER.info("Screen recordings for test {} will be stored at: {}", description.getDisplayName(), recordingFile);
+        if (recordingMode == VncRecordingMode.RECORD_ALL || (!succeeded && recordingMode == VncRecordingMode.RECORD_FAILING)) {


I would prefer a variable for (!succeeded && recordingMode == VncRecordingMode.RECORD_FAILING), since I initially needed some time to parse this boolean expression 😉
Maybe recordFailingTest?

Good idea 👍

kiview · 2017-12-18T19:00:45Z

core/src/main/java/org/testcontainers/containers/Network.java

@@ -16,6 +16,8 @@

 public interface Network extends AutoCloseable, TestRule {

+    Network SHARED = newNetwork();


Cool, we might be able to use it for a more modern Docker-Compose implementation as well?

kiview · 2017-12-18T19:06:59Z

modules/selenium/src/test/java/org/testcontainers/junit/BaseWebDriverContainerTest.java

@@ -37,6 +38,9 @@ private static RemoteWebDriver setupDriverFromRule(BrowserWebDriverContainer rul
    protected static void doSimpleExplore(BrowserWebDriverContainer rule) {
        RemoteWebDriver driver = setupDriverFromRule(rule);
        driver.get("http://en.wikipedia.org/wiki/Randomness");
+
+        // Oh! The irony!
+        assertTrue("Randomness' description has the word 'pattern'", driver.findElementByPartialLinkText("pattern").isDisplayed());


Leading to Wiki authors breaking our tests 🤣

rnorth · 2017-12-18T19:20:37Z

core/src/main/java/org/testcontainers/containers/VncRecordingContainer.java

+    }
+
+    @SneakyThrows
+    public InputStream streamRecord() {


Just a wording thing, but please could this be streamRecording? The same for saveRecordToFile (->saveRecordingToFile)

…rdingContainer, make logic it stopAndRetainRecordingForDescriptionAndSuccessState more explicit, streamRecord -> streamRecording

kiview · 2017-12-18T20:50:17Z

core/src/main/java/org/testcontainers/containers/Network.java

+    Network SHARED = new NetworkImpl(false, null, Collections.emptySet(), null) {
+        @Override
+        public void close() {
+            // Do not avoid users to close SHARED network, only ResourceReaper is allowed to close (destroy) it


I think you meant allow instead of avoid.

Bumps [influxdb-java](https://github.com/influxdata/influxdb-java) from 2.10 to 2.14. <details> <summary>Changelog</summary> *Sourced from [influxdb-java's changelog](https://github.com/influxdata/influxdb-java/blob/master/CHANGELOG.md).* > ## 2.14 [2018-10-12] > > ### Fixes > > - Fixed chunked query exception handling [Issue #523](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/523) > - Memory leak in StringBuilder cache for Point.lineprotocol() [Issue #526](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/521) > > ## 2.13 [2018-09-12] > > ### Fixes > - MessagePack queries: Exception during parsing InfluxDB version [macOS] [PR #487](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/487) > - The InfluxDBResultMapper is able to handle results with a different time precision [PR #501](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/501) > - UDP target host address is cached [PR #502](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/502) > - Error messages from server not parsed correctly when using msgpack [PR #506](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/506) > - Response body must be closed properly in case of JSON response [PR #514](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/514) > - Time is serialized not consistently in MsgPack and Json, missing millis and nanos in MsgPack[PR #517](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/517) > > ### Features > > - Support for Basic Authentication [PR #492](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/492) > - Added possibility to reuse client as a core part of [influxdb-java-reactive](https://github.com/bonitoo-io/influxdb-java-reactive) client [PR #493](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/493) > - Retry capability for writing of BatchPoints [PR #503](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/503) > - Added `BiConsumer` with capability to discontinue a streaming query [Issue #515](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/515) > - Added `onComplete` action that is invoked after successfully end of streaming query [Issue #515](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/515) > > ## 2.12 [2018-07-31] > > ### Fixes > > - Remove code which checks for unsupported influxdb versions [PR #474](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/474) > - Unpredictable errors when OkHttpClient.Builder instance is reused [PR #478](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/478) > > ### Features > > - Support for MessagePack [PR #471](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/471) > - Cache version per influxdb instance and reduce ping() calls for every query call [PR #472](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/472) > - FAQ list for influxdb-java [PR #475](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/475) > > ### Improvements > > - Test: Unit test to ensure tags should be sorted by key in line protocol (to reduce db server overheads) [PR #476](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/476) > > ## 2.11 [2018-07-02] > > ### Features > > - Allow write precision of TimeUnit other than Nanoseconds [PR #321](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/321) > - Support dynamic measurement name in InfluxDBResultMapper [PR #423](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/423) > - Debug mode which allows HTTP requests being sent to the database to be logged [PR #450](https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/450) > - Fix problem of connecting to the influx api with URL which does not points to the url root (e.g. localhots:80/influx-api/) [PR #400] (https://github-redirect.dependabot.com/influxdata/influxdb-java/pull/400) ></table> ... (truncated) </details> <details> <summary>Commits</summary> - [`91d0f09`](influxdata/influxdb-java@91d0f09) [maven-release-plugin] prepare release influxdb-java-2.14 - [`8ffaeb9`](influxdata/influxdb-java@8ffaeb9) Revert "[maven-release-plugin] prepare release influxdb-java-2.14" - [`2781da2`](influxdata/influxdb-java@2781da2) [maven-release-plugin] prepare release influxdb-java-2.14 - [`19c69ed`](influxdata/influxdb-java@19c69ed) [maven-release-plugin] prepare for next development iteration - [`c6d7f25`](influxdata/influxdb-java@c6d7f25) [maven-release-plugin] prepare release influxdb-java-2.14 - [`2f4c594`](influxdata/influxdb-java@2f4c594) Merge pull request [#531](https://github-redirect.dependabot.com/influxdata/influxdb-java/issues/531) from heshengbang/master - [`f653e62`](influxdata/influxdb-java@f653e62) Easy to use try-with-resources, add README.md - [`c7be9b0`](influxdata/influxdb-java@c7be9b0) Easy to use try-with-resources - [`4590d18`](influxdata/influxdb-java@4590d18) - added automated SNAPSHOT publishing to Maven Central repository - [`ce65a41`](influxdata/influxdb-java@ce65a41) - added automated SNAPSHOT publishing to Maven Central repository - Additional commits viewable in [compare view](influxdata/influxdb-java@influxdb-java-2.10...influxdb-java-2.14) </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=org.influxdb:influxdb-java&package-manager=gradle&previous-version=2.10&new-version=2.14)](https://dependabot.com/compatibility-score.html?dependency-name=org.influxdb:influxdb-java&package-manager=gradle&previous-version=2.10&new-version=2.14) 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>

Add "VncRecordingContainer" - Network-based, attachable re-implementa…

7f599e8

…tion of VncRecordingSidekickContainer

bsideup added the enhancement label Dec 17, 2017

bsideup requested review from rnorth and kiview December 17, 2017 20:40

bsideup commented Dec 17, 2017

View reviewed changes

rnorth reviewed Dec 18, 2017

View reviewed changes

fix leftover, add issue # to CHANGELOG.md

0020cba

rnorth reviewed Dec 18, 2017

View reviewed changes

rnorth approved these changes Dec 18, 2017

View reviewed changes

bsideup mentioned this pull request Dec 18, 2017

vnc-recorder is not able to write recordings Linux permissions? #518

Closed

kiview reviewed Dec 18, 2017

View reviewed changes

"docker network attach"-style VncRecordingContainer implementation

0f09dbe

kiview reviewed Dec 18, 2017

View reviewed changes

rnorth reviewed Dec 18, 2017

View reviewed changes

bsideup added 2 commits December 18, 2017 21:34

use SHARED network to limit the number of Networks created by VncReco…

fd409b9

…rdingContainer, make logic it stopAndRetainRecordingForDescriptionAndSuccessState more explicit, streamRecord -> streamRecording

cleanup the imports

176eafe

kiview reviewed Dec 18, 2017

View reviewed changes

bsideup added 5 commits December 18, 2017 21:51

fix typo

067f2d2

restore the timeout

d283f0e

revert "connect"-like implementation for now

bd5eb9b

fix networkAliases

1add62a

make Codacy happy

f0c6336

rnorth merged commit b7bd98b into master Dec 19, 2017

rnorth deleted the generic_vnc_recorder branch December 19, 2017 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "VncRecordingContainer" #526

Add "VncRecordingContainer" #526

bsideup commented Dec 17, 2017

bsideup Dec 17, 2017

kiview Dec 18, 2017

bsideup Dec 17, 2017

bsideup Dec 17, 2017

kiview Dec 18, 2017

bsideup Dec 17, 2017

rnorth Dec 18, 2017

kiview Dec 18, 2017

bsideup Dec 17, 2017

rnorth Dec 18, 2017

bsideup Dec 17, 2017

rnorth Dec 18, 2017

bsideup Dec 17, 2017

rnorth Dec 18, 2017

rnorth Dec 18, 2017

bsideup Dec 18, 2017

rnorth Dec 18, 2017

bsideup Dec 18, 2017

bsideup Dec 18, 2017 •

edited

Loading

rnorth left a comment

kiview Dec 18, 2017

kiview Dec 18, 2017

kiview Dec 18, 2017

rnorth Dec 18, 2017

kiview Dec 18, 2017

kiview Dec 18, 2017

rnorth Dec 18, 2017

kiview Dec 18, 2017

bsideup Dec 18, 2017

		@@ -16,6 +16,8 @@

		public interface Network extends AutoCloseable, TestRule {

		Network SHARED = newNetwork();


		private int frameRate = 30;

		public VncRecordingContainer(@NonNull GenericContainer<?> targetContainer) throws IllegalStateException {

Add "VncRecordingContainer" #526

Add "VncRecordingContainer" #526

Conversation

bsideup commented Dec 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsideup Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

rnorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsideup Dec 18, 2017 •

edited

Loading