Migrating existing rest fe tests to real cases #1631

yaooqinn · 2021-12-27T12:50:01Z

Why are the changes needed?

In this PR, we target the existing UTs from a noop server to a real shared Kyuubi server.

How was this patch tested?

Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request

yaooqinn · 2021-12-27T13:08:49Z

kyuubi-server/src/main/scala/org/apache/kyuubi/server/KyuubiRestFrontendService.scala

@@ -76,7 +76,7 @@ class KyuubiRestFrontendService(override val serverable: Serverable)

  override def connectionUrl: String = {
    checkInitialized()
-    s"${serverAddr.getCanonicalHostName}:$portNum"
+    s"${serverAddr.getCanonicalHostName}:${connector.getLocalPort}"


there's another bug here, in this pr, we only fix a bug for port==0

yaooqinn · 2021-12-27T13:09:55Z

kyuubi-server/src/test/scala/org/apache/kyuubi/RestFrontendTestHelper.scala

@@ -46,40 +48,29 @@ object RestFrontendTestHelper {
  }
 }

-trait RestFrontendTestHelper {
+trait RestFrontendTestHelper extends WithKyuubiServer {


refine to use a real singleton Kyuubi server for testing

yaooqinn · 2021-12-27T13:11:41Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/KyuubiRestFrontendServiceSuite.scala

-
-class KyuubiRestFrontendServiceSuite extends KyuubiFunSuite with RestFrontendTestHelper {
-
-  test("kyuubi REST frontend service basic") {


this case is not very necessary as most logic will be verified in RestFrontendTestHelper

yaooqinn · 2021-12-27T13:12:13Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/KyuubiRestFrontendServiceSuite.scala

-      response = webTarget.path("api/v1/ping").request().post(null)
-      assert(405 == response.getStatus)
-      assert(response.getStatusInfo.getReasonPhrase.equalsIgnoreCase("method not allowed"))
+  test("swagger ui") {


add 2 swagger tests

yaooqinn · 2021-12-27T13:13:55Z

cc @pan3793 @yanghua @simon824 @cfmcgrady thanks

yanghua · 2021-12-27T14:58:20Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/KyuubiRestFrontendServiceSuite.scala

-      }
-    }
+    val resp = webTarget.path("/api/v1/ping").request().get()
+    resp.getStatusInfo


assert the response value(pong) or status?

yanghua · 2021-12-27T15:15:32Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/SessionsResourceSuite.scala

@@ -34,88 +34,62 @@ import org.apache.kyuubi.session.SessionHandle

 class SessionsResourceSuite extends KyuubiFunSuite with RestFrontendTestHelper {

-  test("test open and count session") {
+  test("test open/close and count session") {


Remove the started test in the character?

yanghua

Left some comments.

yanghua · 2021-12-27T15:16:56Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/SessionsResourceSuite.scala

+    val sessions1 = response2.readEntity(classOf[SessionList])
+    assert(sessions1.sessionList.nonEmpty)
+
+    // close a opened session


yanghua · 2021-12-27T15:17:15Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/SessionsResourceSuite.scala


-      assert(sessionHandle.protocol.getValue == 1)
-      assert(sessionHandle.identifier != null)
+    // close a opened session


yanghua · 2021-12-27T15:17:32Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/SessionsResourceSuite.scala

-      assert(200 == response.getStatus)
-
-      val sessionHandle = response.readEntity(classOf[SessionHandle])
+    // verify the open session count


yanghua · 2021-12-27T15:17:43Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/SessionsResourceSuite.scala

-      val openedSessionCount = response.readEntity(classOf[SessionOpenCount])
-      assert(openedSessionCount.openSessionCount == 0)
-    }
+    // verify the open session count again


yanghua · 2021-12-27T15:18:10Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/OperationsResourceSuite.scala

+
+    response = webTarget.path(s"api/v1/operations/$opHandleStr/event")
+      .request(MediaType.APPLICATION_JSON_TYPE).get()
+    assert(404 == response.getStatus)
  }

  test("test get result set metadata") {


remove the started test?

yanghua · 2021-12-27T15:19:05Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/KyuubiRestFrontendServiceSuite.scala

+    assert(405 == response.getStatus)
+    assert(response.getStatusInfo.getReasonPhrase.equalsIgnoreCase("method not allowed"))
+
+    // send a request but throws a exception on the server side


an exception?

cfmcgrady · 2021-12-28T04:30:33Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/OperationsResourceSuite.scala

+    val catalogsHandleStr = getOpHandleStr(OperationType.GET_CATALOGS)
+    var response = webTarget.path(s"api/v1/operations/$catalogsHandleStr/event")
+      .request(MediaType.APPLICATION_JSON_TYPE).get()
+    val operationEvent = response.readEntity(classOf[KyuubiOperationEvent])


never be called.

cfmcgrady · 2021-12-28T04:30:57Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/OperationsResourceSuite.scala

+    var response = webTarget.path(s"api/v1/operations/$catalogsHandleStr/event")
+      .request(MediaType.APPLICATION_JSON_TYPE).get()
+    val operationEvent = response.readEntity(classOf[KyuubiOperationEvent])
+    assert(200 == response.getStatus)


move to checkOpState?

cfmcgrady · 2021-12-28T04:35:53Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/OperationsResourceSuite.scala

+    val statementHandleStr = getOpHandleStr(OperationType.EXECUTE_STATEMENT)
+    response = webTarget.path(s"api/v1/operations/$statementHandleStr/event")
+      .request(MediaType.APPLICATION_JSON_TYPE).get()
+    val statementEvent = response.readEntity(classOf[KyuubiOperationEvent])


cfmcgrady · 2021-12-28T04:42:31Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/SessionsResourceSuite.scala

+    operationHandle = response.readEntity(classOf[OperationHandle])
+    assert(operationHandle.typ == OperationType.GET_COLUMNS)
+
+    var getFunctionsReq = GetFunctionsRequest("default", "default", "default")


nit: var -> val

cfmcgrady · 2021-12-28T04:43:17Z

kyuubi-server/src/test/scala/org/apache/kyuubi/server/api/v1/SessionsResourceSuite.scala

+      .request(MediaType.APPLICATION_JSON_TYPE)
+      .post(Entity.entity(null, MediaType.APPLICATION_JSON_TYPE))
+    assert(200 == response.getStatus)
+    var operationHandle = response.readEntity(classOf[OperationHandle])


nit: var -> val

codecov-commenter · 2021-12-28T04:54:12Z

Codecov Report

Merging #1631 (c0da809) into master (2af105a) will increase coverage by 0.52%.
The diff coverage is 71.42%.

❗ Current head c0da809 differs from pull request most recent head 2726ab4. Consider uploading reports for the commit 2726ab4 to get more accurate results

@@             Coverage Diff              @@
##             master    #1631      +/-   ##
============================================
+ Coverage     58.32%   58.85%   +0.52%     
  Complexity      164      164              
============================================
  Files           260      260              
  Lines         12930    12932       +2     
  Branches       1633     1633              
============================================
+ Hits           7542     7611      +69     
+ Misses         4756     4685      -71     
- Partials        632      636       +4

Impacted Files	Coverage Δ
...apache/kyuubi/server/api/v1/SessionsResource.scala	`77.17% <66.66%> (+0.50%)`	⬆️
...ache/kyuubi/server/KyuubiRestFrontendService.scala	`86.79% <100.00%> (ø)`
.../org/apache/kyuubi/operation/KyuubiOperation.scala	`64.78% <0.00%> (-9.86%)`	⬇️
...ala/org/apache/kyuubi/operation/LaunchEngine.scala	`86.36% <0.00%> (ø)`
...org/apache/kyuubi/operation/ExecuteStatement.scala	`87.35% <0.00%> (ø)`
.../scala/org/apache/kyuubi/server/KyuubiServer.scala	`63.29% <0.00%> (+2.53%)`	⬆️
...ache/kyuubi/server/api/v1/OperationsResource.scala	`73.46% <0.00%> (+8.16%)`	⬆️
.../apache/kyuubi/client/KyuubiSyncThriftClient.scala	`94.95% <0.00%> (+11.76%)`	⬆️
...ache/kyuubi/operation/KyuubiOperationManager.scala	`95.00% <0.00%> (+12.50%)`	⬆️
...pache/kyuubi/server/BackendServiceTimeMetric.scala	`100.00% <0.00%> (+16.21%)`	⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2af105a...2726ab4. Read the comment docs.

pan3793 · 2021-12-28T08:11:13Z

kyuubi-server/src/test/scala/org/apache/kyuubi/RestFrontendTestHelper.scala

+
+  override def afterAll(): Unit = {
+    restApiBaseSuite.tearDown()
+    server.stop()


unnecessary server.stop()?

yaooqinn · 2021-12-28T09:17:49Z

thanks, merged to master

Migrating existing rest fe test to real cases

8196e4d

yaooqinn mentioned this pull request Dec 27, 2021

[Umbrella][KPIP-1] Support RESTful API for Kyuubi #950

Closed

25 tasks

yaooqinn changed the title ~~Migrating existing rest fe test to real cases~~ Migrating existing rests fe test to real cases Dec 27, 2021

Migrating existing rest fe test to real cases

119712e

yaooqinn commented Dec 27, 2021

View reviewed changes

yaooqinn closed this Dec 27, 2021

yaooqinn reopened this Dec 27, 2021

yaooqinn closed this Dec 27, 2021

yaooqinn reopened this Dec 27, 2021

yanghua reviewed Dec 27, 2021

View reviewed changes

yaooqinn added 4 commits December 28, 2021 11:20

Migrating existing rest fe test to real cases

75cca5f

Migrating existing rest fe test to real cases

fbd9769

Migrating existing rest fe test to real cases

0c1feb4

Migrating existing rest fe test to real cases

a31ab50

yaooqinn changed the title ~~Migrating existing rests fe test to real cases~~ Migrating existing rest fe tests to real cases Dec 28, 2021

Migrating existing rest fe test to real cases

80e1404

yaooqinn requested a review from yanghua December 28, 2021 04:06

yaooqinn self-assigned this Dec 28, 2021

yaooqinn added the test label Dec 28, 2021

yaooqinn added this to the v1.5.0 milestone Dec 28, 2021

cfmcgrady reviewed Dec 28, 2021

View reviewed changes

address comments

5d12f70

ci

c0da809

cfmcgrady approved these changes Dec 28, 2021

View reviewed changes

pan3793 reviewed Dec 28, 2021

View reviewed changes

address comments

2726ab4

pan3793 approved these changes Dec 28, 2021

View reviewed changes

yaooqinn closed this in e1587ee Dec 28, 2021

yaooqinn deleted the resttest branch December 28, 2021 09:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrating existing rest fe tests to real cases #1631

Migrating existing rest fe tests to real cases #1631

yaooqinn commented Dec 27, 2021 •

edited

Loading

yaooqinn Dec 27, 2021

yaooqinn Dec 27, 2021

yaooqinn Dec 27, 2021

yaooqinn Dec 27, 2021

yaooqinn commented Dec 27, 2021

yanghua Dec 27, 2021

yaooqinn Dec 28, 2021

yanghua Dec 27, 2021

yanghua left a comment

yanghua Dec 27, 2021

yanghua Dec 27, 2021

yanghua Dec 27, 2021

yanghua Dec 27, 2021

yanghua Dec 27, 2021

yanghua Dec 27, 2021

cfmcgrady Dec 28, 2021

yaooqinn Dec 28, 2021

cfmcgrady Dec 28, 2021

cfmcgrady Dec 28, 2021

cfmcgrady Dec 28, 2021

cfmcgrady Dec 28, 2021

codecov-commenter commented Dec 28, 2021 •

edited

Loading

pan3793 Dec 28, 2021

yaooqinn commented Dec 28, 2021


		class KyuubiRestFrontendServiceSuite extends KyuubiFunSuite with RestFrontendTestHelper {

		test("kyuubi REST frontend service basic") {

Migrating existing rest fe tests to real cases #1631

Migrating existing rest fe tests to real cases #1631

Conversation

yaooqinn commented Dec 27, 2021 • edited Loading

Why are the changes needed?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn commented Dec 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanghua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 28, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

yaooqinn commented Dec 28, 2021

yaooqinn commented Dec 27, 2021 •

edited

Loading

codecov-commenter commented Dec 28, 2021 •

edited

Loading