David Knupp has posted comments on this change. Change subject: IMPALA-3898: Add a pytest skipif decorator based on presence of Impala LZO. ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/3782/2/tests/common/environ.py File tests/common/environ.py: Line 256: if impala_lzo_root is not None: > This returns True, False, or None. Can it just return True or False? It can be, but when comparing to None, I tend to use "is" and "is not" (with None being the default return value anytime one calls .get() on a dict.) This is a standard python idiom, and probably stems from PEP-8: https://www.python.org/dev/peps/pep-0008/#programming-recommendations "Comparisons to singletons like None should always be done with is or is not..." ...though it's admittedly not uncontested: http://stackoverflow.com/questions/7816363/if-a-vs-if-a-is-not-none I'm happy to change it if you feel it's better. PS2, Line 257: if os.path.isfile(os.path.join(impala_lzo_root, 'build', 'libimpalalzo.so')): : return True : return False > if X: Ugh. Thanks. This was mostly an indent error, but I see your point. If for some reason IMPALA_LZO is not set, I don't want to supply a made up value, such as an empty pair of quotes. And because os.path.join() throws an exception if you pass it None as a parameter, so I didn't want to just return os.path.isfile(...). How's this version? http://gerrit.cloudera.org:8080/#/c/3782/2/tests/conftest.py File tests/conftest.py: Line 188: # a StopIteration exception is thrown. This is most assuredly a bug in the > Should you catch only StopIteration exceptions, then? You're probably right. And my comment was kinda crappy -- thanks for pointing this out. There were actually two different exceptions, so I'm catching them both now. (The original main intent though was to make sure that we don't skip any test unless 100% certain that compression_codec was LZO, which any exception would have interfered with.) This is probably more correct. http://gerrit.cloudera.org:8080/#/c/3782/2/tests/data_errors/test_data_errors.py File tests/data_errors/test_data_errors.py: PS2, Line 41: why this is > DataErrorsTest/hdfs-scan-node-errors.test, online 75, references LZO. That table is on line 84 as well, so it appears to be two tests? 73 ==== 74 #---- QUERY 75 select count(*) from functional_text_lzo.bad_text_lzo 76 ---- ERRORS 77 Blocksize: 536870911 is greater than LZO_MAX_BLOCK_SIZE: 67108864 78 ---- RESULTS 79 5141 80 ---- TYPES 81 bigint 82 ==== 83 ---- QUERY 84 select count(field) from functional_text_lzo.bad_text_lzo 85 ---- ERRORS 86 Blocksize: 536870911 is greater than LZO_MAX_BLOCK_SIZE: 67108864 87 ---- RESULTS 88 5141 89 ---- TYPES 90 bigint 91 ==== Admittedly, my understanding of of the interactions between pytest and those .test files and the various workloads/datasets is still murky at best, and I was mainly trying to get this change checked in "quickly" for the ASF effort. FWIW, I think there's actually something screwy about this test. As it is, this test mostly just xfails, whether or not Impala LZO is present. And the only tests that pass are exactly the same ones that fail when LZO isn't present. With LZO: 20 passed, 88 deselected, 124 xfailed in 38.16 seconds http://github.mtv.cloudera.com/gist/dknupp/dad53eee04a8d61f4342 Without LZO: 20 failed, 88 deselected, 124 xfailed in 6.69 seconds http://github.mtv.cloudera.com/gist/dknupp/d5e6e747b9619500866f So we're not losing anything by skipping it altogether. If you want me to take the time to sort this other business out, I will, but it seemed to outside of the scope of getting over the ASF hump. -- To view, visit http://gerrit.cloudera.org:8080/3782 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If61a7799205cd00d440196303a42db32c522f5b1 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: David Knupp <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
