Test JDBC connection creations by kokosing · Pull Request #14749 · trinodb/trino

kokosing · 2022-10-25T12:11:41Z

Test JDBC connection creations

findepi · 2022-10-26T09:00:22Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

Does singleThreaded on a base class apply to child classes?
(i think it does not)

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectionCreation.java

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

findepi · 2022-10-26T09:26:37Z

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

Can this be simpler?
E.g. we don't have any extra credentials in the test, and also a StaticCredentialProvider was already constructed earlier.

I would prefer this testing plugin for postgresql to be as close to real as possible. We don't win much using things like StaticCredentialProvider and we may end up having completely two connectors for postgresql, which I would like to avoid.

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

findepi · 2022-10-26T09:28:02Z

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

See

trino/plugin/trino-resource-group-managers/src/test/java/io/trino/plugin/resourcegroups/db/TestDbResourceGroupsOracleFlywayMigration.java

Lines 86 to 92 in 23ca99c

@Test

public void forceTestNgToRespectSingleThreaded()

{

// TODO: Remove after updating TestNG to 7.4.0+ (https://github.com/trinodb/trino/issues/8571)

// TestNG doesn't enforce @Test(singleThreaded = true) when tests are defined in base class. According to

// https://github.com/cbeust/testng/issues/2361#issuecomment-688393166 a workaround it to add a dummy test to the leaf test class.

}

for the convention.

It is no longer needed.

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

...ostgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClientAuthenticationModule.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

kokosing

AC, PTAL

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectionCreation.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestingH2JdbcModule.java

...ostgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClientAuthenticationModule.java

kokosing · 2022-10-27T14:29:52Z

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

It is no longer needed.

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

kokosing · 2022-10-27T14:33:42Z

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

I would prefer this testing plugin for postgresql to be as close to real as possible. We don't win much using things like StaticCredentialProvider and we may end up having completely two connectors for postgresql, which I would like to avoid.

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

ssheikin · 2022-10-27T15:40:43Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

IllegalStateException(String message, Throwable cause)

There is a difference in semantic between suppressed exception and a cause exception. Neither of them fits in 100% here, however suppressed sounds to be closer ;)

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectionCreation.java

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClientModule.java

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectionCreation.java

Praveen2112 · 2022-10-28T14:17:50Z

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

Instead of creating a new TestingPostgreSqlPlugin can we combine these module and create a JdbcPlugin like this new JdbcPlugin("jdbc", combine(new PostgreSqlClientModule(), new TestingPostgreSqlModule())

kokosing

AC

kokosing · 2022-10-28T15:00:21Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

There is a difference in semantic between suppressed exception and a cause exception. Neither of them fits in 100% here, however suppressed sounds to be closer ;)

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectionCreation.java

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

kokosing · 2022-10-28T15:07:25Z

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

ssheikin · 2022-10-28T16:01:36Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectionCreation.java

should name contain h2?

h2 is an implementation detail. There is no such connector like h2. It is more about testing the base jdbc framework, which we usually call jdbc in tests.

ssheikin · 2022-10-28T16:05:28Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectionCreation.java

Just thoughs aloud: these are trino statements, and reused for different connectors, however even for h2 and postgres numbers are really different. Would be great to reuse them somehow..

Queries are the almost the same, however expected behavior is connector specific. I don't see a good way of doing this and keeping the code simple.

ssheikin · 2022-10-28T16:07:23Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

is it the last entry by intention? does it matter?

To make sure that closed and connectionCreations are in a good shape. However it does not matter much. Tests will fail in case of any exception here.

I think it does not matter too.

kokosing

AC

kokosing · 2022-10-28T18:38:55Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

To make sure that closed and connectionCreations are in a good shape. However it does not matter much. Tests will fail in case of any exception here.

kokosing · 2022-10-28T18:39:39Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectionCreation.java

h2 is an implementation detail. There is no such connector like h2. It is more about testing the base jdbc framework, which we usually call jdbc in tests.

kokosing · 2022-10-28T18:40:55Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectionCreation.java

Queries are the almost the same, however expected behavior is connector specific. I don't see a good way of doing this and keeping the code simple.

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectionCreation.java

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

hashhar · 2022-10-31T11:56:35Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

is ordering across @BeforeClass methods deterministic?

hashhar · 2022-10-31T11:57:42Z

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

clever - unfortunately I can't think of better ways to store this information either

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java

hashhar · 2022-10-31T12:00:09Z

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java

is this significant for the test? Deserves some comment IMO.

I wanted to make sure that we always open connections in a deterministic manner. Like we read data on worker. I don't think it is needed and possibly it lowers the test coverage. So I will remove it.

That way it is possible to inject a different way to provision ConnectionFactory in PostgreSql connector.

hashhar

LGTM % ~~some clarity around #14749 (comment) (@BeforeClass ordering)~~

cla-bot bot added the cla-signed label Oct 25, 2022

kokosing requested review from findepi and ssheikin October 25, 2022 12:12

kokosing mentioned this pull request Oct 25, 2022

Reuse JDBC connection in JDBC connectors #14653

Merged

kokosing force-pushed the origin/master/155_test_jdbc_connections branch from d8d633b to 07b45f2 Compare October 25, 2022 19:56

findepi reviewed Oct 26, 2022

View reviewed changes

ssheikin reviewed Oct 27, 2022

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectionCreationTest.java Outdated Show resolved Hide resolved

kokosing force-pushed the origin/master/155_test_jdbc_connections branch 2 times, most recently from 875bcba to 640cd3e Compare October 27, 2022 14:36

kokosing commented Oct 27, 2022

View reviewed changes

kokosing force-pushed the origin/master/155_test_jdbc_connections branch 2 times, most recently from 54a2cde to 4e90d4d Compare October 27, 2022 14:40

ssheikin reviewed Oct 27, 2022

View reviewed changes

Praveen2112 reviewed Oct 28, 2022

View reviewed changes

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClientModule.java Outdated Show resolved Hide resolved

...ostgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlJdbcConnectionCreation.java Outdated Show resolved Hide resolved

Praveen2112 reviewed Oct 28, 2022

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectionCreation.java Outdated Show resolved Hide resolved

Praveen2112 reviewed Oct 28, 2022

View reviewed changes

kokosing force-pushed the origin/master/155_test_jdbc_connections branch from 4e90d4d to b525f99 Compare October 28, 2022 15:08

kokosing commented Oct 28, 2022

View reviewed changes

ssheikin approved these changes Oct 28, 2022

View reviewed changes

kokosing commented Oct 28, 2022

View reviewed changes

kokosing force-pushed the origin/master/155_test_jdbc_connections branch 3 times, most recently from 8b3adeb to bd1a780 Compare October 31, 2022 09:55

Praveen2112 reviewed Oct 31, 2022

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcConnectionCreation.java Outdated Show resolved Hide resolved

hashhar reviewed Oct 31, 2022

View reviewed changes

kokosing added 2 commits October 31, 2022 13:07

Extract PostgreSqlConnectionFactoryModule

0dcaa71

That way it is possible to inject a different way to provision ConnectionFactory in PostgreSql connector.

Test JDBC connection creations

c697cf6

kokosing force-pushed the origin/master/155_test_jdbc_connections branch from bd1a780 to c697cf6 Compare October 31, 2022 12:26

hashhar approved these changes Oct 31, 2022

View reviewed changes

kokosing merged commit b92d259 into trinodb:master Oct 31, 2022

kokosing deleted the origin/master/155_test_jdbc_connections branch October 31, 2022 14:28

github-actions bot added this to the 402 milestone Oct 31, 2022

colebow mentioned this pull request Nov 1, 2022

Add Trino 402 release notes #14862

Merged

	@Test
	public void forceTestNgToRespectSingleThreaded()
	{
	// TODO: Remove after updating TestNG to 7.4.0+ (https://github.com/trinodb/trino/issues/8571)
	// TestNG doesn't enforce @Test(singleThreaded = true) when tests are defined in base class. According to
	// https://github.com/cbeust/testng/issues/2361#issuecomment-688393166 a workaround it to add a dummy test to the leaf test class.
	}

Conversation

kokosing commented Oct 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing left a comment

hashhar left a comment •

edited

Loading