From 5cb84ad3e96cdf3e3cef5866608107a958749a97 Mon Sep 17 00:00:00 2001 From: Rudy De Busscher Date: Mon, 3 Apr 2023 18:13:36 +0200 Subject: [PATCH] Code review --- .../exception/DataScriptException.java | 24 +++++++++ .../exception/DatabaseScriptException.java | 24 +++++++++ .../database/image/JDBCDriverFileUtil.java | 3 ++ .../DatabaseContainerAdapterMetaData.java | 2 +- .../DatabaseTestcontainersController.java | 50 ++++++++++++------- .../example/hello/DatabasePayaraIT.java | 1 - example/database/wildfly/pom.xml | 2 +- .../src/test/resources/create-tables.sql | 16 ------ .../src/test/resources/drop-tables.sql | 16 ------ .../AbstractIntegrationContainer.java | 4 +- .../exception/UnexpectedException.java | 4 ++ .../container/image/DockerImageProcessor.java | 6 ++- .../container/image/DockerImageProducer.java | 4 +- .../jupiter/ContainerAdapterMetaData.java | 7 +-- .../jupiter/TestcontainersController.java | 2 +- .../ShowLogWhenFailedExceptionHandler.java | 11 ++-- 16 files changed, 108 insertions(+), 68 deletions(-) create mode 100644 database/src/main/java/be/atbash/testing/integration/database/exception/DataScriptException.java create mode 100644 database/src/main/java/be/atbash/testing/integration/database/exception/DatabaseScriptException.java diff --git a/database/src/main/java/be/atbash/testing/integration/database/exception/DataScriptException.java b/database/src/main/java/be/atbash/testing/integration/database/exception/DataScriptException.java new file mode 100644 index 0000000..46d9d56 --- /dev/null +++ b/database/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/database/src/main/java/be/atbash/testing/integration/database/exception/DatabaseScriptException.java b/database/src/main/java/be/atbash/testing/integration/database/exception/DatabaseScriptException.java new file mode 100644 index 0000000..ae80ac7 --- /dev/null +++ b/database/src/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 index 6b9f750..286c376 100644 --- 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/database/src/main/java/be/atbash/testing/integration/database/jupiter/DatabaseContainerAdapterMetaData.java b/database/src/main/java/be/atbash/testing/integration/database/jupiter/DatabaseContainerAdapterMetaData.java index e011f1a..2dd4a51 100644 --- a/database/src/main/java/be/atbash/testing/integration/database/jupiter/DatabaseContainerAdapterMetaData.java +++ b/database/src/main/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/database/src/main/java/be/atbash/testing/integration/database/jupiter/DatabaseTestcontainersController.java b/database/src/main/java/be/atbash/testing/integration/database/jupiter/DatabaseTestcontainersController.java index cf09236..17daf86 100644 --- a/database/src/main/java/be/atbash/testing/integration/database/jupiter/DatabaseTestcontainersController.java +++ b/database/src/main/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/example/database/payara/src/test/java/be/atbash/test/integration/example/hello/DatabasePayaraIT.java b/example/database/payara/src/test/java/be/atbash/test/integration/example/hello/DatabasePayaraIT.java index c1731f5..7331466 100644 --- a/example/database/payara/src/test/java/be/atbash/test/integration/example/hello/DatabasePayaraIT.java +++ b/example/database/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 index 3231db1..8a0418c 100644 --- a/example/database/wildfly/pom.xml +++ b/example/database/wildfly/pom.xml @@ -37,7 +37,7 @@ org.testcontainers mysql - 1.17.5 + ${testcontainers.version} test diff --git a/example/database/wildfly/src/test/resources/create-tables.sql b/example/database/wildfly/src/test/resources/create-tables.sql index 3948ed2..2717251 100644 --- 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 index 592b00a..4dc9665 100644 --- 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 index 40785b0..f17f2cd 100644 --- 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 image, boolean liveLogging) { + protected AbstractIntegrationContainer(Future 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/impl/src/main/java/be/atbash/testing/integration/container/exception/UnexpectedException.java b/impl/src/main/java/be/atbash/testing/integration/container/exception/UnexpectedException.java index 709fd6c..6c246e7 100644 --- a/impl/src/main/java/be/atbash/testing/integration/container/exception/UnexpectedException.java +++ b/impl/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 index 7fdb7c7..d437b54 100644 --- 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 index b0fcb60..95b9841 100644 --- 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 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 index c3458e1..b5e459f 100644 --- 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 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 index 8e294e8..b2cacfc 100644 --- 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 index 888fca6..bf59f4e 100644 --- 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); }