[GitHub] [iceberg] kbendick commented on a diff in pull request #5037: Core: Implement BaseMetastoreCatalog.registerTable()

[GitBox <gi...@apache.org>]

kbendick commented on code in PR #5037:
URL: https://github.com/apache/iceberg/pull/5037#discussion_r920345031


##########
nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java:
##########
@@ -356,6 +360,81 @@ public void testListTables() {
     Assertions.assertThat(catalog.tableExists(TABLE_IDENTIFIER)).isTrue();
   }
 
+  private void testRegister(TableIdentifier identifier, String metadataVersionFiles) {

Review Comment:
   > I do not like the duplicated test code, and I think it would be good to have a general Catalog API test for checking the functionality of the new Catalog implementations. Something like TestCatalog but for all/most of the catalog API methods. We can do it in a different PR, but it would be good to have them. The test for registerTable() is a good candidate for these common tests.
   
   We do have `CatalogTests`, which is what `TestRESTCatalog` is built off of. It’s nice in that it has some methods for subclasses to declare if the catalog being tested supports certain features, so the tests can exit early if they don’t support an optional feature.
   
   For example if the catalog in question doesn’t support namespace properties.
   
   Might be good to see if that can be made to fit this need or can otherwise get some ideas from it. I believe Nessie also implements these tests.



-- 
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: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org