-
Notifications
You must be signed in to change notification settings - Fork 96
Replace DBSelectionExtension with annotations #1828
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I gave it a quick look and left a couple of minor changes requests. I will try to review it properly tomorrow.
I think I prefer the Jupiter convention for annotations.
I would prefer something like this:
@EnabledFor(POSTGRES)
@DisabledFor(DB2)
...ate-reactive-core/src/test/java/org/hibernate/reactive/GeneratedPropertySingleTableTest.java
Outdated
Show resolved
Hide resolved
hibernate-reactive-core/src/test/java/org/hibernate/reactive/annotations/README.md
Outdated
Show resolved
Hide resolved
hibernate-reactive-core/src/test/java/org/hibernate/reactive/annotations/TestingUtil.java
Outdated
Show resolved
Hide resolved
b0edece
to
e1eba5a
Compare
@DavideD I re-worked the logic to implement The critical classes are: DisableForDBTypeCondition and EnableForDBTypeCondition |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @blafond, it looks much nicer now.
There are some minor changes to apply, but looks good overall.
The test related to the annotation at a method level could be grouped in a single class:
public class EnableForMethodTest {
@Test
@EnableFor(value = POSTGRESQL)
public void testEnable() {
// Throw exception if this test is database is NOT POSTGRESQL
assertEquals( POSTGRESQL, DatabaseConfiguration.dbType() );
}
@Test
@EnableFor(value = POSTGRESQL)
@EnableFor(value = MYSQL)
public void testMultipleEnable() {
// Throw exception if this test is run with POSTGRESQL or MYSQL database
assertTrue( DatabaseConfiguration.dbType() == POSTGRESQL ||
DatabaseConfiguration.dbType() == MYSQL );
}
@Test
@EnableForGroup({
@EnableFor(value = MYSQL),
@EnableFor(value = POSTGRESQL)
})
public void testGroupEnable() {
// Throw exception if this test is run with POSTGRESQL or MYSQL database
assertTrue( DatabaseConfiguration.dbType() == POSTGRESQL ||
DatabaseConfiguration.dbType() == MYSQL );
}
// Add the tests with disabled
}
This way we have less test classes around.
We probably could group togethere the others as well, but it would require to use subclasses and I'm not sure it's worth the effort.
...core/src/test/java/org/hibernate/reactive/annotations/tests/DisableForMultipleClassTest.java
Outdated
Show resolved
Hide resolved
...ore/src/test/java/org/hibernate/reactive/annotations/tests/DisableForMultipleMethodTest.java
Show resolved
Hide resolved
...core/src/test/java/org/hibernate/reactive/annotations/tests/EnableForMultipleMethodTest.java
Outdated
Show resolved
Hide resolved
...-core/src/test/java/org/hibernate/reactive/annotations/tests/EnableForMultipleClassTest.java
Outdated
Show resolved
Hide resolved
hibernate-reactive-core/src/test/java/org/hibernate/reactive/annotations/DisableFor.java
Outdated
Show resolved
Hide resolved
...tive-core/src/test/java/org/hibernate/reactive/configuration/ReactiveConnectionPoolTest.java
Outdated
Show resolved
Hide resolved
...tive-core/src/test/java/org/hibernate/reactive/configuration/ReactiveConnectionPoolTest.java
Outdated
Show resolved
Hide resolved
...active-core/src/test/java/org/hibernate/reactive/schema/SchemaUpdateCockroachDBTestBase.java
Outdated
Show resolved
Hide resolved
...ive-core/src/test/java/org/hibernate/reactive/types/BasicTypesAndCallbacksForAllDBsTest.java
Outdated
Show resolved
Hide resolved
...eactive-core/src/test/java/org/hibernate/reactive/annotations/tests/DisableForClassTest.java
Outdated
Show resolved
Hide resolved
@blafond, could you also add the issue number in the title of the commits, please? It's quite helpful. Use this format, please:
Thanks |
Implemented your changes/suggestions and checked formatting as well in my changed files. Found a few more unnecessary lines/comments. |
fixes #1827