Add "VncRecordingContainer" by bsideup · Pull Request #526 · testcontainers/testcontainers-java

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


 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


 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

        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 kiview and rnorth 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


		public interface Network extends AutoCloseable, TestRule {

		Network SHARED = newNetwork();


		private int frameRate = 30;

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

Uh oh!

Comments

Conversation

bsideup commented Dec 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsideup Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnorth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bsideup Dec 18, 2017 •

edited

Loading