David Knupp has posted comments on this change. Change subject: IMPALA-4001: qgen: add proof of concept tests for Query() objects ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/4081/1/tests/comparison/tests/fake_query.py File tests/comparison/tests/fake_query.py: PS1, Line 97: must at least a "must at least *contain* a" ...? "must at least *be* a" ...? PS1, Line 97: fram s/fram/from/ http://gerrit.cloudera.org:8080/#/c/4081/1/tests/comparison/tests/query_object_testdata.py File tests/comparison/tests/query_object_testdata.py: PS1, Line 64: DATASET So -- these are really test cases, yes? Could this just be called QUERY_TEST_CASES, or something. DATASET seems confusing, at least to me. http://gerrit.cloudera.org:8080/#/c/4081/1/tests/comparison/tests/test_query_objects.py File tests/comparison/tests/test_query_objects.py: PS1, Line 25: query_something I'm a little confused by this. query_something is a ... QueryTest object (namedtuple), right? From the DATASET list? Maybe a more descriptive variable name could be employed here, to help clarify some of the pytest magic going on. Line 53: @pytest.mark.parametrize('query_test', DATASET, ids=_idfn) How do you feel about moving all of the tests to the end of the file, rather than interspersing them with utility functions? -- To view, visit http://gerrit.cloudera.org:8080/4081 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ed1960430ae0af469986e33f88aecb6fa74e999 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Brown <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-HasComments: Yes
