From ff82d4752da32af2fdad6233413a070b343afca9 Mon Sep 17 00:00:00 2001 From: Ryan Date: Mon, 16 Sep 2024 18:06:17 +0800 Subject: [PATCH 1/7] [YUNIKORN-2797] Increase handlers.go test coverage --- pkg/webservice/handlers_test.go | 55 ++++++++++++++++++++++++++------- 1 file changed, 43 insertions(+), 12 deletions(-) diff --git a/pkg/webservice/handlers_test.go b/pkg/webservice/handlers_test.go index 45f7669b4..ddb17c804 100644 --- a/pkg/webservice/handlers_test.go +++ b/pkg/webservice/handlers_test.go @@ -318,6 +318,18 @@ func newApplication(appID, partitionName, queueName, rmID string, ugi security.U return objects.NewApplication(siApp, userGroup, nil, rmID) } +func TestGetStackInfo(t *testing.T) { + req, err := http.NewRequest("GET", "", strings.NewReader(baseConf)) + assert.NilError(t, err, "Error creating request") + resp := &MockResponseWriter{} + getStackInfo(resp, req) + + // assert stack trace + assert.Assert(t, strings.Contains(string(resp.outputBytes), "goroutine"), "Stack trace should be present in the response") + assert.Assert(t, strings.Contains(string(resp.outputBytes), "test"), "Stack trace should be present in the response") + assert.Assert(t, strings.Contains(string(resp.outputBytes), "github.com/apache/yunikorn-core/pkg/webservice.getStackInfo"), "Stack trace should be present in the response") +} + func TestValidateConf(t *testing.T) { confTests := []struct { content string @@ -1332,23 +1344,17 @@ func TestGetPartitionNodes(t *testing.T) { } if node.NodeID == node1ID { - assert.Equal(t, node.NodeID, node1ID) - assert.Equal(t, "alloc-1", node.Allocations[0].AllocationKey) - assert.DeepEqual(t, attributesOfnode1, node.Attributes) - assert.DeepEqual(t, map[string]int64{"memory": 50, "vcore": 30}, node.Utilized) + assertNodeInfo(t, node, node1ID, "alloc-1", attributesOfnode1, map[string]int64{"memory": 50, "vcore": 30}) } else { - assert.Equal(t, node.NodeID, node2ID) - assert.Equal(t, "alloc-2", node.Allocations[0].AllocationKey) - assert.DeepEqual(t, attributesOfnode2, node.Attributes) - assert.DeepEqual(t, map[string]int64{"memory": 30, "vcore": 50}, node.Utilized) + assertNodeInfo(t, node, node2ID, "alloc-2", attributesOfnode2, map[string]int64{"memory": 30, "vcore": 50}) } } req, err = createRequest(t, "/ws/v1/partition/default/nodes", map[string]string{"partition": "notexists"}) assert.NilError(t, err, "Get Nodes for PartitionNodes Handler request failed") - resp1 := &MockResponseWriter{} - getPartitionNodes(resp1, req) - assertPartitionNotExists(t, resp1) + resp = &MockResponseWriter{} + getPartitionNodes(resp, req) + assertPartitionNotExists(t, resp) // test params name missing req, err = http.NewRequest("GET", "/ws/v1/partition/default/nodes", strings.NewReader("")) @@ -1358,10 +1364,14 @@ func TestGetPartitionNodes(t *testing.T) { assertParamsMissing(t, resp) // Test specific node - req, err = createRequest(t, "/ws/v1/partition/default/node/node-1", map[string]string{"node": "node-1"}) + req, err = createRequest(t, "/ws/v1/partition/default/node/node-1", map[string]string{"partition": "default", "node": "node-1"}) assert.NilError(t, err, "Get Node for PartitionNode Handler request failed") resp = &MockResponseWriter{} getPartitionNode(resp, req) + var nodeInfo dao.NodeDAOInfo + err = json.Unmarshal(resp.outputBytes, &nodeInfo) + assert.NilError(t, err, unmarshalError) + assertNodeInfo(t, &nodeInfo, node1ID, "alloc-1", attributesOfnode1, map[string]int64{"memory": 50, "vcore": 30}) // Test node id is missing req, err = createRequest(t, "/ws/v1/partition/default/node/node-1", map[string]string{"partition": "default", "node": ""}) @@ -1369,6 +1379,27 @@ func TestGetPartitionNodes(t *testing.T) { resp = &MockResponseWriter{} getPartitionNode(resp, req) assertNodeIDNotExists(t, resp) + + // Test param missing + req, err = http.NewRequest("GET", "/ws/v1/partition/default/node", strings.NewReader("")) + assert.NilError(t, err, "Get Node for PartitionNode Handler request failed") + resp = &MockResponseWriter{} + getPartitionNode(resp, req) + assertParamsMissing(t, resp) + + // Test partition not exist + req, err = createRequest(t, "/ws/v1/partition/notexists/node/node-1", map[string]string{"partition": "notexists"}) + assert.NilError(t, err, "Get Nodes for PartitionNode Handler request failed") + resp = &MockResponseWriter{} + getPartitionNodes(resp, req) + assertPartitionNotExists(t, resp) +} + +func assertNodeInfo(t *testing.T, node *dao.NodeDAOInfo, expectedID string, expectedAllocationKey string, expectedAttibute map[string]string, expectedUtilized map[string]int64) { + assert.Equal(t, expectedID, node.NodeID) + assert.Equal(t, expectedAllocationKey, node.Allocations[0].AllocationKey) + assert.DeepEqual(t, expectedAttibute, node.Attributes) + assert.DeepEqual(t, expectedUtilized, node.Utilized) } // addApp Add app to the given partition and assert the app count, state etc From b2b04db63cd389cbf2997c096b69f52e35b1f325 Mon Sep 17 00:00:00 2001 From: Ryan Date: Wed, 18 Sep 2024 10:40:04 +0800 Subject: [PATCH 2/7] code review --- pkg/webservice/handlers_test.go | 72 ++++++++++++++++++++++++++++++--- 1 file changed, 66 insertions(+), 6 deletions(-) diff --git a/pkg/webservice/handlers_test.go b/pkg/webservice/handlers_test.go index ddb17c804..f777e346f 100644 --- a/pkg/webservice/handlers_test.go +++ b/pkg/webservice/handlers_test.go @@ -319,15 +319,34 @@ func newApplication(appID, partitionName, queueName, rmID string, ugi security.U } func TestGetStackInfo(t *testing.T) { + // normal case req, err := http.NewRequest("GET", "", strings.NewReader(baseConf)) assert.NilError(t, err, "Error creating request") resp := &MockResponseWriter{} getStackInfo(resp, req) + assertIsStackInfo(t, resp.outputBytes) - // assert stack trace - assert.Assert(t, strings.Contains(string(resp.outputBytes), "goroutine"), "Stack trace should be present in the response") - assert.Assert(t, strings.Contains(string(resp.outputBytes), "test"), "Stack trace should be present in the response") - assert.Assert(t, strings.Contains(string(resp.outputBytes), "github.com/apache/yunikorn-core/pkg/webservice.getStackInfo"), "Stack trace should be present in the response") + // Create a deep call stack (100 calls) and check if the stack trace is larger than 15000 bytes + var deepFunction func(int) + deepFunction = func(depth int) { + if depth > 0 { + deepFunction(depth - 1) + } else { + resp = &MockResponseWriter{} + req, _ := http.NewRequest("GET", "/stack", nil) + getStackInfo(resp, req) + assertIsStackInfo(t, resp.outputBytes) + assert.Check(t, len(resp.outputBytes) > 15000, "Expected stack trace larger than 15000 bytes, where it will reach max at around 17898") + } + } + deepFunction(100) +} + +// assert stack trace +func assertIsStackInfo(t *testing.T, outputBytes []byte) { + assert.Assert(t, strings.Contains(string(outputBytes), "goroutine"), "Stack trace should be present in the response") + assert.Assert(t, strings.Contains(string(outputBytes), "test"), "Stack trace should be present in the response") + assert.Assert(t, strings.Contains(string(outputBytes), "github.com/apache/yunikorn-core/pkg/webservice.getStackInfo"), "Stack trace should be present in the response") } func TestValidateConf(t *testing.T) { @@ -1362,11 +1381,52 @@ func TestGetPartitionNodes(t *testing.T) { resp = &MockResponseWriter{} getPartitionNodes(resp, req) assertParamsMissing(t, resp) +} + +func TestGetPartitionNode(t *testing.T) { + partition := setup(t, configDefault, 1) + + // create test application + appID := "app1" + app := newApplication(appID, partition.Name, queueName, rmID, security.UserGroup{User: "testuser", Groups: []string{"testgroup"}}) + err := partition.AddApplication(app) + assert.NilError(t, err, "add application to partition should not have failed") + + // create test nodes + attributesOfnode1 := map[string]string{"Disk": "SSD"} + attributesOfnode2 := map[string]string{"Devices": "camera"} + nodeRes := resources.NewResourceFromMap(map[string]resources.Quantity{siCommon.Memory: 1000, siCommon.CPU: 1000}).ToProto() + node1ID := "node-1" + node1 := objects.NewNode(&si.NodeInfo{NodeID: node1ID, Attributes: attributesOfnode1, SchedulableResource: nodeRes}) + node2ID := "node-2" + node2 := objects.NewNode(&si.NodeInfo{NodeID: node2ID, Attributes: attributesOfnode2, SchedulableResource: nodeRes}) + // create test allocations + resAlloc1 := resources.NewResourceFromMap(map[string]resources.Quantity{siCommon.Memory: 500, siCommon.CPU: 300}) + resAlloc2 := resources.NewResourceFromMap(map[string]resources.Quantity{siCommon.Memory: 300, siCommon.CPU: 500}) + alloc1 := newAlloc("alloc-1", appID, node1ID, resAlloc1) + allocs := []*objects.Allocation{alloc1} + err = partition.AddNode(node1) + assert.NilError(t, err, "add node to partition should not have failed") + _, allocCreated, err := partition.UpdateAllocation(allocs[0]) + assert.NilError(t, err, "add alloc-1 should not have failed") + assert.Check(t, allocCreated) + + alloc2 := newAlloc("alloc-2", appID, node2ID, resAlloc2) + allocs = []*objects.Allocation{alloc2} + err = partition.AddNode(node2) + assert.NilError(t, err, "add node to partition should not have failed") + _, allocCreated, err = partition.UpdateAllocation(allocs[0]) + assert.NilError(t, err, "add alloc-2 should not have failed") + assert.Check(t, allocCreated) + + NewWebApp(schedulerContext.Load(), nil) + + var req *http.Request // Test specific node req, err = createRequest(t, "/ws/v1/partition/default/node/node-1", map[string]string{"partition": "default", "node": "node-1"}) assert.NilError(t, err, "Get Node for PartitionNode Handler request failed") - resp = &MockResponseWriter{} + resp := &MockResponseWriter{} getPartitionNode(resp, req) var nodeInfo dao.NodeDAOInfo err = json.Unmarshal(resp.outputBytes, &nodeInfo) @@ -1387,7 +1447,7 @@ func TestGetPartitionNodes(t *testing.T) { getPartitionNode(resp, req) assertParamsMissing(t, resp) - // Test partition not exist + // Test partition does not exist req, err = createRequest(t, "/ws/v1/partition/notexists/node/node-1", map[string]string{"partition": "notexists"}) assert.NilError(t, err, "Get Nodes for PartitionNode Handler request failed") resp = &MockResponseWriter{} From 11b87c20997807f451b708e6a13a353cd9b54d22 Mon Sep 17 00:00:00 2001 From: Ryan Date: Wed, 18 Sep 2024 12:47:21 +0800 Subject: [PATCH 3/7] Update assert bytes due to possible ci RAM limitations --- pkg/webservice/handlers_test.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/pkg/webservice/handlers_test.go b/pkg/webservice/handlers_test.go index f777e346f..344e1885b 100644 --- a/pkg/webservice/handlers_test.go +++ b/pkg/webservice/handlers_test.go @@ -320,26 +320,28 @@ func newApplication(appID, partitionName, queueName, rmID string, ugi security.U func TestGetStackInfo(t *testing.T) { // normal case - req, err := http.NewRequest("GET", "", strings.NewReader(baseConf)) + req, err := http.NewRequest("GET", "/stack", strings.NewReader(baseConf)) assert.NilError(t, err, "Error creating request") resp := &MockResponseWriter{} getStackInfo(resp, req) assertIsStackInfo(t, resp.outputBytes) - // Create a deep call stack (100 calls) and check if the stack trace is larger than 15000 bytes + // Create a deep call stack (30 calls) and check if the stack trace is larger than 1024 bytes + // assuming RAM is not less than 1024 bytes var deepFunction func(int) deepFunction = func(depth int) { if depth > 0 { deepFunction(depth - 1) } else { resp = &MockResponseWriter{} - req, _ := http.NewRequest("GET", "/stack", nil) + req, err = http.NewRequest("GET", "/stack", nil) + assert.NilError(t, err, httpRequestError) getStackInfo(resp, req) assertIsStackInfo(t, resp.outputBytes) - assert.Check(t, len(resp.outputBytes) > 15000, "Expected stack trace larger than 15000 bytes, where it will reach max at around 17898") + assert.Check(t, len(resp.outputBytes) > 1024, "Expected stack trace larger than 1024 bytes") } } - deepFunction(100) + deepFunction(30) } // assert stack trace From ba1ac07bde3a7cb04d3e063a573284186066de31 Mon Sep 17 00:00:00 2001 From: Ryan Date: Wed, 18 Sep 2024 12:49:33 +0800 Subject: [PATCH 4/7] Update handlers_test.go --- pkg/webservice/handlers_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/webservice/handlers_test.go b/pkg/webservice/handlers_test.go index 344e1885b..43a410232 100644 --- a/pkg/webservice/handlers_test.go +++ b/pkg/webservice/handlers_test.go @@ -326,8 +326,8 @@ func TestGetStackInfo(t *testing.T) { getStackInfo(resp, req) assertIsStackInfo(t, resp.outputBytes) - // Create a deep call stack (30 calls) and check if the stack trace is larger than 1024 bytes - // assuming RAM is not less than 1024 bytes + // Create a deep call stack (30 calls) and check if the stack trace is larger than 5000 bytes + // assuming RAM is not less than 5000 bytes var deepFunction func(int) deepFunction = func(depth int) { if depth > 0 { @@ -338,7 +338,7 @@ func TestGetStackInfo(t *testing.T) { assert.NilError(t, err, httpRequestError) getStackInfo(resp, req) assertIsStackInfo(t, resp.outputBytes) - assert.Check(t, len(resp.outputBytes) > 1024, "Expected stack trace larger than 1024 bytes") + assert.Check(t, len(resp.outputBytes) > 5000, "Expected stack trace larger than 5000 bytes") } } deepFunction(30) From 9e89c2ed1183704b8bb5b5a98c26a2a9a04327f0 Mon Sep 17 00:00:00 2001 From: Ryan Date: Wed, 18 Sep 2024 13:14:05 +0800 Subject: [PATCH 5/7] tame the linter --- pkg/webservice/handlers_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/webservice/handlers_test.go b/pkg/webservice/handlers_test.go index 43a410232..b9e4d0881 100644 --- a/pkg/webservice/handlers_test.go +++ b/pkg/webservice/handlers_test.go @@ -1389,7 +1389,7 @@ func TestGetPartitionNode(t *testing.T) { partition := setup(t, configDefault, 1) // create test application - appID := "app1" + appID := "app-1" app := newApplication(appID, partition.Name, queueName, rmID, security.UserGroup{User: "testuser", Groups: []string{"testgroup"}}) err := partition.AddApplication(app) assert.NilError(t, err, "add application to partition should not have failed") @@ -1398,9 +1398,9 @@ func TestGetPartitionNode(t *testing.T) { attributesOfnode1 := map[string]string{"Disk": "SSD"} attributesOfnode2 := map[string]string{"Devices": "camera"} nodeRes := resources.NewResourceFromMap(map[string]resources.Quantity{siCommon.Memory: 1000, siCommon.CPU: 1000}).ToProto() - node1ID := "node-1" + node1ID := "node_1" node1 := objects.NewNode(&si.NodeInfo{NodeID: node1ID, Attributes: attributesOfnode1, SchedulableResource: nodeRes}) - node2ID := "node-2" + node2ID := "node_2" node2 := objects.NewNode(&si.NodeInfo{NodeID: node2ID, Attributes: attributesOfnode2, SchedulableResource: nodeRes}) // create test allocations @@ -1426,7 +1426,7 @@ func TestGetPartitionNode(t *testing.T) { var req *http.Request // Test specific node - req, err = createRequest(t, "/ws/v1/partition/default/node/node-1", map[string]string{"partition": "default", "node": "node-1"}) + req, err = createRequest(t, "/ws/v1/partition/default/node/node_1", map[string]string{"partition": "default", "node": "node_1"}) assert.NilError(t, err, "Get Node for PartitionNode Handler request failed") resp := &MockResponseWriter{} getPartitionNode(resp, req) @@ -1436,7 +1436,7 @@ func TestGetPartitionNode(t *testing.T) { assertNodeInfo(t, &nodeInfo, node1ID, "alloc-1", attributesOfnode1, map[string]int64{"memory": 50, "vcore": 30}) // Test node id is missing - req, err = createRequest(t, "/ws/v1/partition/default/node/node-1", map[string]string{"partition": "default", "node": ""}) + req, err = createRequest(t, "/ws/v1/partition/default/node/node_1", map[string]string{"partition": "default", "node": ""}) assert.NilError(t, err, "Get Node for PartitionNode Handler request failed") resp = &MockResponseWriter{} getPartitionNode(resp, req) @@ -1450,7 +1450,7 @@ func TestGetPartitionNode(t *testing.T) { assertParamsMissing(t, resp) // Test partition does not exist - req, err = createRequest(t, "/ws/v1/partition/notexists/node/node-1", map[string]string{"partition": "notexists"}) + req, err = createRequest(t, "/ws/v1/partition/notexists/node/node_1", map[string]string{"partition": "notexists"}) assert.NilError(t, err, "Get Nodes for PartitionNode Handler request failed") resp = &MockResponseWriter{} getPartitionNodes(resp, req) From d0c2e67de27d0e2ce4f397906be4a5a6aac1d2d4 Mon Sep 17 00:00:00 2001 From: Ryan Date: Wed, 18 Sep 2024 13:44:15 +0800 Subject: [PATCH 6/7] code review --- pkg/webservice/handlers_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/webservice/handlers_test.go b/pkg/webservice/handlers_test.go index b9e4d0881..7b5bdb241 100644 --- a/pkg/webservice/handlers_test.go +++ b/pkg/webservice/handlers_test.go @@ -324,6 +324,7 @@ func TestGetStackInfo(t *testing.T) { assert.NilError(t, err, "Error creating request") resp := &MockResponseWriter{} getStackInfo(resp, req) + assert.Equal(t, resp.statusCode, http.StatusOK, statusCodeError) assertIsStackInfo(t, resp.outputBytes) // Create a deep call stack (30 calls) and check if the stack trace is larger than 5000 bytes @@ -337,6 +338,7 @@ func TestGetStackInfo(t *testing.T) { req, err = http.NewRequest("GET", "/stack", nil) assert.NilError(t, err, httpRequestError) getStackInfo(resp, req) + assert.Equal(t, resp.statusCode, http.StatusOK, statusCodeError) assertIsStackInfo(t, resp.outputBytes) assert.Check(t, len(resp.outputBytes) > 5000, "Expected stack trace larger than 5000 bytes") } From 9bddd3960240412cf47918dc624f478149c1248a Mon Sep 17 00:00:00 2001 From: Ryan Date: Wed, 18 Sep 2024 14:04:00 +0800 Subject: [PATCH 7/7] revert assert statusOK --- pkg/webservice/handlers_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/pkg/webservice/handlers_test.go b/pkg/webservice/handlers_test.go index 7b5bdb241..b9e4d0881 100644 --- a/pkg/webservice/handlers_test.go +++ b/pkg/webservice/handlers_test.go @@ -324,7 +324,6 @@ func TestGetStackInfo(t *testing.T) { assert.NilError(t, err, "Error creating request") resp := &MockResponseWriter{} getStackInfo(resp, req) - assert.Equal(t, resp.statusCode, http.StatusOK, statusCodeError) assertIsStackInfo(t, resp.outputBytes) // Create a deep call stack (30 calls) and check if the stack trace is larger than 5000 bytes @@ -338,7 +337,6 @@ func TestGetStackInfo(t *testing.T) { req, err = http.NewRequest("GET", "/stack", nil) assert.NilError(t, err, httpRequestError) getStackInfo(resp, req) - assert.Equal(t, resp.statusCode, http.StatusOK, statusCodeError) assertIsStackInfo(t, resp.outputBytes) assert.Check(t, len(resp.outputBytes) > 5000, "Expected stack trace larger than 5000 bytes") }