Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/12124 )
Change subject: IMPALA-8071: Initial unified backend test framework ...................................................................... Patch Set 3: (4 comments) Updated this to incorporate validation that tests are covered by a test filter pattern. http://gerrit.cloudera.org:8080/#/c/12124/3/be/src/service/unified-betest-main.cc File be/src/service/unified-betest-main.cc: http://gerrit.cloudera.org:8080/#/c/12124/3/be/src/service/unified-betest-main.cc@23 PS3, Line 23: using namespace std; > In theory we don't like importing the whole namespace :). It's probably not Removed. It turns out this was unnecessary (common/names.h gets me what I care about). http://gerrit.cloudera.org:8080/#/c/12124/3/be/src/util/CMakeLists.txt File be/src/util/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/12124/3/be/src/util/CMakeLists.txt@106 PS3, Line 106: #decompress-test.cc > Maybe remove these comments? It might be more helpful to document the reaso Yes, makes sense, done. http://gerrit.cloudera.org:8080/#/c/12124/3/be/src/util/CMakeLists.txt@149 PS3, Line 149: ADD_UNIFIED_BE_LSAN_TEST(benchmark-test "BenchmarkTest.*") > I was thinking about whether people would "accidentally" forget to add or u Yes, I am worried that the patterns might get stale or people might forget to add the right pattern. I added a check that verifies that all of the tests that are in the unified backend executable are covered by some pattern. So, if someone creates a test and it is linked into the binary but there is no pattern that would run it, the build fails. Basically, if I take the union of all the patterns, then running the unified executable with --gtest_list_tests should produce the same list of tests with or without the patterns. There are two types of mistakes that this doesn't detect: 1. Dead patterns that don't match any tests. 2. Overlapping patterns for different tests (i.e. some test might get executed twice) We might want checks for these later. http://gerrit.cloudera.org:8080/#/c/12124/3/bin/gen-backend-test-script.sh File bin/gen-backend-test-script.sh: http://gerrit.cloudera.org:8080/#/c/12124/3/bin/gen-backend-test-script.sh@18 PS3, Line 18: > Maybe document what this script does and the arguments just in a couple of Done -- To view, visit http://gerrit.cloudera.org:8080/12124 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia03ef38719b1fbc0fe2025e16b7b3d3dd4488842 Gerrit-Change-Number: 12124 Gerrit-PatchSet: 3 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 07 Feb 2019 01:07:26 +0000 Gerrit-HasComments: Yes
