Tim Armstrong 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) This is looking good. Our hard drives will thank you. 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 too bad here to import the things we actually need. 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 reason they're not converted at the ADD_BE*_TEST invocations below. 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 update patterns here if they modified the tests. I guess we have to assume that people are going to manually run tests that they add, but it seems like someone could rename things, run the test suite and think everything was ok. I'm not sure that I"m sold on this idea myself, but maybe the wrapper should fail if no tests are selected by the pattern? That would catch most mistakes, I think. 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 lines? -- 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: Sat, 12 Jan 2019 00:03:15 +0000 Gerrit-HasComments: Yes
