Add shading tests #390

bsideup · 2017-07-02T12:41:03Z

Shading is a hot topic, and usually painful :D

I added a few tests of our final jar to make sure that our shading configuration is correct.

Also, during this PR, I revisited a few dependencies, removed shading from jdbc module (no idea why it was there :D)

N.B! run mvn -DskipTests -pl core clean package before running the test from IDE

bsideup · 2017-07-02T12:42:32Z

core/pom.xml

@@ -34,6 +34,10 @@
                    <groupId>org.glassfish.jersey.connectors</groupId>
                    <artifactId>*</artifactId>
                </exclusion>
+                <exclusion>
+                    <groupId>log4j</groupId>


it's a common rule not to ship logging implementation with libraries. I have no idea why @docker-java does that.

bsideup · 2017-07-02T12:43:44Z

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

@@ -24,7 +24,6 @@
 import java.util.Date;
 import java.util.concurrent.TimeUnit;

-import static com.google.common.base.Preconditions.checkState;


since it's the only place where we use Guava, I removed it from selenium module and inlined the implementation of checkState, so that we don't have to shade guava into the module

👍 good idea - I thought it was more widely used but it's good to remove it!

kiview

LGTM

asafm

Very good addition

asafm · 2017-07-06T08:58:11Z

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

@@ -87,8 +86,11 @@ protected Integer getLivenessCheckPort() {
    @Override
    protected void configure() {

-        checkState(desiredCapabilities != null);
-        if (! customImageNameIsSet) {
+        if(desiredCapabilities == null) {


if (desiredCapabilities == null) {

Added space between if and (

ok, I see. Will fix it now

asafm · 2017-07-06T09:01:54Z

shade-test/jar-file/src/test/java/org/testcontainers/JarFileShadingTest.java

+
+    @BeforeClass
+    public static void setUp() throws Exception {
+        Path path = Paths.get("..", "..", "core", "target", "testcontainers-0-SNAPSHOT.jar");


This is prone to refactor of locations.
How about you copy this jar to this project target directory using copy-dependencies goal of maven-dependencies-plugin? You can add it in the pom.xml of this project.

it's not very IDE-friendly I'm afraid.

Test will fail if we refactor the locations => should be fine

Can you please elaborate why is it not IDE friendly?
I though one of Maven projects best practice is to avoid depending on explicit locations, but depends on artifacts. When you use copy dependencies you alleviate your self from that since you use GAV and not relative paths. You are free then to place core where ever you want.

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.

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.
I'm saying the same thing - someone must run mvn install on the core project and on the project it self. Then you run the test from the IDE - same.
So both rely on manual mvn operation.
Am I missing something here?

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

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.

asafm · 2017-07-06T09:02:39Z

shade-test/jar-file/src/test/java/org/testcontainers/JarFileShadingTest.java

+    public static void setUp() throws Exception {
+        Path path = Paths.get("..", "..", "core", "target", "testcontainers-0-SNAPSHOT.jar");
+
+        fileSystem = FileSystems.newFileSystem(URI.create("jar:" + path.toUri()), emptyMap());


Really cool trick!

asafm · 2017-07-06T09:05:54Z

shade-test/jar-file/src/test/java/org/testcontainers/JarFileShadingTest.java

+        );
+
+        assertThatFileList(root.resolve("META-INF").resolve("native")).containsOnly(
+                "liborg-testcontainers-shaded-netty-transport-native-epoll.so"


Funny that we shade something which is architecture specific - I mean this might not work for Mac or Windows?

it's not used where not supposed to be :)

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?

We choose different strategies on different platforms, also Netty has a mechanism where they optionally load epoll library where it's supported ( Linux )

I'm guessing that this will have to change when merged with #393 and netty-transport-native-kqueue comes in. NBD though!

@rnorth yeah, I'll rebase #393 after we merge this one :)

rnorth

This looks great - awesome work! This should help fix real pain for users, and will help us systematically solve future shading issues as well. +💯!

rnorth · 2017-07-06T20:23:57Z

shade-test/jar-file/src/test/java/org/testcontainers/JarFileShadingTest.java

+
+    @BeforeClass
+    public static void setUp() throws Exception {
+        Path path = Paths.get("..", "..", "core", "target", "testcontainers-0-SNAPSHOT.jar");


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.

rnorth · 2017-07-06T20:34:51Z

AFAICT you've resolved all discussions with @asafm so, from my perspective, this should be good to merge!

bsideup added 4 commits July 2, 2017 13:45

Add shading tests

d5a7c6a

put back docker-java

0d1cfc2

fix test

13e3dda

do not shade jdbc module

c195f25

bsideup added the enhancement label Jul 2, 2017

bsideup added this to the 1.4.0 milestone Jul 2, 2017

bsideup requested review from rnorth and kiview July 2, 2017 12:41

bsideup commented Jul 2, 2017

View reviewed changes

bsideup mentioned this pull request Jul 2, 2017

Public API tests #391

Merged

kiview approved these changes Jul 6, 2017

View reviewed changes

asafm reviewed Jul 6, 2017

View reviewed changes

fix missing space

4372a8a

rnorth approved these changes Jul 6, 2017

View reviewed changes

bsideup merged commit a32e980 into master Jul 6, 2017

bsideup deleted the add_shading_tests branch July 6, 2017 20:51

bsideup mentioned this pull request Jul 10, 2017

Fix Guava shading in jdbc module #399

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add shading tests #390

Add shading tests #390

bsideup commented Jul 2, 2017

bsideup Jul 2, 2017 •

edited

Loading

rnorth Jul 2, 2017

bsideup Jul 2, 2017

rnorth Jul 2, 2017

kiview left a comment

asafm left a comment

asafm Jul 6, 2017

bsideup Jul 6, 2017

asafm Jul 6, 2017

bsideup Jul 6, 2017

asafm Jul 6, 2017

bsideup Jul 6, 2017

asafm Jul 6, 2017

bsideup Jul 6, 2017

asafm Jul 6, 2017

bsideup Jul 6, 2017

rnorth Jul 6, 2017

asafm Jul 6, 2017

asafm Jul 6, 2017

bsideup Jul 6, 2017

asafm Jul 6, 2017

bsideup Jul 6, 2017

rnorth Jul 6, 2017

bsideup Jul 6, 2017

rnorth left a comment

rnorth Jul 6, 2017

rnorth commented Jul 6, 2017

Add shading tests #390

Add shading tests #390

Conversation

bsideup commented Jul 2, 2017

bsideup Jul 2, 2017 • edited Loading

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

kiview left a comment

Choose a reason for hiding this comment

asafm 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

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

rnorth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnorth commented Jul 6, 2017

bsideup Jul 2, 2017 •

edited

Loading