Close Postgres test resources in order

Before this change testing Postgres server was closed before the query runner. That led to the situation where query runner was up and running but without a dependant service. In case of Postgres testing this was not causing any issue as query runner was not used during that time, but this reveals a generic tests infra issue that may affect other tests.
trinodb · Sep 22, 2020 · 5f9d7ae · 5f9d7ae
1 parent a223d02
commit 5f9d7ae
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 32 deletions.
diff --git a/...ql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlCaseInsensitiveMapping.java b/...ql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlCaseInsensitiveMapping.java
@@ -17,7 +17,6 @@
 import com.google.common.collect.ImmutableSet;
 import io.prestosql.testing.AbstractTestQueryFramework;
 import io.prestosql.testing.QueryRunner;
-import org.testng.annotations.AfterClass;
 import org.testng.annotations.Test;
 
 import java.sql.Connection;
@@ -42,19 +41,17 @@ protected QueryRunner createQueryRunner()
             throws Exception
     {
         postgreSqlServer = new TestingPostgreSqlServer();
+        closeAfterClass(() -> {
+            postgreSqlServer.close();
+            postgreSqlServer = null;
+        });
         return PostgreSqlQueryRunner.createPostgreSqlQueryRunner(
                 postgreSqlServer,
                 ImmutableMap.of(),
                 ImmutableMap.of("case-insensitive-name-matching", "true"),
                 ImmutableSet.of());
     }
 
-    @AfterClass(alwaysRun = true)
-    public final void destroy()
-    {
-        postgreSqlServer.close();
-    }
-
     @Test
     public void testNonLowerCaseSchemaName()
             throws Exception

diff --git a/...gresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlDistributedQueries.java b/...gresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlDistributedQueries.java
@@ -19,7 +19,6 @@
 import io.prestosql.testing.sql.JdbcSqlExecutor;
 import io.prestosql.testing.sql.TestTable;
 import io.prestosql.tpch.TpchTable;
-import org.testng.annotations.AfterClass;
 
 import static io.prestosql.plugin.postgresql.PostgreSqlQueryRunner.createPostgreSqlQueryRunner;
 
@@ -33,6 +32,10 @@ protected QueryRunner createQueryRunner()
             throws Exception
     {
         postgreSqlServer = new TestingPostgreSqlServer();
+        closeAfterClass(() -> {
+            postgreSqlServer.close();
+            postgreSqlServer = null;
+        });
         return createPostgreSqlQueryRunner(
                 postgreSqlServer,
                 ImmutableMap.of(),
@@ -44,12 +47,6 @@ protected QueryRunner createQueryRunner()
                 TpchTable.getTables());
     }
 
-    @AfterClass(alwaysRun = true)
-    public final void destroy()
-    {
-        postgreSqlServer.close();
-    }
-
     @Override
     protected boolean supportsViews()
     {

diff --git a/...esql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlIntegrationSmokeTest.java b/...esql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlIntegrationSmokeTest.java
@@ -17,7 +17,6 @@
 import io.prestosql.testing.AbstractTestIntegrationSmokeTest;
 import io.prestosql.testing.QueryRunner;
 import org.intellij.lang.annotations.Language;
-import org.testng.annotations.AfterClass;
 import org.testng.annotations.Test;
 
 import java.sql.Connection;
@@ -46,16 +45,14 @@ protected QueryRunner createQueryRunner()
             throws Exception
     {
         postgreSqlServer = new TestingPostgreSqlServer();
+        closeAfterClass(() -> {
+            postgreSqlServer.close();
+            postgreSqlServer = null;
+        });
         execute("CREATE EXTENSION file_fdw");
         return PostgreSqlQueryRunner.createPostgreSqlQueryRunner(postgreSqlServer, CUSTOMER, NATION, ORDERS, REGION);
     }
 
-    @AfterClass(alwaysRun = true)
-    public final void destroy()
-    {
-        postgreSqlServer.close();
-    }
-
     @Test
     public void testDropTable()
     {

diff --git a/...to-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java b/...to-postgresql/src/test/java/io/prestosql/plugin/postgresql/TestPostgreSqlTypeMapping.java
@@ -36,7 +36,6 @@
 import io.prestosql.testing.sql.JdbcSqlExecutor;
 import io.prestosql.testing.sql.PrestoSqlExecutor;
 import io.prestosql.testing.sql.TestTable;
-import org.testng.annotations.AfterClass;
 import org.testng.annotations.BeforeClass;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
@@ -145,19 +144,17 @@ protected QueryRunner createQueryRunner()
             throws Exception
     {
         postgreSqlServer = new TestingPostgreSqlServer();
+        closeAfterClass(() -> {
+            postgreSqlServer.close();
+            postgreSqlServer = null;
+        });
         return createPostgreSqlQueryRunner(
                 postgreSqlServer,
                 ImmutableMap.of(),
                 ImmutableMap.of("jdbc-types-mapped-to-varchar", "Tsrange, Inet" /* make sure that types are compared case insensitively */),
                 ImmutableList.of());
     }
 
-    @AfterClass(alwaysRun = true)
-    public final void destroy()
-    {
-        postgreSqlServer.close();
-    }
-
     @BeforeClass
     public void setUp()
     {

diff --git a/presto-testing/pom.xml b/presto-testing/pom.xml
@@ -92,6 +92,11 @@
             <artifactId>jackson-annotations</artifactId>
         </dependency>
 
+        <dependency>
+            <groupId>com.google.errorprone</groupId>
+            <artifactId>error_prone_annotations</artifactId>
+        </dependency>
+
         <dependency>
             <groupId>com.google.guava</groupId>
             <artifactId>guava</artifactId>

diff --git a/presto-testing/src/main/java/io/prestosql/testing/AbstractTestQueryFramework.java b/presto-testing/src/main/java/io/prestosql/testing/AbstractTestQueryFramework.java
@@ -16,6 +16,8 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.MoreCollectors;
+import com.google.common.io.Closer;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import io.airlift.units.Duration;
 import io.prestosql.Session;
 import io.prestosql.cost.CostCalculator;
@@ -57,6 +59,8 @@
 import org.weakref.jmx.MBeanExporter;
 import org.weakref.jmx.testing.TestingMBeanServer;
 
+import java.io.Closeable;
+import java.io.IOException;
 import java.util.List;
 import java.util.Optional;
 import java.util.OptionalLong;
@@ -66,7 +70,6 @@
 import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.base.Strings.nullToEmpty;
 import static com.google.common.collect.ImmutableList.toImmutableList;
-import static io.airlift.testing.Closeables.closeAllRuntimeException;
 import static io.prestosql.SystemSessionProperties.JOIN_DISTRIBUTION_TYPE;
 import static io.prestosql.SystemSessionProperties.JOIN_REORDERING_STRATEGY;
 import static io.prestosql.sql.ParsingUtil.createParsingOptions;
@@ -83,14 +86,15 @@ public abstract class AbstractTestQueryFramework
     private QueryRunner queryRunner;
     private H2QueryRunner h2QueryRunner;
     private SqlParser sqlParser;
+    private final Closer afterClassCloser = Closer.create();
     private io.prestosql.sql.query.QueryAssertions queryAssertions;
 
     @BeforeClass
     public void init()
             throws Exception
     {
-        queryRunner = createQueryRunner();
-        h2QueryRunner = new H2QueryRunner();
+        queryRunner = afterClassCloser.register(createQueryRunner());
+        h2QueryRunner = afterClassCloser.register(new H2QueryRunner());
         sqlParser = new SqlParser();
         queryAssertions = new io.prestosql.sql.query.QueryAssertions(queryRunner);
     }
@@ -100,8 +104,9 @@ protected abstract QueryRunner createQueryRunner()
 
     @AfterClass(alwaysRun = true)
     public void close()
+            throws IOException
     {
-        closeAllRuntimeException(queryRunner, h2QueryRunner);
+        afterClassCloser.close();
         queryRunner = null;
         h2QueryRunner = null;
         sqlParser = null;
@@ -459,4 +464,10 @@ protected OperatorStats searchScanFilterAndProjectOperatorStats(QueryId queryId,
                 .filter(summary -> nodeId.equals(summary.getPlanNodeId()) && summary.getOperatorType().equals("ScanFilterAndProjectOperator"))
                 .collect(MoreCollectors.onlyElement());
     }
+
+    @CanIgnoreReturnValue
+    protected final <T extends Closeable> T closeAfterClass(T resource)
+    {
+        return afterClassCloser.register(resource);
+    }
 }