Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17577 )
Change subject: [rest] add integration test ...................................................................... Patch Set 43: (7 comments) http://gerrit.cloudera.org:8080/#/c/17577/43/src/kudu/integration-tests/rest_server-itest.cc File src/kudu/integration-tests/rest_server-itest.cc: http://gerrit.cloudera.org:8080/#/c/17577/43/src/kudu/integration-tests/rest_server-itest.cc@50 PS43, Line 50: CHECK_OK Why not to use ASSERT_OK() here? http://gerrit.cloudera.org:8080/#/c/17577/43/src/kudu/integration-tests/rest_server-itest.cc@66 PS43, Line 66: KuduTableCreator* table_creator In Kudu we tend not using raw pointers in the code: consider wrapping this into std::unique_ptr() http://gerrit.cloudera.org:8080/#/c/17577/43/src/kudu/integration-tests/rest_server-itest.cc@71 PS43, Line 71: KUDU_CHECK_OK Here and above: would ASSERT_OK() work here? What's the reason using KUDU_CHECK_OK() instead of ASSERT_OK()? http://gerrit.cloudera.org:8080/#/c/17577/43/src/kudu/integration-tests/rest_server-itest.cc@115 PS43, Line 115: ASSERT_OK(curl.FetchURL(url, &response)); So, how do we know that the web server returned proper results? I guess that should be the actual work done in this test scenario after verifying the server returned code 200, no? http://gerrit.cloudera.org:8080/#/c/17577/43/src/kudu/integration-tests/rest_server-itest.cc@124 PS43, Line 124: ASSERT_OK(curl.FetchURL(url, &response)); Ah, that's interesting: why creation of the table done via PostToURL, but deletion -- via FetchURL? http://gerrit.cloudera.org:8080/#/c/17577/43/src/kudu/integration-tests/rest_server-itest.cc@125 PS43, Line 125: } I think this test scenario should verify that after the server reported success on DELETE operation for the table, the table is indeed deleted/dropped. http://gerrit.cloudera.org:8080/#/c/17577/43/src/kudu/integration-tests/rest_server-itest.cc@134 PS43, Line 134: } I guess after this the scenario should verify that GetKuduTableSchema REST method returns the proper results if queried about the renamed table, right? -- To view, visit http://gerrit.cloudera.org:8080/17577 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic1b2900c67d601fa5a3a747d2475e1e86a6ad2e3 Gerrit-Change-Number: 17577 Gerrit-PatchSet: 43 Gerrit-Owner: Khazar Mammadli <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 26 Apr 2022 22:25:30 +0000 Gerrit-HasComments: Yes
