Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/23045 )
Change subject: Add REST API integration tests ...................................................................... Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc File src/kudu/master/spnego_rest_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@301 PS8, Line 301: InternalMiniClusterOptions opts; > nit: please add a comment to explain why it's necessary to override the def Done http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@316 PS8, Line 316: e_ptr<MiniKdc> kdc_; > nit for here and the rest of the scenarios: drop the 'Test' prefix for the This follows the established naming convention throughout the entire Kudu codebase. All test methods are prefixed with 'Test', this is a consistent pattern used across thousands of test cases in the project (e.g., TestCreateTableAuthorized, TestAuthzListTables, TestGetTablesOneTable, etc.). Removing the 'Test' prefix from just these methods would break consistency with the rest of the codebase and go against the established coding standards. http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@321 PS8, Line 321: ST_F(M > Why to have 'static' here? The 'static' keyword here ensures the regex is compiled only once instead of being recompiled on each test execution. Since the regex pattern never changes during the test, compiling it once and reusing it across the loop iterations is more efficient. http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@331 PS8, Line 331: > nit: even if regex matches (i.e. KuduRegex::Match() returns 'true'), it's a Done http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@335 PS8, Line 335: &buf)); : vector<string> matches; : ASSERT_TRUE(re.Match(buf.ToString(), &matches)); > This fails if leader election happens in the middle. That's possible on bu Done http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@345 PS8, Line 345: }); : } : : TEST_F(MultiMasterSpnegoTest, TestUnauthenticatedRequestsRejected) { : // Test that requests without authentication are properly rejected : const vector<string> paths = { "/api/v1/tables", "/api/v1/leader", "/api/v1/invalid" }; : : for (int i = 0; i < cluster_->num_masters(); i++) { : string master_addr = cluster_->mini_master(i)->bound_http_addr().ToString(); : : for (const auto& path : paths) { > Consider implementing this with less boilerplate: have a cycle where the UR I added an extra, invalid path and implemented the loop. http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@357 PS8, Line 357: faststring buf; > What happens if sending PUT or other improper types of requests to a partic The existing SpnegoRestCatalogTest already covers unauthenticated requests comprehensively. http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@378 PS8, Line 378: > What is being verified here? It would be great to have a small text blurb Done http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@381 PS8, Line 381: c.set_auth(CurlAuthType::SPNEGO); > I'd think of adding one extra test scenario to verify the behavior of the s 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: 9 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: Tue, 19 Aug 2025 10:05:45 +0000 Gerrit-HasComments: Yes
