Tianyi Wang has posted comments on this change. Change subject: IMPALA-5425: Add test for validating input when setting query options ......................................................................
Patch Set 7: (5 comments) Thanks for the detailed review. I'd like to discuss in person if possible. http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc File be/src/service/query-options-test.cc: PS6, Line 137: }; : TestByteCaseSet(options, case_set_i64); : TestByteCaseSet(options, case_set_i32); : } : : // Test an enum testcase. May or may not accept integer : template<typename KeyType, typename EnumType> : void TestEnumCase(TQueryOptions& options, > Can you use a templatiezed class to store the test cases instead of a tuple Do you mean replacing the make_pair in L142 or replacing the make_tuple in L148? Line 171: }); > Could you use https://github.com/google/googletest/blob/master/googletest/d I'm not sure how these are related. I think value-parameterized-tests is used when you want to run a test multiple times, with a variable being different values in each run. Here it is about iterating through test cases (tuples for now) with different types. To be more specific, in the example given: class FooTest : public ::testing::TestWithParam<const char*> What should "const char*" be replaced with here? http://gerrit.cloudera.org:8080/#/c/7805/7/be/src/service/query-options-test.cc File be/src/service/query-options-test.cc: Line 79: auto ok = MakeOk(options, test_case.first); > I think it would be helpful to see that these are functions here. Can you r It is a lambda with no explicit typename. It could be replaced with a functor class though. I think to make it more like a function we can use CamelCase. I checked google C++ guide and didn't find a naming convention for lambda expressions. Line 81: auto lb = test_case.second.first; > Do these need to be auto? No. Will fix. Line 103: for (auto& value : common_value) { > const? Please check elsewhere, too. Will fix. -- To view, visit http://gerrit.cloudera.org:8080/7805 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7 Gerrit-PatchSet: 7 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tianyi Wang <tw...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com> Gerrit-HasComments: Yes
