Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/23045 )
Change subject: Add REST API integration tests ...................................................................... Patch Set 10: (11 comments) Overall looks good, just a few questions and nits. 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 cover for the REST API as well. As one can see, master_authz-itest.cc contains numerous scenarios for authz logic already. >From one side, ideally there might be 1-1 correspondence for the cases when >the REST API isn't itself a restriction/limitation (i.e. when the REST API >doesn't allow to implement particular scenarios due to API limitations >themselves). >From the other side, if the calls from REST API are transparently forwarded to >the authz logic that already has all the coverage in master_authz-itest.cc, >why to add anything that resembles already existing non-REST API scenarios? What do you think? 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 value for the parameter? http://gerrit.cloudera.org:8080/#/c/23045/10/src/kudu/integration-tests/master_authz-itest.cc@1072 PS10, Line 1072: string nit: 'const string&' or 'const auto&' avoid copying and to show 'table_id' it's not going to change below? 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 assertion triggers or the idea is to continue with the rest of the code below? If the former, maybe wrap AssertUnauthorizedResponse with NO_FATALS()? 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@316 PS8, Line 316: e_ptr<MiniKdc> kdc_; > All test methods are prefixed with 'Test', this is a consistent pattern used > across thousands of test cases in the project At first glance it might seem to be a consistent pattern, but that's not true: 1) There are many test cases without 'Test' prefix in the name in the project. E.g., even under the 'src/kudu/master' directory there are at least SysCatalogTest.LoadClusterID, SysCatalogEncryptedCertTest.LoadCertAuthorityInfoEncrypted, etc. 2) In many test scenarios, 'Test' isn't a _prefix_, but a _suffix_ for the scenario name, and that's another funny bit about this questionable naming scheme. E.g., RowOperationsTest.FuzzTest, CodegenTest.ObservablesTest, HdrHistogramTest.SimpleTest, MetricsTest.SimpleCounterMergeTest and many more. > Removing the 'Test' prefix from just these methods would break consistency > with the rest of the codebase and go against the established coding standards. Sure, feel free to keep this as-is, it was just a nit. However, there isn't much consistency in this regard across the Kudu's codebase, and there isn't much value in keeping this 'standard' since it's not a standard de facto and such test naming pattern does not make much sense. Also, there isn't anything that would rely on having 'Test' both in the name of the test suite and test scenario, so there isn't a risk of breaking anything if drifting away from the 'Test.Test' stutter in new tests, AFAIK. http://gerrit.cloudera.org:8080/#/c/23045/8/src/kudu/master/spnego_rest_catalog-test.cc@321 PS8, Line 321: ST_F(M > compiling it once and reusing it across the loop iterations is more efficient. OK, but there isn't any loop here as of PS8. That would make sense if the execution flow would go over the code section multiple times. As of PS8, the code in this scenario is executed only once during the lifecycle of the test process, unless running the same scenario in a loop with --gtest_repeat (and that's not used by any existing upstream Kudu CI component at least for this test, AFAIK). So, the only case when having 'static' here in PS8 gives you any benefit if manually running the test with --gtest_repeat=N flag, where N > 1. I see that in PS10 it's now enclosed into ASSERT_EVENTUALLY scope, so there it makes more sense. 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) { > I added an extra, invalid path and implemented the loop. Thanks a lot! 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); > Done Thank you for adding one extra test scenario! 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: // Test behavior when client makes requests to former leader after leadership change nit: move this to line 391 as of PS10 to become a comment for the whole test scenario? http://gerrit.cloudera.org:8080/#/c/23045/10/src/kudu/master/spnego_rest_catalog-test.cc@432 PS10, Line 432: vector nit: make this 'const'? http://gerrit.cloudera.org:8080/#/c/23045/10/src/kudu/master/spnego_rest_catalog-test.cc@441 PS10, Line 441: "{\"error\":\"Master is not the leader\"}" nit: Do you really want to hardcode the formatting of the JSON message? If you are sure the formatting it's not ever going to change, that's totally fine, but otherwise consider string matching that wouldn't break even if extra space(s) appeared around the strings. -- 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: 10 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: Wed, 20 Aug 2025 02:39:19 +0000 Gerrit-HasComments: Yes
