samredai commented on a change in pull request #3252:
URL: https://github.com/apache/iceberg/pull/3252#discussion_r725422479



##########
File path: 
hive-metastore/src/test/java/org/apache/iceberg/hive/TestHiveCatalog.java
##########
@@ -118,6 +122,37 @@ public void testCreateTableWithCaching() throws Exception {
     }
   }
 
+  @Test
+  public void testNoArgConstructor() throws Exception {
+    Catalog catalog = new HiveCatalog();
+  }
+
+  @Test
+  public void testSetConf() throws Exception {
+    HiveCatalog catalog = new HiveCatalog();
+    Configuration hadoopConfiguration = new Configuration();
+    catalog.setConf(hadoopConfiguration);
+  }
+
+  @Test
+  public void testInitialize() throws Exception {
+
+    HiveCatalog catalog = Mockito.spy(HiveCatalog.class);
+    Configuration mockHadoopConfiguration = Mockito.mock(Configuration.class);
+    CachedClientPool mockCachedClientPool = 
Mockito.mock(CachedClientPool.class);
+
+    Mockito.doNothing().when(catalog).setConf(any());
+    Mockito.doReturn(mockCachedClientPool).when(catalog)
+            .makeCachedClientPool(any(), any());
+    Mockito.doReturn("test").when(mockHadoopConfiguration).get(any());
+
+    catalog.setConf(mockHadoopConfiguration);
+    catalog.initialize("test", Collections.<String, String>emptyMap());
+
+    Mockito.verify(catalog, Mockito.times(1)).makeCachedClientPool(any(), 
any());
+    Assert.assertEquals(catalog.name(), "test");

Review comment:
       Great, thanks for the guidance here! Agreed the mocks come with more 
overhead than needed. Playing with this some more it seems that `initialize` 
can't work without a `conf` so scenario `1.` might be the only acceptable 
sequence, even if `uri` and `warehouse` are provided. What are your thoughts on 
adding a runtime error in `initialize` if `conf` is null?
   ```java
   if (this.conf == null){
     throw new RuntimeException("Missing configuration, a configuration must be 
set using setConf before initializing a HiveCatalog");
   }
   ```
   Then the tests can be as simple as checking that `initialize` succeeds with 
a `conf` and raises an exception without it.




-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to