Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/23045 )
Change subject: Add REST API integration tests ...................................................................... Patch Set 11: (7 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'm curious what were the criteria to understand what sub/scenarios to cove I think there might be some confusion about what the existing tests cover. We have three distinct test layers: rest_catalog-test.cc - Basic REST functionality (no auth) spnego_rest_catalog-test.cc - SPNEGO auth with mini KDC RestApiAuthzITest in master_authz-itest.cc - Full REST+Ranger+KDC integration The REST API authorization tests validate the HTTP/JSON/SPNEGO code path, which is completely separate from the RPC client path. The authorization logic is the same, but the integration stack (SPNEGO negotiation, JSON serialization, principal mapping) is different and needs end-to-end testing. 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), > Why to have RestApiAuthzITest as a parameterized test if there is just one The parameterization is required by the test infrastructure. The SetUp() method calls SetUpCluster(GetParam()) which expects a harness parameter. This pattern is consistent throughout the file, even MasterAuthzITest uses the same single-parameter pattern. Removing the parameterization would require redesigning the entire test base class architecture. http://gerrit.cloudera.org:8080/#/c/23045/10/src/kudu/integration-tests/master_authz-itest.cc@1072 PS10, Line 1072: const > nit: 'const string&' or 'const auto&' avoid copying and to show 'table_id' Done http://gerrit.cloudera.org:8080/#/c/23045/10/src/kudu/integration-tests/master_authz-itest.cc@1086 PS10, Line 1086: AssertUnauthorizedResponse > Here and elsewhere: do you want the test to exit right at this point if the The current usage of AssertUnauthorizedResponse without NO_FATALS() is intentional and follows the correct testing pattern for these authorization tests. http://gerrit.cloudera.org:8080/#/c/23045/10/src/kudu/master/spnego_rest_catalog-test.cc File src/kudu/master/spnego_rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/23045/10/src/kudu/master/spnego_rest_catalog-test.cc@393 PS10, Line 393: ST_F(MultiMasterSpnegoTest, TestRequestsToFormerLeaderAfterElection) { > nit: move this to line 391 as of PS10 to become a comment for the whole tes Done http://gerrit.cloudera.org:8080/#/c/23045/10/src/kudu/master/spnego_rest_catalog-test.cc@432 PS10, Line 432: const > nit: make this 'const'? Done http://gerrit.cloudera.org:8080/#/c/23045/10/src/kudu/master/spnego_rest_catalog-test.cc@441 PS10, Line 441: "\"error\""); > nit: Do you really want to hardcode the formatting of the JSON message? If Done -- 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 16:34:43 +0000 Gerrit-HasComments: Yes
