Code review

atbashEE · Apr 3, 2023 · 5cb84ad · 5cb84ad
1 parent 14b3fe7
commit 5cb84ad
Show file tree

Hide file tree

Showing 16 changed files with 108 additions and 68 deletions.
diff --git a/...e/src/main/java/be/atbash/testing/integration/database/exception/DataScriptException.java b/...e/src/main/java/be/atbash/testing/integration/database/exception/DataScriptException.java
@@ -0,0 +1,24 @@
+/*
+ * Copyright 2022-2023 Rudy De Busscher (https://www.atbash.be)
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package be.atbash.testing.integration.database.exception;
+
+import org.junit.platform.commons.JUnitException;
+
+public class DataScriptException extends JUnitException {
+    public DataScriptException(String message, Throwable cause) {
+        super(message, cause);
+    }
+}
diff --git a/...c/main/java/be/atbash/testing/integration/database/exception/DatabaseScriptException.java b/...c/main/java/be/atbash/testing/integration/database/exception/DatabaseScriptException.java
@@ -0,0 +1,24 @@
+/*
+ * Copyright 2022-2023 Rudy De Busscher (https://www.atbash.be)
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package be.atbash.testing.integration.database.exception;
+
+import org.junit.platform.commons.JUnitException;
+
+public class DatabaseScriptException extends JUnitException {
+    public DatabaseScriptException(String message, Throwable cause) {
+        super(message, cause);
+    }
+}
diff --git a/database/src/main/java/be/atbash/testing/integration/database/image/JDBCDriverFileUtil.java b/database/src/main/java/be/atbash/testing/integration/database/image/JDBCDriverFileUtil.java
@@ -27,6 +27,9 @@
 
 public final class JDBCDriverFileUtil {
 
+    private JDBCDriverFileUtil() {
+    }
+
     public static String getDriverFile(TestContext testContext) {
         DatabaseContainerAdapterMetaData databaseMetaData = testContext.getInstance(DatabaseContainerAdapterMetaData.class);
 

diff --git a/...java/be/atbash/testing/integration/database/jupiter/DatabaseContainerAdapterMetaData.java b/...java/be/atbash/testing/integration/database/jupiter/DatabaseContainerAdapterMetaData.java
@@ -62,7 +62,7 @@ private static Class<?> checkClass(String className) {
         try {
             result = Class.forName(className);
         } catch (ClassNotFoundException e) {
-            ; // Just a test, lets continue
+            // Just a test, lets continue
         }
         return result;
     }

diff --git a/...java/be/atbash/testing/integration/database/jupiter/DatabaseTestcontainersController.java b/...java/be/atbash/testing/integration/database/jupiter/DatabaseTestcontainersController.java
@@ -18,6 +18,8 @@
 import be.atbash.testing.integration.container.AbstractIntegrationContainer;
 import be.atbash.testing.integration.container.exception.UnexpectedException;
 import be.atbash.testing.integration.container.image.TestContext;
+import be.atbash.testing.integration.database.exception.DataScriptException;
+import be.atbash.testing.integration.database.exception.DatabaseScriptException;
 import be.atbash.testing.integration.database.exception.FileNotFoundException;
 import be.atbash.testing.integration.jupiter.ContainerAdapterMetaData;
 import be.atbash.testing.integration.jupiter.TestcontainersController;
@@ -98,14 +100,7 @@ public void start() {
             manageStartDatabaseContainer(startSignal, failure);
 
             startContainers();
-            try {
-                boolean containerStarted = startSignal.await(1, TimeUnit.MINUTES);
-                if (!containerStarted) {
-                    throw new AssertionError("Test aborted since Database container does not start within a reasonable time");
-                }
-            } catch (InterruptedException e) {
-                throw new UnexpectedException("Database Container start interrupted", e);
-            }
+            awaitStartup(startSignal);
 
             // We can only access the Database container after it is started.
             // But this start() s part of beforeAll and thus before PostProcessTestInstance needs it.
@@ -119,7 +114,18 @@ public void start() {
             throw new AssertionError("Test aborted");
         }
 
-        LOGGER.info("All containers started in " + (System.currentTimeMillis() - start) + "ms");
+        LOGGER.info(String.format("All containers started in %s ms", System.currentTimeMillis() - start));
+    }
+
+    private static void awaitStartup(CountDownLatch startSignal) {
+        try {
+            boolean containerStarted = startSignal.await(1, TimeUnit.MINUTES);
+            if (!containerStarted) {
+                throw new AssertionError("Test aborted since Database container does not start within a reasonable time");
+            }
+        } catch (InterruptedException e) {
+            throw new UnexpectedException("Database Container start interrupted", e);
+        }
     }
 
     public void injectInstances(Object testInstance) {
@@ -131,7 +137,7 @@ public void injectInstances(Object testInstance) {
             databaseContainerField = testClass.getField("databaseContainer");
             databaseContainerField.set(null, jdbcDatabaseContainer);
         } catch (Exception e) {
-            e.printStackTrace();
+            throw new UnexpectedException("Unexpected exception happened during injection of istances in database test fields", e);
         }
     }
 
@@ -159,7 +165,7 @@ private void startAndPrepareDatabaseContainer(CountDownLatch startSignal, Failur
 
                 ScriptUtils.executeDatabaseScript(delegate, createTables, script);
             } catch (ScriptException e) {
-                throw new RuntimeException(e);
+                throw new DatabaseScriptException("failure during execution of script : " + script, e);
             }
 
             String dataFile = databaseContainerMetaData.getDatabaseContainerIntegrationTest().databaseScriptFiles().initData();
@@ -168,13 +174,9 @@ private void startAndPrepareDatabaseContainer(CountDownLatch startSignal, Failur
                 throw new FileNotFoundException(String.format("The file with name '%s' is not found on the class path", dataFile));
             }
 
-            try {
-                dataSet = new XlsDataSet(Paths.get(testDataFile.toURI()).toFile());
-            } catch (IOException | DataSetException | URISyntaxException e) {
-                throw new RuntimeException(e);
-            }
+            createDataSet(testDataFile);
 
-        } catch (Throwable e) {
+        } catch (Throwable e) {  // We really want to have all exceptions captured here for proper Thread handling.
             // Report to the calling context that there was a problem.
             // Since this method can be executed in a Thread, throwing an Exception is not enough.
             callback.failed();
@@ -185,13 +187,21 @@ private void startAndPrepareDatabaseContainer(CountDownLatch startSignal, Failur
         }
     }
 
+    private void createDataSet(URL testDataFile) {
+        try {
+            dataSet = new XlsDataSet(Paths.get(testDataFile.toURI()).toFile());
+        } catch (IOException | DataSetException | URISyntaxException e) {
+            throw new UnexpectedException("Unexpected exception happened during creation of Dataset from " + testDataFile, e);
+        }
+    }
+
     public void uploadData() {
         try {
 
             DatabaseOperation.CLEAN_INSERT.execute(connection, dataSet);
 
         } catch (DatabaseUnitException | SQLException e) {
-            throw new RuntimeException(e);
+            throw new DataScriptException("Exception during execution of data insert", e);
         }
     }
 
@@ -221,11 +231,13 @@ public void clearData() {
             DatabaseOperation.DELETE_ALL.execute(connection, dataSet);
 
         } catch (DatabaseUnitException | SQLException e) {
-            throw new RuntimeException(e);
+            throw new DataScriptException("Exception during execution of data delete", e);
+
         }
 
     }
 
+    @Override
     public void stopContainers() throws IllegalAccessException {
         super.stopContainers();
 

diff --git a/...abase/payara/src/test/java/be/atbash/test/integration/example/hello/DatabasePayaraIT.java b/...abase/payara/src/test/java/be/atbash/test/integration/example/hello/DatabasePayaraIT.java
@@ -16,7 +16,6 @@
 package be.atbash.test.integration.example.hello;
 
 import be.atbash.testing.integration.database.jupiter.DatabaseContainerIntegrationTest;
-import be.atbash.testing.integration.database.jupiter.DatabaseScriptFiles;
 import be.atbash.testing.integration.database.test.AbstractDatabaseContainerIntegrationTest;
 import be.atbash.testing.integration.jupiter.ContainerIntegrationTest;
 import be.atbash.testing.integration.jupiter.SupportedRuntime;

diff --git a/example/database/wildfly/pom.xml b/example/database/wildfly/pom.xml
@@ -37,7 +37,7 @@
         <dependency>
             <groupId>org.testcontainers</groupId>
             <artifactId>mysql</artifactId>
-            <version>1.17.5</version>
+            <version>${testcontainers.version}</version>
             <scope>test</scope>
         </dependency>
 

diff --git a/example/database/wildfly/src/test/resources/create-tables.sql b/example/database/wildfly/src/test/resources/create-tables.sql
@@ -1,19 +1,3 @@
---
--- Copyright 2022-2023 Rudy De Busscher (https://www.atbash.be)
---
--- Licensed under the Apache License, Version 2.0 (the "License");
--- you may not use this file except in compliance with the License.
--- You may obtain a copy of the License at
---
---     http://www.apache.org/licenses/LICENSE-2.0
---
--- Unless required by applicable law or agreed to in writing, software
--- distributed under the License is distributed on an "AS IS" BASIS,
--- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
--- See the License for the specific language governing permissions and
--- limitations under the License.
---
-
 CREATE TABLE Company (id BIGINT NOT NULL, name VARCHAR(255), PRIMARY KEY (id))
 CREATE TABLE Employee (EMPLOYEE_ID BIGINT NOT NULL, FAVORITE_COLOR VARCHAR(255), FIRST_NAME VARCHAR(255), GENDER VARCHAR(255), HIRE_DATE DATE, LAST_NAME VARCHAR(255), COMPANY_ID BIGINT, PRIMARY KEY (EMPLOYEE_ID))
 ALTER TABLE Employee ADD CONSTRAINT FK_Employee_COMPANY_ID FOREIGN KEY (COMPANY_ID) REFERENCES Company (id)

diff --git a/example/database/wildfly/src/test/resources/drop-tables.sql b/example/database/wildfly/src/test/resources/drop-tables.sql
@@ -1,19 +1,3 @@
---
--- Copyright 2022-2023 Rudy De Busscher (https://www.atbash.be)
---
--- Licensed under the Apache License, Version 2.0 (the "License");
--- you may not use this file except in compliance with the License.
--- You may obtain a copy of the License at
---
---     http://www.apache.org/licenses/LICENSE-2.0
---
--- Unless required by applicable law or agreed to in writing, software
--- distributed under the License is distributed on an "AS IS" BASIS,
--- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
--- See the License for the specific language governing permissions and
--- limitations under the License.
---
-
 ALTER TABLE Employee DROP FOREIGN KEY FK_Employee_COMPANY_ID
 DROP TABLE company
 DROP TABLE Employee

diff --git a/impl/src/main/java/be/atbash/testing/integration/container/AbstractIntegrationContainer.java b/impl/src/main/java/be/atbash/testing/integration/container/AbstractIntegrationContainer.java
@@ -38,13 +38,13 @@ public abstract class AbstractIntegrationContainer<SELF extends AbstractIntegrat
 
     private final boolean liveLogging;
 
-    public AbstractIntegrationContainer(Future<String> image, boolean liveLogging) {
+    protected AbstractIntegrationContainer(Future<String> image, boolean liveLogging) {
         super(image);
         this.liveLogging = liveLogging;
         setNetwork(Network.SHARED);
     }
 
-    public AbstractIntegrationContainer(DockerImageName dockerImageName) {
+    protected AbstractIntegrationContainer(DockerImageName dockerImageName) {
         // FIXME Is this constructor still needed. Since child constructors aren't used.
         super(dockerImageName);
         this.liveLogging = false;

diff --git a/.../src/main/java/be/atbash/testing/integration/container/exception/UnexpectedException.java b/.../src/main/java/be/atbash/testing/integration/container/exception/UnexpectedException.java
@@ -20,4 +20,8 @@ public class UnexpectedException extends RuntimeException {
     public UnexpectedException(String message, Throwable cause) {
         super(message, cause);
     }
+
+    public UnexpectedException(String message) {
+        super(message);
+    }
 }
diff --git a/impl/src/main/java/be/atbash/testing/integration/container/image/DockerImageProcessor.java b/impl/src/main/java/be/atbash/testing/integration/container/image/DockerImageProcessor.java
@@ -24,7 +24,11 @@
  * Is required for OpenLiberty but also used for the other runtimes as MountableFile with .withCopyToContainer() and
  * TestExecutionExceptionHandler don't go well together (Broken pipe when previous run failed)
  */
-public class DockerImageProcessor {
+public final class DockerImageProcessor {
+
+    private DockerImageProcessor() {
+    }
+
     public static ImageFromDockerfile getImage(SupportedRuntime supportedRuntime, ContainerAdapterMetaData metaData, TestContext testContext) {
         String version = System.getProperty("be.atbash.test.runtime.version", "");
         DockerImageProducer producer = retrieveProducer(supportedRuntime);

diff --git a/impl/src/main/java/be/atbash/testing/integration/container/image/DockerImageProducer.java b/impl/src/main/java/be/atbash/testing/integration/container/image/DockerImageProducer.java
@@ -43,7 +43,7 @@ public abstract class DockerImageProducer {
      * according the supported Runtime.  Some additional statements are added if a Docker file is supplied in
      * the location.
      *
-     * @param metaData
+     * @param metaData The metadata for the container.
      * @param version  The version if a default DockerFile is created
      * @return The DockerFile that will be used within the test.
      */
@@ -102,7 +102,7 @@ protected void copyLocationContentToTempFile(String source, Path tempDir) {
             try (Stream<Path> pathStream = Files.find(Path.of(source),
                     Integer.MAX_VALUE,
                     (filePath, fileAttr) -> fileAttr.isRegularFile()
-                            && !filePath.getFileName().endsWith("Dockerfile"))) {
+                            && !filePath.getFileName().endsWith(DOCKERFILE))) {
                 // For each of the found files, copy it to the temporary directory.
                 pathStream.forEach(p -> copyToTemp(source, p, tempDir));
             }

diff --git a/impl/src/main/java/be/atbash/testing/integration/jupiter/ContainerAdapterMetaData.java b/impl/src/main/java/be/atbash/testing/integration/jupiter/ContainerAdapterMetaData.java
@@ -150,7 +150,7 @@ private static String determineCustomBuildDirectory(CustomBuildFile customBuildF
     }
 
     public static int determinePort(SupportedRuntime supportedRuntime) {
-        int port = 0;
+        int port;
         switch (supportedRuntime) {
 
             case PAYARA_MICRO:
@@ -161,6 +161,8 @@ public static int determinePort(SupportedRuntime supportedRuntime) {
             case OPEN_LIBERTY:
                 port = 9080;
                 break;
+            default:
+                throw new UnexpectedException("No support for " + supportedRuntime);
         }
         return port;
     }
@@ -183,12 +185,11 @@ private static File findAppFile(String applicationLocation) {
 
         // Find a .war file in the target/ directories
         Set<File> matches = new HashSet<>(findAppFiles(dir));
-        if (matches.size() == 0) {
+        if (matches.isEmpty()) {
 
             try {
                 throw new IllegalStateException(String.format("No .war files found in folder (including sub folders) %s.", dir.getCanonicalPath()));
             } catch (IOException e) {
-
                 throw new UnexpectedException("Exception during File.getCanonicalPath()", e);
             }
         }

diff --git a/impl/src/main/java/be/atbash/testing/integration/jupiter/TestcontainersController.java b/impl/src/main/java/be/atbash/testing/integration/jupiter/TestcontainersController.java
@@ -139,7 +139,7 @@ public void start() {
 
         long start = System.currentTimeMillis();
         startContainers();
-        LOGGER.info("All containers started in " + (System.currentTimeMillis() - start) + "ms");
+        LOGGER.info(String.format("All containers started in %s ms" ,  System.currentTimeMillis() - start));
     }
 
     protected void startContainers() {

diff --git a/impl/src/main/java/be/atbash/testing/integration/test/ShowLogWhenFailedExceptionHandler.java b/impl/src/main/java/be/atbash/testing/integration/test/ShowLogWhenFailedExceptionHandler.java
@@ -1,5 +1,5 @@
 /*
- * Copyright 2022 Rudy De Busscher (https://www.atbash.be)
+ * Copyright 2022-2023 Rudy De Busscher (https://www.atbash.be)
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -41,9 +41,11 @@ public void handleTestExecutionException(ExtensionContext extensionContext, Thro
 
             AbstractIntegrationContainer<?> mainContainer = getMainContainer(extensionContext.getRequiredTestClass());
 
-            String logs = mainContainer.getLogs();
-            System.out.println("Container log");
-            System.out.println(logs);
+            if (mainContainer != null) {
+                String logs = mainContainer.getLogs();
+                System.out.println("Container log");
+                System.out.println(logs);
+            }
         }
         throw throwable;  // rethrow. We just wanted to output the container log.
     }
@@ -68,7 +70,6 @@ private AbstractIntegrationContainer<?> getMainContainer(Class<?> testClass) {
 
                 }
 
-
             } catch (IllegalArgumentException | IllegalAccessException e) {
                 LOGGER.warn("Unable to access field " + containerField, e);
             }