Michael Brown has posted comments on this change. Change subject: IMPALA-3491: Use unique_database fixture in test_shell_commandline.py. ......................................................................
Patch Set 2: (6 comments) This looks pretty good! Let's set up a private Jenkins build, especially since you're introducing new serial tests, but also to get you introduced to that flow. http://gerrit.cloudera.org:8080/#/c/3301/2//COMMIT_MSG Commit Message: PS2, Line 20: Additonal Additional http://gerrit.cloudera.org:8080/#/c/3301/2/tests/shell/test_shell_commandline.py File tests/shell/test_shell_commandline.py: PS2, Line 51: fq_table_name = '.'.join([unique_database, request.node.name]) We need to make sure that request.node.name is a valid Impala identifier, because the length constraints of a valid Python identifier are longer than those of Impala. There's already a helper method somewhere in tests.common that tells if you if the string is a valid identifier. You also need to decide how to handle the (unlikely but possible) case that the request.node.name is an invalid Impala identifier. unique_database does this sort of handling, too, so you can look there for example. PS2, Line 53: request.instance.execute_query(stmt) We want this to succeed in all cases, and we should fail if it doesn't. There's a execute_query_expect_success() method you can use instead for this purpose. PS2, Line 74: request.instance.execute_query(stmt) Same thing here: we want this to succeed. PS2, Line 78: class TestImpalaShell(ImpalaTestSuite): Can you double check with Ishaan that making this inherit now is OK? Since you removed teardown_class and setup_class it seems safe, but I want to make sure we didn't miss something here. PS2, Line 143: @pytest.mark.execute_serially : def test_kerberos_option(self): Did you ever figure out why this must be serial? -- To view, visit http://gerrit.cloudera.org:8080/3301 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Icdcb04a99c0907fc1ba56baa2497fafb33b0e34e Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: David Knupp <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
