> On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java > > Lines 680 (patched) > > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line680> > > > > Do you need to insert data for this test case? I don't think is > > necessary. Inserts are expensive in Hive, so removing it will save time on > > the test execution.
I will remove it > On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java > > Lines 682 (patched) > > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line682> > > > > Code convention: separate if and ( by a space. > > > > Btw, I noticed that this test case has two different test paths > > depending on the value of this flag. I don't see this a good practice when > > writing test cases because if you change the flag for an unknown reason, > > then you will never execute the other test case and you won't know if it > > fails. > > > > I'd rather write another test case for the grant option enabled instead > > of putting logic conditions in test cases. I will split the test into two test cases: one for positive and one for negative > On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java > > Lines 710-717 (patched) > > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line710> > > > > This test case is a negative test case and it can be put in different > > method. will split it out > On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java > > Lines 716-717 (patched) > > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line716> > > > > How do we validate that the error was not authorized and it was not a > > syntax error? will use HiveSqlException > On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java > > Lines 725 (patched) > > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line725> > > > > This line fails immediatly, but instead of throwing a test case error, > > it goes to the finally() clause and finish with the test successfully. why do you think this fails? It should work. > On Sept. 20, 2018, 2:20 p.m., Sergio Pena wrote: > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java > > Lines 733 (patched) > > <https://reviews.apache.org/r/68779/diff/1/?file=2090407#file2090407line733> > > > > Aren't we running this test case in other parts of this class? Seems > > redundant to test that the ALTER TABLE fails with a table that does not > > exist. > > > > I prefer to see different methods for each test case you're trying to > > validate instead of putting several into one method. I will remove it - Na ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68779/#review208796 ----------------------------------------------------------- On Sept. 20, 2018, 4:24 a.m., Na Li wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68779/ > ----------------------------------------------------------- > > (Updated Sept. 20, 2018, 4:24 a.m.) > > > Review request for sentry, Arjun Mishra, kalyan kumar kalvagadda, and Sergio > Pena. > > > Bugs: sentry-2409 > https://issues.apache.org/jira/browse/sentry-2409 > > > Repository: sentry > > > Description > ------- > > add code to get table name and DB name based on token children size > > > Diffs > ----- > > > sentry-binding/sentry-binding-hive/src/main/java/org/apache/sentry/binding/hive/HiveAuthzBindingHook.java > 6731d1a > > sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/dbprovider/TestOwnerPrivileges.java > 55a79ee > > > Diff: https://reviews.apache.org/r/68779/diff/1/ > > > Testing > ------- > > unit tests in TestOwnerPrivileges passed > > > Thanks, > > Na Li > >
