David Knupp has posted comments on this change. Change subject: IMPALA-3491: Use unique_database fixture in test_last_ddl_time_update.py. ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/3104/1/tests/metadata/test_last_ddl_time_update.py File tests/metadata/test_last_ddl_time_update.py: Line 4: import pytest Since tests are no longer being marked with 'execute_serially', importing pytest no longer seems to be necessary. Line 7: from subprocess import call call no longer seems to be used anymore (since __cleanup no longer exists.) Line 33: def test_alter(self, vector, unique_database): More of a question than a comment. I've most often seen test code written in such a way that each individual test scenario -- e.g. dropping a table, changing the location of a table, renaming columns, etc. -- would get it's own separate function, which would be useful especially when reporting specific test failures. Is there a particular reason why many individual tests are all clumped under the umbrella test_alter() and test_insert() methods? It may simply be that the setup and teardown of the database is expensive enough that we wouldn't want to incur the cost before and after each test. Line 63: def test_insert(self, vector, unique_database): Again, a question more so than an actual request. It's not uncommon to specify input params in a python doc string, what they are, and where they are defined, e.g. from the Google convention: Args: arg1 (int): Description of arg1 arg2 (str): Description of arg2 Returns: retval: Description of return value (if any) Just specifying that unique_database is a fixture defined within conftest.py at the root tests might be a useful comment to make for any future readers of this code. Or is that overkill? Perhaps potential readers of this file are assumed to be familiar enough with the conventions of pytest to know the various places to look for unique_database if so needed. -- To view, visit http://gerrit.cloudera.org:8080/3104 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I97e96217301078d48584c51218345dc96f6853a6 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-HasComments: Yes
