Skip to content
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.

Sign up for GitHub

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

Cleanup in MySQL and SQL Server tests #6759

Merged
merged 5 commits into from
Feb 1, 2021
Merged

Cleanup in MySQL and SQL Server tests #6759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
19288f1
Remove not needed parameter
kokosing Jan 28, 2021
00295c2
Update comment
kokosing Jan 28, 2021
3a54d97
Remove extra not needed annotations
kokosing Jan 28, 2021
f5addd9
Make method static
kokosing Jan 28, 2021
6f2f189
Close testing servers after QueryRunner
kokosing Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ else if (baseRelation instanceof JdbcQueryRelationHandle) {
throw new IllegalArgumentException("Unsupported relation: " + baseRelation);
}

List<String> clauses = toConjuncts(client, session, connection, tupleDomain, accumulator::add);
List<String> clauses = toConjuncts(session, connection, tupleDomain, accumulator::add);
if (additionalPredicate.isPresent()) {
clauses = ImmutableList.<String>builder()
.addAll(clauses)
Expand Down Expand Up @@ -212,7 +212,6 @@ private static Domain pushDownDomain(JdbcClient client, ConnectorSession session
}

private List<String> toConjuncts(
JdbcClient client,
ConnectorSession session,
Connection connection,
TupleDomain<ColumnHandle> tupleDomain,
Expand Down
3 changes: 2 additions & 1 deletion plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveDistributedQueries.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ protected boolean supportsCommentOnColumn()
@Override
public void testDelete()
{
// Hive connector currently does not support row-by-row delete
// Hive connector supports row-by-row delete only for ACID tables
// but these currently cannot be used with file metastore.
}

@Test
Expand Down
7 changes: 0 additions & 7 deletions plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlIntegrationSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import io.trino.testing.MaterializedRow;
import io.trino.testing.sql.TestTable;
import org.intellij.lang.annotations.Language;
import org.testng.annotations.AfterClass;
import org.testng.annotations.Test;

import static com.google.common.collect.Iterables.getOnlyElement;
Expand All @@ -40,12 +39,6 @@ abstract class BaseMySqlIntegrationSmokeTest
{
protected TestingMySqlServer mysqlServer;

@AfterClass(alwaysRun = true)
public final void destroy()
{
mysqlServer.close();
}

@Test
@Override
public void testDescribeTable()
Expand Down
2 changes: 1 addition & 1 deletion ...l/src/test/java/io/trino/plugin/mysql/TestGlobalTransactionMySqlIntegrationSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public class TestGlobalTransactionMySqlIntegrationSmokeTest
protected QueryRunner createQueryRunner()
throws Exception
{
mysqlServer = new TestingMySqlServer(true);
mysqlServer = closeAfterClass(new TestingMySqlServer(true));
return createMySqlQueryRunner(mysqlServer, CUSTOMER, NATION, ORDERS, REGION);
}
}
9 changes: 1 addition & 8 deletions plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlCaseInsensitiveMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import com.google.common.collect.ImmutableSet;
import io.trino.testing.AbstractTestQueryFramework;
import io.trino.testing.QueryRunner;
import org.testng.annotations.AfterClass;
import org.testng.annotations.Test;

import java.util.stream.Stream;
Expand All @@ -40,16 +39,10 @@ public class TestMySqlCaseInsensitiveMapping
protected QueryRunner createQueryRunner()
throws Exception
{
mySqlServer = new TestingMySqlServer();
mySqlServer = closeAfterClass(new TestingMySqlServer());
return createMySqlQueryRunner(mySqlServer, ImmutableMap.of(), ImmutableMap.of("case-insensitive-name-matching", "true"), ImmutableList.of());
}

@AfterClass(alwaysRun = true)
public final void destroy()
{
mySqlServer.close();
}

@Test
public void testNonLowerCaseSchemaName()
throws Exception
Expand Down
2 changes: 1 addition & 1 deletion plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ public void testImplementSum()
Optional.empty()); // filter not supported
}

private void testImplementAggregation(AggregateFunction aggregateFunction, Map<String, ColumnHandle> assignments, Optional<String> expectedExpression)
private static void testImplementAggregation(AggregateFunction aggregateFunction, Map<String, ColumnHandle> assignments, Optional<String> expectedExpression)
{
Optional<JdbcExpression> result = JDBC_CLIENT.implementAggregation(SESSION, aggregateFunction, assignments);
if (expectedExpression.isEmpty()) {
Expand Down
10 changes: 1 addition & 9 deletions plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlDistributedQueries.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import io.trino.testing.QueryRunner;
import io.trino.testing.sql.TestTable;
import io.trino.tpch.TpchTable;
import org.testng.annotations.AfterClass;

import java.util.Optional;

Expand All @@ -38,7 +37,7 @@ public class TestMySqlDistributedQueries
protected QueryRunner createQueryRunner()
throws Exception
{
this.mysqlServer = new TestingMySqlServer();
this.mysqlServer = closeAfterClass(new TestingMySqlServer());
return createMySqlQueryRunner(
mysqlServer,
ImmutableMap.of(),
Expand All @@ -50,13 +49,6 @@ protected QueryRunner createQueryRunner()
TpchTable.getTables());
}

@AfterClass(alwaysRun = true)
public final void destroy()
{
mysqlServer.close();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see closeAfterClass benefits, especially where state management is hard to do without it. That was the reason why we added that method.
But no, it does not improve readability. It makes is less clear that the resources is properly disposed of after the test class.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But no, it does not improve readability.

I don't agree.

Pros of closeAfterClass:

  • it is concise. Used when resource is created is like try-with-resources you know already that it is going to bel deallocated properly.
  • It is also correct. Resources are deallocated in proper order.

Cons of @AfterClass kind of deallocation:

  • it is detached from allocation. You need to find a place where object is deallocated. In this commit I removed a case where object was created in one class and destroyed in other. Since it is detached one can even deallocate thing several times.
  • I always need to double check if alwaysRun is used. Wasted energy, while with closeAfterClass I know it is implemented correctly.
  • more boilerplate, like a need to create field.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You missed the fact @AfterClass cleanup is standard and globally applicable, whereas closeAfterClass is available in these tests only.

I am not OK making closeAfterClass a rule. I am 'whatever' on a change here

mysqlServer = null;
}

@Override
protected boolean supportsDelete()
{
Expand Down
2 changes: 1 addition & 1 deletion plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlIntegrationSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public class TestMySqlIntegrationSmokeTest
protected QueryRunner createQueryRunner()
throws Exception
{
mysqlServer = new TestingMySqlServer(false);
mysqlServer = closeAfterClass(new TestingMySqlServer(false));
return createMySqlQueryRunner(mysqlServer, CUSTOMER, NATION, ORDERS, REGION);
}

Expand Down
9 changes: 1 addition & 8 deletions plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlTypeMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
import io.trino.testing.sql.SqlExecutor;
import io.trino.testing.sql.TestTable;
import io.trino.testing.sql.TrinoSqlExecutor;
import org.testng.annotations.AfterClass;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

Expand Down Expand Up @@ -93,16 +92,10 @@ public class TestMySqlTypeMapping
protected QueryRunner createQueryRunner()
throws Exception
{
mysqlServer = new TestingMySqlServer();
mysqlServer = closeAfterClass(new TestingMySqlServer());
return createMySqlQueryRunner(mysqlServer, ImmutableMap.of(), ImmutableMap.of(), ImmutableList.of());
}

@AfterClass(alwaysRun = true)
public final void destroy()
{
mysqlServer.close();
}

@Test
public void testBasicTypes()
{
Expand Down
2 changes: 1 addition & 1 deletion plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ public void testImplementSum()
Optional.empty()); // filter not supported
}

private void testImplementAggregation(AggregateFunction aggregateFunction, Map<String, ColumnHandle> assignments, Optional<String> expectedExpression)
private static void testImplementAggregation(AggregateFunction aggregateFunction, Map<String, ColumnHandle> assignments, Optional<String> expectedExpression)
{
Optional<JdbcExpression> result = JDBC_CLIENT.implementAggregation(SESSION, aggregateFunction, assignments);
if (expectedExpression.isEmpty()) {
Expand Down
2 changes: 1 addition & 1 deletion plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public void testImplementSum()
Optional.empty()); // filter not supported
}

private void testImplementAggregation(AggregateFunction aggregateFunction, Map<String, ColumnHandle> assignments, Optional<String> expectedExpression)
private static void testImplementAggregation(AggregateFunction aggregateFunction, Map<String, ColumnHandle> assignments, Optional<String> expectedExpression)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same applies to all TestXxxClient classes, as they are copies.
Not much benefit though IMO

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got warnings in IDE. I don't like warnings, but I do like static methods.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't have a warning here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See also: https://errorprone.info/bugpattern/MethodCanBeStatic

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we decide to use that?

I got warnings in IDE. I don't like warnings, but I do like static methods.

i don't like either.

but "method can be static" is test code is counter productive -- the tests themselves are isntance methods, so we shouldn't care if a test helper is an instance method as well, especially when it implements a test's logic (as case here) as compared to being ordinary utility method (those should definitely be static)

{
Optional<JdbcExpression> result = JDBC_CLIENT.implementAggregation(SESSION, aggregateFunction, assignments);
if (expectedExpression.isEmpty()) {
Expand Down
12 changes: 1 addition & 11 deletions ...no-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerDistributedQueries.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,11 @@
import io.trino.testing.QueryRunner;
import io.trino.testing.sql.TestTable;
import io.trino.tpch.TpchTable;
import org.testng.annotations.AfterClass;
import org.testng.annotations.Test;

import java.util.Optional;

import static io.trino.plugin.sqlserver.SqlServerQueryRunner.createSqlServerQueryRunner;

@Test
public class TestSqlServerDistributedQueries
extends AbstractTestDistributedQueries
{
Expand All @@ -35,7 +32,7 @@ public class TestSqlServerDistributedQueries
protected QueryRunner createQueryRunner()
throws Exception
{
this.sqlServer = new TestingSqlServer();
this.sqlServer = closeAfterClass(new TestingSqlServer());
sqlServer.start();
return createSqlServerQueryRunner(
sqlServer,
Expand All @@ -48,13 +45,6 @@ protected QueryRunner createQueryRunner()
TpchTable.getTables());
}

@AfterClass(alwaysRun = true)
public final void destroy()
{
sqlServer.close();
sqlServer = null;
}

@Override
protected boolean supportsDelete()
{
Expand Down
10 changes: 1 addition & 9 deletions ...-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerIntegrationSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import io.trino.testing.AbstractTestIntegrationSmokeTest;
import io.trino.testing.QueryRunner;
import io.trino.testing.sql.TestTable;
import org.testng.annotations.AfterClass;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;

Expand All @@ -41,7 +40,6 @@
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertTrue;

@Test
public class TestSqlServerIntegrationSmokeTest
extends AbstractTestIntegrationSmokeTest
{
Expand All @@ -51,17 +49,11 @@ public class TestSqlServerIntegrationSmokeTest
protected QueryRunner createQueryRunner()
throws Exception
{
sqlServer = new TestingSqlServer();
sqlServer = closeAfterClass(new TestingSqlServer());
sqlServer.start();
return createSqlServerQueryRunner(sqlServer, ImmutableMap.of(), ImmutableMap.of(), ImmutableList.of(CUSTOMER, NATION, ORDERS, REGION));
}

@AfterClass(alwaysRun = true)
public final void destroy()
{
sqlServer.close();
}

@Test
public void testInsert()
{
Expand Down
9 changes: 1 addition & 8 deletions plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerTypeMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import io.trino.testing.datatype.DataSetup;
import io.trino.testing.datatype.SqlDataTypeTest;
import io.trino.testing.sql.TrinoSqlExecutor;
import org.testng.annotations.AfterClass;
import org.testng.annotations.Test;

import static io.trino.plugin.sqlserver.SqlServerQueryRunner.createSqlServerQueryRunner;
Expand All @@ -37,7 +36,7 @@ public class TestSqlServerTypeMapping
protected QueryRunner createQueryRunner()
throws Exception
{
sqlServer = new TestingSqlServer();
sqlServer = closeAfterClass(new TestingSqlServer());
sqlServer.start();
return createSqlServerQueryRunner(
sqlServer,
Expand All @@ -46,12 +45,6 @@ protected QueryRunner createQueryRunner()
ImmutableList.of());
}

@AfterClass(alwaysRun = true)
public final void destroy()
{
sqlServer.close();
}

@Test
public void testVarbinary()
{
Expand Down
12 changes: 1 addition & 11 deletions ...server/src/test/java/io/trino/plugin/sqlserver/TestSqlServerWithoutSnapshotIsolation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import io.trino.testing.MaterializedResult;
import io.trino.testing.QueryRunner;
import io.trino.tpch.TpchTable;
import org.testng.annotations.AfterClass;
import org.testng.annotations.Test;

import static io.trino.plugin.sqlserver.SqlServerQueryRunner.createSqlServerQueryRunner;
Expand All @@ -29,13 +28,11 @@
public class TestSqlServerWithoutSnapshotIsolation
extends AbstractTestQueryFramework
{
private TestingSqlServer sqlServer;

@Override
protected QueryRunner createQueryRunner()
throws Exception
{
sqlServer = new TestingSqlServer(false);
TestingSqlServer sqlServer = closeAfterClass(new TestingSqlServer(false));
sqlServer.start();
return createSqlServerQueryRunner(
sqlServer,
Expand All @@ -44,13 +41,6 @@ protected QueryRunner createQueryRunner()
ImmutableList.of(TpchTable.NATION));
}

@AfterClass(alwaysRun = true)
public final void destroy()
{
sqlServer.close();
sqlServer = null;
}

@Test
public void testCreateReadTable()
{
Expand Down