-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add shading tests #390
Add shading tests #390
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,6 @@ | |
import java.util.Date; | ||
import java.util.concurrent.TimeUnit; | ||
|
||
import static com.google.common.base.Preconditions.checkState; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since it's the only place where we use Guava, I removed it from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 good idea - I thought it was more widely used but it's good to remove it! |
||
import static java.time.temporal.ChronoUnit.SECONDS; | ||
|
||
/** | ||
|
@@ -87,8 +86,11 @@ protected Integer getLivenessCheckPort() { | |
@Override | ||
protected void configure() { | ||
|
||
checkState(desiredCapabilities != null); | ||
if (! customImageNameIsSet) { | ||
if (desiredCapabilities == null) { | ||
throw new IllegalStateException(); | ||
} | ||
|
||
if (!customImageNameIsSet) { | ||
super.setDockerImageName(getImageForCapabilities(desiredCapabilities)); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<project xmlns="http://maven.apache.org/POM/4.0.0" | ||
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
<parent> | ||
<artifactId>shade-test</artifactId> | ||
<groupId>org.testcontainers</groupId> | ||
<version>0-SNAPSHOT</version> | ||
<relativePath>../pom.xml</relativePath> | ||
</parent> | ||
<modelVersion>4.0.0</modelVersion> | ||
|
||
<artifactId>jar-file</artifactId> | ||
|
||
<dependencies> | ||
<dependency> | ||
<groupId>org.assertj</groupId> | ||
<artifactId>assertj-core</artifactId> | ||
<version>3.6.2</version> | ||
<scope>test</scope> | ||
</dependency> | ||
</dependencies> | ||
|
||
</project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
package org.testcontainers; | ||
|
||
import org.assertj.core.api.ListAssert; | ||
import org.junit.*; | ||
|
||
import java.io.IOException; | ||
import java.net.URI; | ||
import java.nio.file.*; | ||
|
||
import static java.util.Collections.emptyMap; | ||
import static org.assertj.core.api.Assertions.assertThat; | ||
|
||
public class JarFileShadingTest { | ||
|
||
private static FileSystem fileSystem; | ||
|
||
private static Path root; | ||
|
||
@BeforeClass | ||
public static void setUp() throws Exception { | ||
Path path = Paths.get("..", "..", "core", "target", "testcontainers-0-SNAPSHOT.jar"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is prone to refactor of locations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's not very IDE-friendly I'm afraid. Test will fail if we refactor the locations => should be fine There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please elaborate why is it not IDE friendly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We run tests from IDE, and would like to keep that. Doing something in Maven means that we have to run the tests with Maven and only Maven. Personally, I would prefer to use Gradle and "delegate build to Gradle" feature of IntelliJ, but we use Maven ATM. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this test you rely on the fact that someone ran mvn install on the core project right? Once he has done that you can the test from the IDE. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then how does it matter where the jar file is? Again, the test will fail if we change paths. Doing "copy dependency" might lead to the dependency copied from Maven's local repo => more magical and error-prone There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's going to be pretty obvious why this has broken if someone ever manages to break it, so I think we'll be OK with this. |
||
|
||
fileSystem = FileSystems.newFileSystem(URI.create("jar:" + path.toUri()), emptyMap()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Really cool trick! |
||
|
||
root = fileSystem.getRootDirectories().iterator().next(); | ||
} | ||
|
||
@AfterClass | ||
public static void tearDown() throws Exception { | ||
fileSystem.close(); | ||
} | ||
|
||
@Test | ||
public void testPackages() throws Exception { | ||
assertThatFileList(root).containsOnly( | ||
"docker-java.properties", | ||
"org/", | ||
"META-INF/", | ||
"com/" | ||
); | ||
|
||
assertThatFileList(root.resolve("org")).containsOnly( | ||
"testcontainers/" | ||
); | ||
|
||
assertThatFileList(root.resolve("com")).containsOnly( | ||
"github/" | ||
); | ||
|
||
assertThatFileList(root.resolve("com").resolve("github")).containsOnly( | ||
"dockerjava/" | ||
); | ||
} | ||
|
||
@Test | ||
public void testMetaInf() throws Exception { | ||
assertThatFileList(root.resolve("META-INF")).containsOnly( | ||
"MANIFEST.MF", | ||
"services/", | ||
"native/" | ||
); | ||
|
||
assertThatFileList(root.resolve("META-INF").resolve("native")).containsOnly( | ||
"liborg-testcontainers-shaded-netty-transport-native-epoll.so" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Funny that we shade something which is architecture specific - I mean this might not work for Mac or Windows? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's not used where not supposed to be :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean? Docker Java Client doesn't use Netty communicate over TCP to Docker daemon? Does it do it using this native library or plain nio? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We choose different strategies on different platforms, also Netty has a mechanism where they optionally load epoll library where it's supported ( Linux ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing that this will have to change when merged with #393 and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
); | ||
} | ||
|
||
@Test | ||
public void testMetaInfServices() throws Exception { | ||
assertThatFileList(root.resolve("META-INF").resolve("services")) | ||
.allMatch(it -> it.startsWith("org.testcontainers.")); | ||
} | ||
|
||
private ListAssert<String> assertThatFileList(Path path) throws IOException { | ||
return (ListAssert) assertThat(Files.list(path)) | ||
.extracting(Path::getFileName) | ||
.extracting(Object::toString); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a common rule not to ship logging implementation with libraries. I have no idea why @docker-java does that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍