----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58536/#review177172 -----------------------------------------------------------
sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/transport/testSentryHDFSClientTransportConfig.java Lines 33 (patched) <https://reviews.apache.org/r/58536/#comment250698> 'HDFS constants will return ...' sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/transport/testSentryHDFSClientTransportConfig.java Lines 63-89 (patched) <https://reviews.apache.org/r/58536/#comment250699> You can test exceptions with less code. try { transportConfig.getSentryServerRpcAddress(conf); Assert.assertFalse(true); // This will make the test fail because no exception is thrown. } catch (MissingConfigurationException e) { /* do nothing - test passed */ } sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/transport/testSentryHDFSClientTransportConfig.java Lines 122 (patched) <https://reviews.apache.org/r/58536/#comment250700> This method is changing values of two static variables (conf and properties). If in the future we add other tests that use default values, then this might alter our test cases because the static variable will have different values. - Why do you need the static properties variable here? You can set the values directly to the Configuration object instead. - Why don't return a Configuration object instead of using a static variable? The static is not used anywhere else, and the new values are only for one test case. Same questoins are for testSentryPolicyClientTransportConfig.java sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/transport/testSentryPolicyClientTransportConfig.java Lines 33 (patched) <https://reviews.apache.org/r/58536/#comment250701> 'policy constants will return ...' sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/transport/testSentryPolicyClientTransportConfig.java Lines 63-90 (patched) <https://reviews.apache.org/r/58536/#comment250702> You can test exceptions with less code. try { transportConfig.getSentryServerRpcAddress(conf); Assert.assertFalse(true); // This will make the test fail because no exception is thrown. } catch (MissingConfigurationException e) { /* do nothing - test passed */ } - Sergio Pena On April 19, 2017, 4:40 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58536/ > ----------------------------------------------------------- > > (Updated April 19, 2017, 4:40 p.m.) > > > Review request for sentry, Alexander Kolbasov, Hao Hao, Na Li, Sergio Pena, > Vamsee Yarlagadda, and Vadim Spector. > > > Bugs: SENTRY-1678 > https://issues.apache.org/jira/browse/SENTRY-1678 > > > Repository: sentry > > > Description > ------- > > Added test classes to test the implmentations of > SentryClientTransportConfigInterface for policy and hdfs configuraitons. > > > Diffs > ----- > > > sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/transport/testSentryHDFSClientTransportConfig.java > PRE-CREATION > > sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/transport/testSentryPolicyClientTransportConfig.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/58536/diff/1/ > > > Testing > ------- > > > Thanks, > > kalyan kumar kalvagadda > >
