Jason Fehr has posted comments on this change. ( http://gerrit.cloudera.org:8080/23319 )
Change subject: IMPALA-14333: [part 1] Make all pytest compatible with Python3 ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/23319/1/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/23319/1/tests/authorization/test_ranger.py@816 PS1, Line 816: def _run_query_with_client(self, query, impala_client, expect_success): To me, this function does not clarify or simplify the code. Instead, the code would be clearer if either self.execute_query_expect_success or self.execute_query_expect_failure was called directly in the tests below. http://gerrit.cloudera.org:8080/#/c/23319/1/tests/authorization/test_ranger.py@892 PS1, Line 892: user_client = self.create_impala_client(user=OWNER_USER) Can both of these lines use the with self.create_impala_client() as client_name syntax instead of manually closing? Same question in the below tests that have the same pattern. http://gerrit.cloudera.org:8080/#/c/23319/1/tests/custom_cluster/test_saml2_sso.py File tests/custom_cluster/test_saml2_sso.py: http://gerrit.cloudera.org:8080/#/c/23319/1/tests/custom_cluster/test_saml2_sso.py@111 PS1, Line 111: SSO_ARGS_WITH_GROUP_FILTER = (SSO_ARGS + " " Since this line is being modified, why not switch to using .format? SSO_ARGS_WITH_GROUP_FILTER = "{} --saml2_group_filter=group1,group2 --saml2_group_attribute_name=eduPersonAffiliation".format(SSO_ARGS) http://gerrit.cloudera.org:8080/#/c/23319/1/tests/custom_cluster/test_saml2_sso.py@240 PS1, Line 240: return resp I'm curious why this additional return variable is needed? http://gerrit.cloudera.org:8080/#/c/23319/2/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/23319/2/tests/shell/test_shell_interactive.py@192 PS2, Line 192: def _convert_bytes(self, bytes): Duplicate of https://gerrit.cloudera.org/c/23319/2/tests/common/impala_test_suite.py#1825 -- To view, visit http://gerrit.cloudera.org:8080/23319 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I401a93b6cc7bcd17f41d24e7a310e0c882a550d4 Gerrit-Change-Number: 23319 Gerrit-PatchSet: 2 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Thu, 21 Aug 2025 17:29:20 +0000 Gerrit-HasComments: Yes
