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 7: Funnily enough, building Impala LZO did fail the first time I tried to verify this (because I'd forgotten to rebase first): http://github.mtv.cloudera.com/gist/dknupp/7b30d18f6a71a6174021 ...but that caused the whole build to fail. For what it's worth. I think my understanding of Dan's comment is slightly different. He mentioned "the scenario where we rename the lzo so for some reason and lose test coverage because of that." Which implies to me that building LZO is successful, but that the assumption being used to check for LZO (namely, that ${IMPALA_LZO}/build/libimpalalzo.so exists) no longer holds true, so we wind up skipping a bunch of tests that should have been run. Did I completely get that wrong? I'm not sure what moving the logic for that to a separate script would buy us. (But perhaps I'm missing your point Tim?) Maybe I just need to be less specific in _is_impalalzo_present(). Maybe if the directory ${IMPALA_LZO} exists, that's good enough to run LZO tests. If there's been a silent build failure for some reason, tests will still fail. Only if ${IMPALA_LZO} isn't a directory at all, then it's safe to skip the tests, with the assumption at that point being that the tests are being run by someone (an EXTERNAL someone) who didn't have Impala LZO because they only have access to the ASF repo. -- 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: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: David Knupp <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Internal Jenkins 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: No
