impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Brown (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3491: Use unique database fixture in test ddl.py.
Date Mon, 29 Aug 2016 21:22:52 GMT
Michael Brown has posted comments on this change.

Change subject: IMPALA-3491: Use unique database fixture in test_ddl.py.
......................................................................


Patch Set 1:

(5 comments)

Eyes began to glaze over at last 10% of test_ddl.py. Here's some other comments for now.

http://gerrit.cloudera.org:8080/#/c/4155/1/testdata/workloads/functional-query/queries/QueryTest/alter-table.test
File testdata/workloads/functional-query/queries/QueryTest/alter-table.test:

PS1, Line 263: create external table t_part (i int) partitioned by (j int, s string)
             : location '$FILESYSTEM_PREFIX/test-warehouse/$DATABASE.db/t_part_tmp';
             : alter table t_part add partition (j=cast(2-1 as int), s='2012');
             : alter table t_part add if not exists partition (j=1, s='2012');
             : alter table t_part add if not exists partition (j=1, s='2012/withslash');
             : alter table t_part add partition (j=1, s=substring('foo2013bar', 4, 8));
Will the test framework adequately fail the test if one of these reports an error? It matters
here, because unlike in some cases below, there's no SELECT at the end of this to implicitly
verify they all worked.


http://gerrit.cloudera.org:8080/#/c/4155/1/tests/metadata/test_ddl.py
File tests/metadata/test_ddl.py:

PS1, Line 234:     assert "ddl_test_db" not in self.all_db_names()
(I found this by searching for the removed members of TEST_DBS.)

  assert unique_database not in self.all_db_names()


PS1, Line 255:     unique_dabatase2 = unique_database + '2'
             :     self._create_db(unique_dabatase2, sync=True)
Clever, but it's unfortunate you have to do this. I'm not aware of pytest being able to use
the same fixture twice (or more) in a test.

Should we parametrize unique_database optionally to return a set of databases instead of just
one? It seems like it could use your incrementing numerical suffix scheme and be fine.


PS1, Line 328:     create_fn_stmt = "create function {0}.f() returns int "\
             :                      "location '{1}/libTestUdfs.so' symbol='NoArgs'"\
             :                      .format(unique_database, WAREHOUSE)
Tip for here and elsewhere: You can join strings across lines without \ if they are within
parens.

    create_fn_stmt = ("create function {0}.f() returns int "
                      "location '{1}/libTestUdfs.so' symbol='NoArgs'"
                      .format(unique_database, WAREHOUSE))


http://gerrit.cloudera.org:8080/#/c/4155/1/tests/metadata/test_hms_integration.py
File tests/metadata/test_hms_integration.py:

Line 55:   def test_sanity(self, vector):
Not your changes, but in case you want to improve: This test doesn't have many asserts. I
think it could be improved by replacing the self.client.execute() statements with self.execute_query_expect_success()
statement. L70 for sure is never verified.


-- 
To view, visit http://gerrit.cloudera.org:8080/4155
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idf667dd5e960768879c019e2037cf48ad4e4241b
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message