Cleanup in MySQL and SQL Server tests #6759

kokosing · 2021-01-29T15:14:32Z

No description provided.

plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveDistributedQueries.java

...trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerDistributedQueries.java

findepi · 2021-01-29T15:19:04Z

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerClient.java

@@ -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)


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

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

i don't have a warning here

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

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)

findepi · 2021-01-29T15:24:12Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlIntegrationSmokeTest.java

-
-    private void execute(String sql)
-    {
-        mysqlServer.execute(sql, mysqlServer.getUsername(), mysqlServer.getPassword());


This is wrong direction IMO.
Subclass TestGlobalTransactionMySqlIntegrationSmokeTest initializes the server on its own.
Instead of this, we should remove the server field in this class.

please drop this commit for now

Notice that parameters mysqlServer.getUsername(), mysqlServer.getPassword() are useless as these are defaults. Then method exists only for mysqlServer.execute(sql);. Inlining this makes it easier.

Instead of this, we should remove the server field in this class.

I agree that this would be better. However, it is common that we inject infra for tests like that.

Do you suggest that we should make execute abstract? That would be on the other hand increase boilerplate on implementation side.

Do you suggest that we should make execute abstract?

Yes, i think so, but let's out of scope here

findepi · 2021-01-29T15:26:02Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlDistributedQueries.java

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


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.

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.

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

kokosing

Responded to comments, no code changes yet.

...trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerDistributedQueries.java

kokosing · 2021-01-29T20:30:00Z

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestSqlServerClient.java

@@ -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)


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

kokosing · 2021-01-29T20:34:36Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlIntegrationSmokeTest.java

-
-    private void execute(String sql)
-    {
-        mysqlServer.execute(sql, mysqlServer.getUsername(), mysqlServer.getPassword());


Notice that parameters mysqlServer.getUsername(), mysqlServer.getPassword() are useless as these are defaults. Then method exists only for mysqlServer.execute(sql);. Inlining this makes it easier.

Instead of this, we should remove the server field in this class.

I agree that this would be better. However, it is common that we inject infra for tests like that.

Do you suggest that we should make execute abstract? That would be on the other hand increase boilerplate on implementation side.

kokosing · 2021-01-29T20:41:56Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestMySqlDistributedQueries.java

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


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.

kokosing · 2021-01-30T22:09:26Z

@findepi CA, PTAL

cla-bot bot added the cla-signed label Jan 29, 2021

kokosing requested a review from losipiuk January 29, 2021 15:14

findepi reviewed Jan 29, 2021

View reviewed changes

kokosing commented Jan 29, 2021

View reviewed changes

kokosing force-pushed the origin/master/271_close branch from 5a312cf to 007939f Compare January 30, 2021 22:08

kokosing added 5 commits February 1, 2021 11:40

Remove not needed parameter

19288f1

Update comment

00295c2

Remove extra not needed annotations

3a54d97

Make method static

f5addd9

Close testing servers after QueryRunner

6f2f189

kokosing force-pushed the origin/master/271_close branch from 007939f to 6f2f189 Compare February 1, 2021 10:42

This was referenced Feb 1, 2021

Allow to reuse few testing servers #6776

Merged

Fix case insensitive predicate pushdown for MySQL, MemSQL and SQL Server #6753

Merged

findepi approved these changes Feb 1, 2021

View reviewed changes

kokosing merged commit d55d899 into trinodb:master Feb 1, 2021

kokosing deleted the origin/master/271_close branch February 1, 2021 12:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup in MySQL and SQL Server tests #6759

Cleanup in MySQL and SQL Server tests #6759

kokosing commented Jan 29, 2021

findepi Jan 29, 2021

kokosing Jan 29, 2021

findepi Jan 29, 2021

kokosing Jan 30, 2021

findepi Feb 1, 2021

findepi Jan 29, 2021

kokosing Jan 29, 2021

findepi Jan 29, 2021

findepi Jan 29, 2021

kokosing Jan 29, 2021

findepi Jan 29, 2021

kokosing left a comment

kokosing Jan 29, 2021

kokosing Jan 29, 2021

kokosing Jan 29, 2021

kokosing commented Jan 30, 2021

Cleanup in MySQL and SQL Server tests #6759

Cleanup in MySQL and SQL Server tests #6759

Conversation

kokosing commented Jan 29, 2021

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

kokosing 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

kokosing commented Jan 30, 2021