impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-3491: Use unique database fixture in test last ddl time update.py.
Date Fri, 03 Jun 2016 16:28:07 GMT
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 <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message