Alex Behm 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 p Cleaned up imports. Line 7: from subprocess import call > call no longer seems to be used anymore (since __cleanup no longer exists.) Cleaned up imports. Line 33: def test_alter(self, vector, unique_database): > More of a question than a comment. I've most often seen test code written i I don't really know since I'm not the original author of this test, but to me it seems overkill to create a separate test for every different type of alteration. The overhead of database setup/teardown is an issue, but imo, the worse issue is that we'd need around 2x the number of test code lines. I'm not exactly an authority on good test code or practices, so we should consider revisiting our practices if you feel strongly we should change them. Line 63: def test_insert(self, vector, unique_database): > Again, a question more so than an actual request. It's not uncommon to spec I'm a little worried about comment overkill. Virtually all of our tests have exactly this signature or a signature without the unique_database. We'd need to add these comments in so many places that the comments seem to lose value. There are other ways of figuring out where the fixture comes from (git grep?). I'm happy you are asking these questions because it's another instance where you/Michael/Harrison should weigh in on our practices. If you feel the value of the comments exceed the burden then we should reconsider. -- 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: Alex Behm <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-HasComments: Yes
