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.

Sign up for GitHub

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

Jump to bottom

Framework-agnostic container & test lifecycle #702

Merged
merged 3 commits into from
Jun 13, 2018
Merged

Framework-agnostic container & test lifecycle #702

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
246e8d8
Implement framework-agnostic container & test lifecycle
bsideup May 17, 2018
32305e9
Simpliy TestDescription interface
bsideup May 18, 2018
64fc9b3
{before,after}TestBlock -> {before,after}Test
bsideup Jun 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
39 changes: 32 additions & 7 deletions core/src/main/java/org/testcontainers/containers/DockerComposeContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import com.github.dockerjava.api.DockerClient;
import com.github.dockerjava.api.model.Container;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Joiner;
import com.google.common.base.Splitter;
import com.google.common.collect.Maps;
Expand All @@ -11,6 +10,7 @@
import org.apache.commons.lang.StringUtils;
import org.apache.commons.lang.SystemUtils;
import org.junit.runner.Description;
import org.junit.runners.model.Statement;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.slf4j.profiler.Profiler;
Expand All @@ -21,6 +21,7 @@
import org.testcontainers.containers.wait.strategy.Wait;
import org.testcontainers.containers.wait.strategy.WaitAllStrategy;
import org.testcontainers.containers.wait.strategy.WaitStrategy;
import org.testcontainers.lifecycle.Startable;
import org.testcontainers.utility.*;
import org.zeroturnaround.exec.InvalidExitValueException;
import org.zeroturnaround.exec.ProcessExecutor;
Expand Down Expand Up @@ -54,7 +55,7 @@
/**
* Container which launches Docker Compose, for the purposes of launching a defined set of containers.
*/
public class DockerComposeContainer<SELF extends DockerComposeContainer<SELF>> extends FailureDetectingExternalResource {
public class DockerComposeContainer<SELF extends DockerComposeContainer<SELF>> extends FailureDetectingExternalResource implements Startable {

/**
* Random identifier which will become part of spawned containers names, so we can shut them down
Expand Down Expand Up @@ -115,8 +116,35 @@ public DockerComposeContainer(String identifier, List<File> composeFiles) {
}

@Override
@VisibleForTesting
@Deprecated
public Statement apply(Statement base, Description description) {
return super.apply(base, description);
}

@Override
@Deprecated
public void starting(Description description) {
start();
}

@Override
@Deprecated
protected void succeeded(Description description) {
}

@Override
@Deprecated
protected void failed(Throwable e, Description description) {
}

@Override
@Deprecated
public void finished(Description description) {
stop();
}

@Override
public void start() {
final Profiler profiler = new Profiler("Docker Compose container rule");
profiler.setLogger(logger());
profiler.start("Docker Compose container startup");
Expand Down Expand Up @@ -227,10 +255,7 @@ private Logger logger() {
}

@Override
@VisibleForTesting
public void finished(Description description) {


public void stop() {
synchronized (MUTEX) {
try {
// shut down the ambassador container
Expand Down
93 changes: 80 additions & 13 deletions core/src/main/java/org/testcontainers/containers/GenericContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.junit.runner.Description;
import org.junit.runners.model.Statement;
import org.rnorth.ducttape.ratelimits.RateLimiter;
import org.rnorth.ducttape.ratelimits.RateLimiterBuilder;
import org.rnorth.ducttape.unreliables.Unreliables;
Expand All @@ -38,6 +39,9 @@
import org.testcontainers.containers.wait.WaitStrategy;
import org.testcontainers.containers.wait.strategy.WaitStrategyTarget;
import org.testcontainers.images.RemoteDockerImage;
import org.testcontainers.lifecycle.Startable;
import org.testcontainers.lifecycle.TestLifecycleAware;
import org.testcontainers.lifecycle.TestDescription;
import org.testcontainers.utility.*;

import java.io.File;
Expand Down Expand Up @@ -75,7 +79,7 @@
@EqualsAndHashCode(callSuper = false)
public class GenericContainer<SELF extends GenericContainer<SELF>>
extends FailureDetectingExternalResource
implements Container<SELF>, AutoCloseable, WaitStrategyTarget {
implements Container<SELF>, AutoCloseable, WaitStrategyTarget, Startable {

private static final Charset UTF8 = Charset.forName("UTF-8");

Expand Down Expand Up @@ -192,7 +196,15 @@ public GenericContainer(@NonNull final Future<String> image) {
/**
* Starts the container using docker, pulling an image if necessary.
*/
@Override
public void start() {
if (containerId != null) {
return;
}
doStart();
}

protected void doStart() {
Profiler profiler = new Profiler("Container startup");
profiler.setLogger(logger());

Expand Down Expand Up @@ -285,21 +297,27 @@ private void tryStart(Profiler profiler) {
/**
* Stops the container.
*/
@Override
public void stop() {

if (containerId == null) {
return;
}

String imageName;

try {
imageName = image.get();
} catch (Exception e) {
imageName = "<unknown>";
}
String imageName;

try {
imageName = image.get();
} catch (Exception e) {
imageName = "<unknown>";
Copy link
Member

Choose a reason for hiding this comment

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

I see it's just copied from the old implementation, but does this acutally happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, to give you a bit of history here:

When I was implementing https://github.com/testcontainers/testcontainers-java/pull/106/files, we wanted to keep the backward compatibility as much as possible, and one of such is Logger based on image's name :)

}

ResourceReaper.instance().stopAndRemoveContainer(containerId, imageName);
ResourceReaper.instance().stopAndRemoveContainer(containerId, imageName);
} finally {
containerId = null;
containerInfo = null;
}
}

/**
Expand Down Expand Up @@ -633,12 +651,66 @@ public void addExposedPorts(int... ports) {
}
}

private TestDescription toDescription(Description description) {
return new TestDescription() {
@Override
public String getTestId() {
return getDisplayName();
}

@Override
public String getDisplayName() {
return description.getDisplayName();
}

@Override
public Optional<String[]> getNameParts() {
return Optional.of(new String[]{
description.getClassName(),
description.getMethodName()
});
}

@Override
public Optional<String> getFilesystemFriendlyName() {
return getNameParts().map(it -> String.join("-", it));
}
};
}

@Override
@Deprecated
public Statement apply(Statement base, Description description) {
return super.apply(base, description);
}

@Override
@Deprecated
protected void starting(Description description) {
if (this instanceof TestLifecycleAware) {
((TestLifecycleAware) this).beforeTestBlock(toDescription(description));
}
this.start();
}

@Override
@Deprecated
protected void succeeded(Description description) {
if (this instanceof TestLifecycleAware) {
((TestLifecycleAware) this).afterTestBlock(toDescription(description), Optional.empty());
}
}

@Override
@Deprecated
protected void failed(Throwable e, Description description) {
if (this instanceof TestLifecycleAware) {
((TestLifecycleAware) this).afterTestBlock(toDescription(description), Optional.of(e));
}
}

@Override
@Deprecated
protected void finished(Description description) {
this.stop();
}
Expand Down Expand Up @@ -961,11 +1033,6 @@ public SELF withStartupAttempts(int attempts) {
return self();
}

@Override
public void close() {
stop();
}

/**
* Allow low level modifications of {@link CreateContainerCmd} after it was pre-configured in {@link #tryStart(Profiler)}.
* Invocation happens eagerly on a moment when container is created.
Expand Down
13 changes: 13 additions & 0 deletions core/src/main/java/org/testcontainers/lifecycle/Startable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package org.testcontainers.lifecycle;

public interface Startable extends AutoCloseable {
Copy link
Member

Choose a reason for hiding this comment

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

Why to we need this interface in this PR? It seems it's not used anyhwere (besides being implemented).
To use it in the testframework specific implementation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it defines a unified set of methods for both GenericContainer and DockerComposeContainer


void start();

void stop();

@Override
default void close() {
stop();
}
}
20 changes: 20 additions & 0 deletions core/src/main/java/org/testcontainers/lifecycle/TestDescription.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package org.testcontainers.lifecycle;

import java.util.Optional;

public interface TestDescription {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we need all methods as the public interface.
The way BrowserWebDriverContainer works with it is kind of awkward, implementing the fallback if getFilesystemFriendlyName() is not implemented itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

What would you suggest? :)

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to not have getFilesystemFriendlyName in here, since it seems oddly use case specific?

Or if we want to make it convenient, we could maybe have the default implementation here?
return getNameParts().map(it -> String.join("-", it));
And then the default implementation of getNameParts() would also become

return Optional.of(new String[]{
                    getTestId()
                });

Copy link
Member Author

Choose a reason for hiding this comment

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

oddly use case specific

But we have this use case at least in one place.

default implementation

I don't think this is a good default implementation to be honest, I would rather keep it explicit and let framework adapters think about it, what's the best way to provide a FS friendly name of the test

Copy link
Member

Choose a reason for hiding this comment

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

What about omitting the default implementation completely then?

Copy link
Member Author

Choose a reason for hiding this comment

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

fine by me :)


String getTestId();

default String getDisplayName() {
return getTestId();
}

default Optional<String[]> getNameParts() {
return Optional.empty();
}

default Optional<String> getFilesystemFriendlyName() {
return Optional.empty();
}
}
14 changes: 14 additions & 0 deletions core/src/main/java/org/testcontainers/lifecycle/TestLifecycleAware.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package org.testcontainers.lifecycle;

import java.util.Optional;

public interface TestLifecycleAware {

default void beforeTestBlock(TestDescription description) {
Copy link
Member

Choose a reason for hiding this comment

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

Why beforeTestBlock and not beforeTest? What are the semantics of block?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just in case if some Test frameworks define Test as something more bigger and an atomic execution unit is some block of code. Arguable :)

Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer removing block in order to remove the ambiguity, but your explanation is also fine, so that's a IMO thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, I summon @rnorth as a tiebreaker 😄


}

default void afterTestBlock(TestDescription description, Optional<Throwable> throwable) {

}
}
4 changes: 2 additions & 2 deletions modules/kafka/src/main/java/org/testcontainers/containers/KafkaContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public String getBootstrapServers() {
}

@Override
public void start() {
protected void doStart() {
String networkAlias = getNetworkAliases().get(0);
proxy = new SocatContainer()
.withNetwork(getNetwork())
Expand All @@ -77,7 +77,7 @@ public void start() {
withCommand("sh", "-c", "zookeeper-server-start /zookeeper.properties & /etc/confluent/docker/run");
}

super.start();
super.doStart();
}

@Override
Expand Down
28 changes: 16 additions & 12 deletions modules/selenium/src/main/java/org/testcontainers/containers/BrowserWebDriverContainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import com.google.common.collect.ImmutableSet;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.junit.runner.Description;
import org.openqa.selenium.remote.BrowserType;
import org.openqa.selenium.remote.DesiredCapabilities;
import org.openqa.selenium.remote.RemoteWebDriver;
Expand All @@ -18,11 +17,14 @@
import org.testcontainers.containers.wait.LogMessageWaitStrategy;
import org.testcontainers.containers.wait.WaitAllStrategy;
import org.testcontainers.containers.wait.WaitStrategy;
import org.testcontainers.lifecycle.TestDescription;
import org.testcontainers.lifecycle.TestLifecycleAware;

import java.io.File;
import java.net.MalformedURLException;
import java.net.URL;
import java.time.Duration;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.TimeUnit;

Expand All @@ -33,7 +35,7 @@
* <p>
* The container should expose Selenium remote control protocol and VNC.
*/
public class BrowserWebDriverContainer<SELF extends BrowserWebDriverContainer<SELF>> extends GenericContainer<SELF> implements VncService, LinkableContainer {
public class BrowserWebDriverContainer<SELF extends BrowserWebDriverContainer<SELF>> extends GenericContainer<SELF> implements VncService, LinkableContainer, TestLifecycleAware {

private static final String CHROME_IMAGE = "selenium/standalone-chrome-debug:%s";
private static final String FIREFOX_IMAGE = "selenium/standalone-firefox-debug:%s";
Expand Down Expand Up @@ -203,13 +205,15 @@ public RemoteWebDriver getWebDriver() {
}

@Override
protected void failed(Throwable e, Description description) {
stopAndRetainRecordingForDescriptionAndSuccessState(description, false);
}

@Override
protected void succeeded(Description description) {
stopAndRetainRecordingForDescriptionAndSuccessState(description, true);
public void afterTestBlock(TestDescription description, Optional<Throwable> throwable) {
retainRecordingIfNeeded(
description.getFilesystemFriendlyName().orElseGet(() ->
description.getNameParts()
.map(it -> String.join("-", it))
.orElse(description.getDisplayName())
),
throwable.isPresent()
);
}

@Override
Expand All @@ -233,7 +237,7 @@ public void stop() {
super.stop();
}

private void stopAndRetainRecordingForDescriptionAndSuccessState(Description description, boolean succeeded) {
private void retainRecordingIfNeeded(String prefix, boolean succeeded) {
final boolean shouldRecord;
switch (recordingMode) {
case RECORD_ALL:
Expand All @@ -248,8 +252,8 @@ private void stopAndRetainRecordingForDescriptionAndSuccessState(Description des
}

if (shouldRecord) {
File recordingFile = recordingFileFactory.recordingFileForTest(vncRecordingDirectory, description, succeeded);
LOGGER.info("Screen recordings for test {} will be stored at: {}", description.getDisplayName(), recordingFile);
File recordingFile = recordingFileFactory.recordingFileForTest(vncRecordingDirectory, prefix, succeeded);
LOGGER.info("Screen recordings for test {} will be stored at: {}", prefix, recordingFile);

vncRecordingContainer.saveRecordingToFile(recordingFile);
}
Expand Down