Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23045 )
Change subject: Add REST API integration tests ...................................................................... Patch Set 11: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/23045/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/23045/10//COMMIT_MSG@9 PS10, Line 9: Additions to master_authz-itest.cc > I think there might be some confusion about what the existing tests cover. Right -- my point was exactly about the integration-level coverage: what were the criteria to build this particular set of scenarios? In other words, how to gauge with some level of confidence that with this set of the REST API integration scenarios there is enough (or still not enough?) coverage for the code paths that aren't (and cannot be) covered by the RPC client paths? The description doesn't have the "why" part, it just provides the list of the integration scenarios added. Since I couldn't derive the reasoning from list, I'm trying to get better understanding of the "why" part by asking this question. Thanks! http://gerrit.cloudera.org:8080/#/c/23045/10/src/kudu/integration-tests/master_authz-itest.cc File src/kudu/integration-tests/master_authz-itest.cc: http://gerrit.cloudera.org:8080/#/c/23045/10/src/kudu/integration-tests/master_authz-itest.cc@1055 PS10, Line 1055: ::testing::Values(kRanger), > The parameterization is required by the test infrastructure. > Removing the parameterization would require redesigning the entire test base > class architecture. The base class MasterAuthzITestBase isn't parameterized. Removing the inheritance of the newly added RestApiAuthzITest from ::testing::WithParamInterface<HarnessEnum>, calling SetUpCluster(kRanger), and switching from TEST_P to TEST_F macro for RestApiAuthzITest is all what's needed to make it non-parameterized. Nothing in the already existing code needs to be touched, no '... redesigning the entire test base class architecture ...' is required. To get a better idea on what's going on, I took a closer look at the history of this file and the parameterized approach turned to be just a remnant from the times when there were multiple authz providers (i.e. Sentry in addition to Ranger). If needed, check https://gerrit.cloudera.org/#/c/15880/ for more context. So, I'm fine with keeping this new test suite parameterized from the sake of uniformity, but there isn't any other reason to have a parameter, IIUC. -- To view, visit http://gerrit.cloudera.org:8080/23045 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ifd3ff0dfd67cbc2b5ed0454372dd2bcea71e2ba3 Gerrit-Change-Number: 23045 Gerrit-PatchSet: 11 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Thu, 21 Aug 2025 19:25:25 +0000 Gerrit-HasComments: Yes
