HHH-13724 Add testing configs and dialect for CockroachDB #3280
Conversation
rafiss
changed the title
| */ | ||
| public class CockroachDB201Dialect extends CockroachDB192Dialect { | ||
| public CockroachDB201Dialect() { | ||
| super(); |
There was a problem hiding this comment.
technically the default invocation of super can be omitted. it is only necessary when parameter(s) needed.
There was a problem hiding this comment.
I noticed that other dialects include the explicit super() call. e.g. PostgreSQL94Dialect.
| @@ -107,6 +107,16 @@ else if (Clob.class.isAssignableFrom( type )) { | |||
| : value; | |||
| return (X) clob; | |||
| } | |||
| else if ( String.class.isAssignableFrom( type ) ) { | |||
| if ( ClobImplementer.class.isInstance( value ) ) { | |||
There was a problem hiding this comment.
So two patterns show up above and it seems great to make them consistent. I think 'isAssignableFrom' is better than the deprecated 'Class#isInstance()', though even instanceof is enough for only static class is used here.
There was a problem hiding this comment.
Class#isInstance isn't deprecated, but I agree that instanceof is better here since we have the class available statically.
I can't replace with isAssignableFrom because that method accepts parameter ofClass<?>, which is not the type of value`
| @@ -232,6 +235,8 @@ public void testMultiselectWithPredicates() { | |||
| } | |||
|
|
|||
| @Test | |||
| @SkipForDialect(CockroachDB192Dialect.class) | |||
| @RequiresDialect(CockroachDB201Dialect.class) | |||
There was a problem hiding this comment.
do we still need @SkipForDialect if we have provided @RequiresDialect already?
There was a problem hiding this comment.
Good point. Actually, I'm not sure how I should implement this. Previously, this test would run against all dialects. Now I want it to run against all dialects except for CockroachDB192Dialect. But if I just use @SkipForDialect(CockroachDB192Dialect.class) then this will also skip the test for CockroachDB201Dialect since it is a sub-class of the 19.2 dialect.
There was a problem hiding this comment.
have you tried with the property strictMatching it is present in both @RequiresDialect and @SkipForDialect?
There was a problem hiding this comment.
ah found it -- there is a strictMatching property i can use here
There was a problem hiding this comment.
@dreab8 thanks! I didn't refresh so I missed your comment.
rafiss
force-pushed
the
HHH-13724-cockroachdb-dialects
branch
from
8c6a5b9
to
41f2ffc
Compare
|
getting closer. now only checkStyle issue shows up during building |
rafiss
force-pushed
the
HHH-13724-cockroachdb-dialects
branch
from
034b482
to
412e482
Compare
| public boolean supportsMixedTypeArithmetic() { | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
can we avoid the introduction of these 2 methods and instead use @SkipForDialect annotation?
There was a problem hiding this comment.
Sounds good. I've removed these new methods
rafiss
force-pushed
the
HHH-13724-cockroachdb-dialects
branch
from
a3be89f
to
d4c502e
Compare
Also add appropriate Skip/Require for the 19.2 and 20.1 dialects
- use instanceOf instead of isInstance() - fix Skip/Require dialect annotations - add license header
Remove the supportsLoFunction and supportsMixedTypeArithmetic dialect checks
rafiss
force-pushed
the
HHH-13724-cockroachdb-dialects
branch
from
4744ff9
to
2ed5f84
Compare
This required updating the tests to avoid dropping and recreating a table in the same transaction, and skipping some tests that will not work with CockroachDB.
...e/src/test/java/org/hibernate/test/where/annotations/LazyManyToManyNonUniqueIdWhereTest.java
Show resolved
Hide resolved
...re/src/test/java/org/hibernate/test/where/annotations/LazyOneToManyNonUniqueIdWhereTest.java
Show resolved
Hide resolved
...e/src/test/java/org/hibernate/test/where/hbm/LazyManyToManyNonUniqueIdNotFoundWhereTest.java
Show resolved
Hide resolved
...nate-core/src/test/java/org/hibernate/test/where/hbm/LazyManyToManyNonUniqueIdWhereTest.java
Show resolved
Hide resolved
| @@ -41,9 +41,12 @@ | |||
| public void setup() { | |||
| doInHibernate( | |||
| this::sessionFactory, session -> { | |||
| session.createSQLQuery( "DROP TABLE IF EXISTS MAIN_TABLE cascade" ).executeUpdate(); | |||
There was a problem hiding this comment.
here
session.createSQLQuery( getDialect().getDropTableString( "MAIN_TABLE" ) ).executeUpdate();
| session.createSQLQuery( "drop table MATERIAL_RATINGS cascade" ).executeUpdate(); | ||
| session.createSQLQuery( "drop table BUILDING_RATINGS cascade" ).executeUpdate(); | ||
| session.createSQLQuery( "drop table ASSOCIATION_TABLE cascade" ).executeUpdate(); | ||
| session.createSQLQuery( "drop table MAIN_TABLE cascade" ).executeUpdate(); |
There was a problem hiding this comment.
here has to be
session.createSQLQuery( getDialect().getDropTableString( "MATERIAL_RATINGS" ) ).executeUpdate();
session.createSQLQuery( getDialect().getDropTableString( "BUILDING_RATINGS" ) ).executeUpdate();
session.createSQLQuery( getDialect().getDropTableString( "ASSOCIATION_TABLE" ) ).executeUpdate();
session.createSQLQuery( getDialect().getDropTableString( "MAIN_TABLE" ) ).executeUpdate();
...e/src/test/java/org/hibernate/test/where/annotations/LazyManyToManyNonUniqueIdWhereTest.java
Show resolved
Hide resolved
...re/src/test/java/org/hibernate/test/where/annotations/LazyOneToManyNonUniqueIdWhereTest.java
Show resolved
Hide resolved
...e/src/test/java/org/hibernate/test/where/hbm/LazyManyToManyNonUniqueIdNotFoundWhereTest.java
Show resolved
Hide resolved
| session.createSQLQuery( "drop table MATERIAL_RATINGS cascade" ).executeUpdate(); | ||
| session.createSQLQuery( "drop table BUILDING_RATINGS cascade" ).executeUpdate(); | ||
| session.createSQLQuery( "drop table ASSOCIATION_TABLE cascade" ).executeUpdate(); | ||
| session.createSQLQuery( "drop table MAIN_TABLE cascade" ).executeUpdate(); |
...nate-core/src/test/java/org/hibernate/test/where/hbm/LazyManyToManyNonUniqueIdWhereTest.java
Show resolved
Hide resolved
| @@ -41,9 +41,12 @@ | |||
| public void setup() { | |||
| doInHibernate( | |||
| this::sessionFactory, session -> { | |||
| session.createSQLQuery( "DROP TABLE IF EXISTS MAIN_TABLE cascade" ).executeUpdate(); | |||
| @@ -126,7 +126,7 @@ protected void doBind(PreparedStatement st, X value, int index, WrapperOptions o | |||
| CharacterStream.class, | |||
| options | |||
| ); | |||
| st.setCharacterStream( index, characterStream.asReader(), characterStream.getLength() ); | |||
| st.setCharacterStream( index, characterStream.asReader() ); | |||
There was a problem hiding this comment.
I think this change is unnecessary
There was a problem hiding this comment.
thanks -- went back to the original version
| @@ -137,7 +137,7 @@ protected void doBind(CallableStatement st, X value, String name, WrapperOptions | |||
| CharacterStream.class, | |||
| options | |||
| ); | |||
| st.setCharacterStream( name, characterStream.asReader(), characterStream.getLength() ); | |||
| st.setCharacterStream( name, characterStream.asReader() ); | |||
| @@ -61,6 +61,7 @@ | |||
| private static final Logger log = Logger.getLogger( LockTest.class ); | |||
|
|
|||
| @Test | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
There was a problem hiding this comment.
can this be changed with @SkipForDialect( value = CockroachDB192Dialect.class
| @@ -145,6 +145,7 @@ public void testNativeSql() { | |||
| } | |||
|
|
|||
| @Test | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
There was a problem hiding this comment.
can this be changed with @SkipForDialect( value = CockroachDB192Dialect.class
| @@ -170,6 +171,7 @@ public void testPessimisticForcedIncrementOverall() { | |||
| } | |||
|
|
|||
| @Test | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
There was a problem hiding this comment.
can this be changed with @SkipForDialect( value = CockroachDB192Dialect.class
| @@ -67,6 +67,7 @@ public void releaseResources() { | |||
|
|
|||
| @Test(timeout = 1000 * 30) //30 seconds | |||
| @TestForIssue(jiraKey = "HHH-11617") | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
There was a problem hiding this comment.
can this be changed with @SkipForDialect( value = CockroachDB192Dialect.class
| @@ -249,6 +252,7 @@ public void testConstraints() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
There was a problem hiding this comment.
can this be changed with @SkipForDialect( value = CockroachDB192Dialect.class
| @@ -392,6 +396,7 @@ public void testMultiTable() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
There was a problem hiding this comment.
can this be changed with @SkipForDialect( value = CockroachDB192Dialect.class
| @@ -1095,6 +1090,7 @@ public void testDeleteEmpty() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
There was a problem hiding this comment.
can this be changed with @SkipForDialect( value = CockroachDB192Dialect.class
| @@ -71,6 +70,7 @@ public void testSQLServerSkipLocked() { | |||
|
|
|||
| @Test | |||
| @RequiresDialect({ PostgreSQL95Dialect.class }) | |||
| @RequiresDialectFeature(DialectChecks.SupportSkipLocked.class) | |||
There was a problem hiding this comment.
I think @RequiresDialectFeature can be removed
| @@ -201,6 +201,7 @@ public void testRefreshWithExplicitLowerLevelLockMode() { | |||
|
|
|||
| @Test | |||
| @TestForIssue(jiraKey = "HHH-12257") | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
There was a problem hiding this comment.
can this be changed with @SkipForDialect( value = CockroachDB192Dialect.class
| @@ -54,6 +56,7 @@ public void createTestData() { | |||
| @Test | |||
| @RequiresDialect({ Oracle8iDialect.class, PostgreSQL81Dialect.class, | |||
| SQLServer2005Dialect.class } ) | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
There was a problem hiding this comment.
I think @RequiresDialectFeature can be removed
| @@ -79,6 +82,7 @@ public void testNoWait() | |||
|
|
|||
| @Test | |||
| @RequiresDialect({ Oracle8iDialect.class, PostgreSQL95Dialect.class }) | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
There was a problem hiding this comment.
I think @RequiresDialectFeature can be removed
| session.createSQLQuery( getDialect().getDropTableString( "MATERIAL_RATINGS" ) ).executeUpdate(); | ||
| session.createSQLQuery( getDialect().getDropTableString( "BUILDING_RATINGS" ) ).executeUpdate(); | ||
| session.createSQLQuery( getDialect().getDropTableString( "ASSOCIATION_TABLE" ) ).executeUpdate(); | ||
| session.createSQLQuery( getDialect().getDropTableString( "MAIN_TABLE" ) ).executeUpdate(); |
There was a problem hiding this comment.
This change is causing a failure with Oracle, it has to be reverted to
session.createSQLQuery( "delete from MATERIAL_RATINGS" ).executeUpdate();
session.createSQLQuery( "delete from BUILDING_RATINGS" ).executeUpdate();
session.createSQLQuery( "delete from ASSOCIATION_TABLE" ).executeUpdate();
session.createSQLQuery( "delete from MAIN_TABLE" ).executeUpdate();
| @@ -227,6 +226,7 @@ public void testRevisionsCounts() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
| @SkipForDialect(value = CockroachDB192Dialect.class, comment = "requires serial_normalization=sql_sequence setting") | |||
There was a problem hiding this comment.
it seems this should be
@SkipForDialects( value = {
@SkipForDialect(value = CockroachDB192Dialect.class, comment = "requires serial_normalization=sql_sequence setting"),
@SkipForDialect(value = Oracle8iDialect.class,
comment = "Oracle does not support identity key generation")
})
otherwise the
@SkipForDialect(value = Oracle8iDialect.class,
comment = "Oracle does not support identity key generation")
of the test class declaration is ignored and the test fails on Oracle.
There was a problem hiding this comment.
done, didn't realize the method-level and class-level annotations could conflict.
There was a problem hiding this comment.
Thank you for your comments! I have addressed them.
Out of curiosity, why is SkipForDialect preferred instead of RequiresDialectFeature?
My reasoning for using RequiresDialectFeature is that a later version of CockroachDB will add support for the NoWait feature, and the dialect will return true for supportsNoWait(). Then the tests marked with @RequiresDialectFeature(DialectChecks.SupportNoWait.class) will automatically start running with that new dialect version.
If we use @SkipForDialect( value = CockroachDB192Dialect.class ), then we have to remember to come back to every test that is marked with this annotation, and manually determine which features the test uses and decide if it should be enabled for the new dialect.
| @@ -137,7 +137,7 @@ protected void doBind(CallableStatement st, X value, String name, WrapperOptions | |||
| CharacterStream.class, | |||
| options | |||
| ); | |||
| st.setCharacterStream( name, characterStream.asReader(), characterStream.getLength() ); | |||
| st.setCharacterStream( name, characterStream.asReader() ); | |||
| @@ -61,6 +61,7 @@ | |||
| private static final Logger log = Logger.getLogger( LockTest.class ); | |||
|
|
|||
| @Test | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
| @@ -67,6 +67,7 @@ public void releaseResources() { | |||
|
|
|||
| @Test(timeout = 1000 * 30) //30 seconds | |||
| @TestForIssue(jiraKey = "HHH-11617") | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
| @@ -145,6 +145,7 @@ public void testNativeSql() { | |||
| } | |||
|
|
|||
| @Test | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
| @@ -170,6 +171,7 @@ public void testPessimisticForcedIncrementOverall() { | |||
| } | |||
|
|
|||
| @Test | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
| @@ -201,6 +201,7 @@ public void testRefreshWithExplicitLowerLevelLockMode() { | |||
|
|
|||
| @Test | |||
| @TestForIssue(jiraKey = "HHH-12257") | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
| @@ -54,6 +56,7 @@ public void createTestData() { | |||
| @Test | |||
| @RequiresDialect({ Oracle8iDialect.class, PostgreSQL81Dialect.class, | |||
| SQLServer2005Dialect.class } ) | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
| @@ -79,6 +82,7 @@ public void testNoWait() | |||
|
|
|||
| @Test | |||
| @RequiresDialect({ Oracle8iDialect.class, PostgreSQL95Dialect.class }) | |||
| @RequiresDialectFeature(DialectChecks.SupportNoWait.class) | |||
| session.createSQLQuery( getDialect().getDropTableString( "MATERIAL_RATINGS" ) ).executeUpdate(); | ||
| session.createSQLQuery( getDialect().getDropTableString( "BUILDING_RATINGS" ) ).executeUpdate(); | ||
| session.createSQLQuery( getDialect().getDropTableString( "ASSOCIATION_TABLE" ) ).executeUpdate(); | ||
| session.createSQLQuery( getDialect().getDropTableString( "MAIN_TABLE" ) ).executeUpdate(); |
| @@ -227,6 +226,7 @@ public void testRevisionsCounts() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
| @SkipForDialect(value = CockroachDB192Dialect.class, comment = "requires serial_normalization=sql_sequence setting") | |||
There was a problem hiding this comment.
done, didn't realize the method-level and class-level annotations could conflict.
|
Thanks Rafi for the changes, |
|
Thanks Rafi, I'm closing this PR in favour of #3459 where I rebased the PR on top of master, squashed the commits and fixed an issue with a test recently added. |
This adds a CockroachDB dialect and build configuration for testing with CockroachDB.
Tests that use features that CockroachDB does not support yet are annotated with SkipForDialect.