You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sentry.apache.org by kalyan kumar kalvagadda <kk...@cloudera.com> on 2017/04/19 16:40:52 UTC
Review Request 58536: SENTRY-1678 Add test class to test refactored
refactored config code
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58536/
-----------------------------------------------------------
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
Re: Review Request 58536: SENTRY-1678 Add test class to test
refactored refactored config code
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 58536: SENTRY-1678 Add test class to test
refactored refactored config code
Posted by Sergio Pena <se...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58536/#review177173
-----------------------------------------------------------
Another thing. We should change the JIRA subject to reflect what the patch is doing. This seems too generic.
- 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
>
>
Re: Review Request 58536: SENTRY-1678 Add test class to test
refactored refactored config code
Posted by Vamsee Yarlagadda <va...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58536/#review176966
-----------------------------------------------------------
Fix it, then Ship it!
sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/transport/testSentryHDFSClientTransportConfig.java
Lines 30 (patched)
<https://reviews.apache.org/r/58536/#comment250504>
Test class for SentryHDFSClientTransportConfig
sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/transport/testSentryHDFSClientTransportConfig.java
Lines 35 (patched)
<https://reviews.apache.org/r/58536/#comment250503>
Classes should start with CamelCase.
sentry-core/sentry-core-common/src/test/java/org/apache/sentry/core/common/transport/testSentryPolicyClientTransportConfig.java
Lines 35 (patched)
<https://reviews.apache.org/r/58536/#comment250505>
Same comment as above. CamelCase
- Vamsee Yarlagadda
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
>
>