zabetak commented on code in PR #4791:
URL: https://github.com/apache/hive/pull/4791#discussion_r1354900459
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/tools/schematool/TestSchemaToolForMetastore.java:
##########
@@ -467,6 +471,18 @@ public void testMetastoreDbPropertiesAfterUpgrade() throws
HiveMetaException, IO
validateMetastoreDbPropertiesTable();
}
+ @Test
+ public void testValidateHiveShortVersion() {
+ String hiveShortVersion = MetastoreVersionInfo.getShortVersion();
+ String hiveVersion = MetastoreVersionInfo.getVersion();
+ Assert.assertEquals(hiveVersion, StringUtils.join(hiveShortVersion,
"-SNAPSHOT"));
Review Comment:
```suggestion
Assert.assertEquals(hiveVersion.replace("-SNAPSHOT", ""),
hiveShortVersion);
```
This is to avoid the situation that we are in a release branch and
hiveVersion does not have the "-SNAPSHOT" suffix.
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/tools/schematool/TestSchemaToolForMetastore.java:
##########
@@ -467,6 +471,18 @@ public void testMetastoreDbPropertiesAfterUpgrade() throws
HiveMetaException, IO
validateMetastoreDbPropertiesTable();
}
+ @Test
+ public void testValidateHiveShortVersion() {
Review Comment:
Since we have three asserts it would be better to divide this in three
separate tests. The name of each test should denote what we are checking.
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/tools/schematool/TestSchemaToolForMetastore.java:
##########
@@ -467,6 +471,18 @@ public void testMetastoreDbPropertiesAfterUpgrade() throws
HiveMetaException, IO
validateMetastoreDbPropertiesTable();
}
+ @Test
+ public void testValidateHiveShortVersion() {
+ String hiveShortVersion = MetastoreVersionInfo.getShortVersion();
+ String hiveVersion = MetastoreVersionInfo.getVersion();
+ Assert.assertEquals(hiveVersion, StringUtils.join(hiveShortVersion,
"-SNAPSHOT"));
+ Assert.assertTrue(hiveVersion.startsWith(hiveShortVersion));
+ String fileName =
Stream.of("src/test/resources/sql/postgres/upgrade-3.1.3000-to-" ,
hiveShortVersion ,".postgres.sql").
+ collect(Collectors.joining());
+ File file = new File(fileName);
+ Assert.assertTrue(file.exists());
Review Comment:
I think that this test may be a bit redundant since if hiveShortVersion is
different, the metastore upgrade will fail when the relevant tests run.
I like the idea of having unit tests checking that upgrade and install
scripts are present for the current version but if we go into this direction we
should be better doing that for all supported databases and in a more
appropriate place (not inside this class).
##########
standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/tools/schematool/TestSchemaToolForMetastore.java:
##########
@@ -467,6 +471,18 @@ public void testMetastoreDbPropertiesAfterUpgrade() throws
HiveMetaException, IO
validateMetastoreDbPropertiesTable();
}
+ @Test
+ public void testValidateHiveShortVersion() {
+ String hiveShortVersion = MetastoreVersionInfo.getShortVersion();
+ String hiveVersion = MetastoreVersionInfo.getVersion();
Review Comment:
Every other tests in this class requires a database and makes calls to the
schema tool while these new tests don't.
Consider moving them elsewhere for instance in `TestMetastoreVersion`.
Creating a new `TestMetastoreVersionInfo` class would make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
