----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63958/#review191546 -----------------------------------------------------------
I see you have different test cases per file and not per method, and I assume is because you call TestHDFSIntegrationBase.setup() on the @BeforeClass part. Why not calling it in the @Before part instead and put all the tests cases into one single file? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestHmsNotificationProcessing.java Lines 48 (patched) <https://reviews.apache.org/r/63958/#comment269390> method should start with lower case sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestHmsNotificationProcessing.java Lines 96-107 (patched) <https://reviews.apache.org/r/63958/#comment269389> If these tests are commented because they're not used, then we should remove them. sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestHmsNotificationProcessingWithOutHdfsSync.java Lines 46 (patched) <https://reviews.apache.org/r/63958/#comment269391> method should start with lower case sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestHmsNotificationProcessingWithOutSyncOnCreate.java Lines 47 (patched) <https://reviews.apache.org/r/63958/#comment269392> method should start with lower case sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java Lines 198 (patched) <https://reviews.apache.org/r/63958/#comment269387> Why was this moved out of the createSentry() method? Isn't createSentry called only once on the @BeforeClass method? is it needed? sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java Lines 52-54 (patched) <https://reviews.apache.org/r/63958/#comment269388> Can we configure these properties in the same way other properties done by using global variables instead? This abstract test class needs some redesign and I don't like global variables, but to keep the configuration consistent, can we do it for these? Or, why aren't they just set by defalut on the TestHDFIntegrationBase? - Sergio Pena On Nov. 20, 2017, 4:25 p.m., kalyan kumar kalvagadda wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63958/ > ----------------------------------------------------------- > > (Updated Nov. 20, 2017, 4:25 p.m.) > > > Review request for sentry, Na Li and Sergio Pena. > > > Bugs: SENTRY-2034 > https://issues.apache.org/jira/browse/SENTRY-2034 > > > Repository: sentry > > > Description > ------- > > Currently, there are no e2e tests that test the functionality of pulling the > notifications from HMS and processing them. Which include updating the update > the permissions based on the HMS updates. > > > Diffs > ----- > > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestDbPrivilegeCleanupOnDrop.java > 21383046f8a5b7386cfe9f6f62c0119542a73382 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestHmsNotificationProcessing.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestHmsNotificationProcessingBase.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestHmsNotificationProcessingWithOutHdfsSync.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestHmsNotificationProcessingWithOutSyncOnCreate.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestHmsNotificationProcessingWithOutSyncOnDrop.java > PRE-CREATION > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationAdvanced.java > 95bbaeb5a678ee4b1e7adaab9985346027ecfc24 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationBase.java > 3c8b70ec3db59a4e6de6a7fb8b1509c4fe228c25 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationEnd2End.java > 645fc354c3b4e5b21456ee9a471253ffa4624f17 > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegrationWithHA.java > a911e2f50f63db6a99996ce44a74ad6bcc2a82b0 > > > Diff: https://reviews.apache.org/r/63958/diff/1/ > > > Testing > ------- > > Made sure that new tests added and all the older tests pass. > > > Thanks, > > kalyan kumar kalvagadda > >
