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

Make default file recording directory of BrowserWebDriverContainer platform independent #2562

Merged
merged 4 commits into from Apr 13, 2020

Conversation

kiview
Copy link
Member

@kiview kiview commented Apr 13, 2020

Fixes incompatibility of Windows with default recording directory, makes Windows work OOTB.

Not 100% sure how to best handle the IOException in the constructor.

@@ -79,6 +82,14 @@ public BrowserWebDriverContainer() {
.withStrategy(new HostPortWaitStrategy())
.withStartupTimeout(Duration.of(15, SECONDS));

try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should move it to configure and create the dir only if recording is enabled

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the code.

@bsideup
Copy link
Member

bsideup commented Apr 13, 2020

/azp run "Windows 10 - Docker for Windows"

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@bsideup
Copy link
Member

bsideup commented Apr 13, 2020

/azp run Windows 10 - Docker for Windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kiview
Copy link
Member Author

kiview commented Apr 13, 2020

/azp run Windows 10 - Docker for Windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@@ -52,6 +54,7 @@
private static final int VNC_PORT = 5900;

private static final String NO_PROXY_KEY = "no_proxy";
public static final String TC_TEMP_DIR_PREFIX = "tc";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Private?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, sorry.

@bsideup
Copy link
Member

bsideup commented Apr 13, 2020

/azp run Windows 10 - Docker for Windows

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kiview kiview merged commit d33f647 into master Apr 13, 2020
@delete-merged-branch delete-merged-branch bot deleted the vnc-windows-defaultdir-fix branch April 13, 2020 15:39
@rnorth
Copy link
Member

rnorth commented Apr 13, 2020

This was released in https://github.com/testcontainers/testcontainers-java/releases/tag/1.14.0 🎉

Thanks for the contribution!

quincy pushed a commit to quincy/testcontainers-java that referenced this pull request May 28, 2020
…atform independent (testcontainers#2562)

Co-authored-by: Sergei Egorov <bsideup@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants