JetBrains / Exposed

Kotlin SQL Framework
http://jetbrains.github.io/Exposed/
Apache License 2.0
8.05k stars 674 forks source link

EXPOSED-398 Gradle task testH2_v1 runs tests on version 2.2.224 #2110

Closed obabichevjb closed 2 weeks ago

obabichevjb commented 4 weeks ago

Here is the fix for the problem of the wrong H2 version inside h2_v1 tests (should be 1.4.200 but actually used 2.2.224)

I found out that the problem is in how dependencies are added during the call of testDb("h2_v1") function inside root build.gradle.kts. That function registers a new gradle task like tasks.register<Test>("test${db.name.capitalized()}") { ... }, but dependencies are registered outside it.

I don't know was it made intentionally or not, it looks like it's not a big problem (except the fact that every test configuration has all the db drivers) unless there are different versions of the same driver.

And this fix does not solve the problem of MySql8 configuration, that is actually using version 5.

obabichevjb commented 4 weeks ago

@bog-walk Hi, you mentioned in YT issue that ArrayColumnTypeTests.kt should be fixed using excludingH2Version1, because it will be broken for the H2v1.

Actually, there are more tests that fail with H2v1. With the last commit I made testH2_v1 green again, but it affected many test cases.

obabichevjb commented 3 weeks ago

@bog-walk @e5l Finally I made a quite large change set that should fix driver versions for H2_v1, MySql 5 (and probably MariaDB) databases.

As far as I understand everything correct, the initial problem is related to the loading of driver classes. It was working in the way to add the driver dependencies into every db specific test configuration.

It's easy to check. We can add something like Class.forName("org.h2.Driver").getDeclaredConstructor().newInstance() into a test and run it with common gradle task test. The test will be executed for different databases and inside every test the driver (in this case H2) would be created without problems.

I have not so much experience with gradle, but the solution I found in isolating driver dependencies among specific test configurations. The following code does it:

val driverConfiguration = configurations.create("${db.name}DriverConfiguration")
        dependencies {
            db.dependencies.forEach {
                driverConfiguration(it)
            }
        }

        classpath += files(driverConfiguration.resolve())

Here is a new configuration created and added to the classpath of the specific test task. But I'm not sure if it can be made better...

I took into account another PR https://github.com/JetBrains/Exposed/pull/1967/files and actually brought the fix for MySql database. I found out that MySql 5 and 8 drivers have also different driver class names.

And also we had an extra mechanism of excluding H2 v1 cases from tests (via excludingH2Version1 function).

As a result a new structure of TestDB is introduced. Now it has separated entities for different versions of databases like H2_V1, H2_V2, MYSQL_V5, MYSQL_V8. I hope that it will make easier to specify for particular tests which databases it should cover. Also extended the list of predefined groups of databases:

val allH2V1 = listOf(H2_V1, H2_V1_MYSQL)
val allH2V2 = listOf(H2_V2, H2_V2_MYSQL, H2_V2_PSQL, H2_V2_MARIADB, H2_V2_ORACLE, H2_V2_SQLSERVER)
val allH2 = allH2V1 + allH2V2
val allMySql = listOf(MYSQL_V5, MYSQL_V8)
val allMariaDB = listOf(MARIADB)
val allMySqlLike = allMySql + allMariaDB + listOf(H2_V2_MYSQL, H2_V2_MARIADB, H2_V1_MYSQL)
val allPostgres = listOf(POSTGRESQL, POSTGRESQLNG)
val allPostgresLike = allPostgres + listOf(H2_V2_PSQL)
val allOracleLike = listOf(ORACLE, H2_V2_ORACLE)
val allSqlServerLike = listOf(SQLSERVER, H2_V2_SQLSERVER)
val all = TestDB.entries.toList()

But it affects many tests and can be controversial, if you see any problems with it I'd be happy to know about them)

obabichevjb commented 3 weeks ago

Some of tests I marked with // TODO probably could be fixed for MySql to look at them later, because at the current moment it was not clear to me should it work for MySql, and if so for which versions. There not so many of them, but it should be fixed in the future anyway.

e5l commented 3 weeks ago

Please consider keeping such big renamings in a separate PR from logical changes. The PR is hard to review

obabichevjb commented 2 weeks ago

@bog-walk Thank you for the review, I merged it, but if you see any new problems here after resolving the comments, I can fix them with removing of isOldMySql