wilfred-s commented on a change in pull request #372:
URL: 
https://github.com/apache/incubator-yunikorn-core/pull/372#discussion_r815581130



##########
File path: pkg/webservice/handlers_test.go
##########
@@ -607,6 +607,21 @@ func TestGetClusterUtilJSON(t *testing.T) {
        assert.NilError(t, err, "Error when load clusterInfo from config")
        assert.Equal(t, 1, len(schedulerContext.GetPartitionMapClone()))
 
+       // check build information of RM
+       BuildInfoMap := make(map[string]string)

Review comment:
       NIT: We do not have to export this `buildInfoMap` is enough.

##########
File path: pkg/scheduler/context_test.go
##########
@@ -255,6 +255,49 @@ func TestContext_AddNode(t *testing.T) {
        }
 }
 
+func TestContext_AddRMBuildInformation(t *testing.T) {
+       context := createTestContext(t, pName)
+
+       rmID1 := "myCluster1"
+       BuildInfoMap1 := make(map[string]string)

Review comment:
       NIT: We do not have to export this `buildInfoMap` is enough.

##########
File path: pkg/scheduler/context.go
##########
@@ -164,6 +173,15 @@ func (cc *ClusterContext) processRMRegistrationEvent(event 
*rmevent.RMRegistrati
        cc.policyGroup = policyGroup
        configs.ConfigContext.Set(policyGroup, conf)
 
+       // store the build information of RM
+       if cc.rmInfos == nil {
+               cc.rmInfos = make(map[string]*RMInformation)
+       }
+       cc.rmInfos[rmID] = &RMInformation{
+               RMBuildInformation: event.Registration.BuildInfo,
+       }
+       cc.rmInfos[rmID].RMBuildInformation["rmId"] = rmID
+

Review comment:
       Instead of duplicating this from `SetRMInfos()` we should call it here.

##########
File path: pkg/scheduler/context_test.go
##########
@@ -255,6 +255,49 @@ func TestContext_AddNode(t *testing.T) {
        }
 }
 
+func TestContext_AddRMBuildInformation(t *testing.T) {
+       context := createTestContext(t, pName)
+
+       rmID1 := "myCluster1"
+       BuildInfoMap1 := make(map[string]string)
+       BuildInfoMap1["buildDate"] = "2006-01-02T15:04:05-0700"
+       BuildInfoMap1["buildVersion"] = "latest"
+       BuildInfoMap1["isPluginVersion"] = "false"
+       rm1 := &si.RegisterResourceManagerRequest{
+               RmID:        rmID1,
+               PolicyGroup: "policygroup",
+               Version:     "0.0.2",
+               BuildInfo:   BuildInfoMap1,
+       }

Review comment:
       Leverage the `SetRMInfos()` call that you have added. Create the test 
context as you did. Call `SetRMInfos()` with the buildInfoMap created here.
   The first one should give a length of 1 etc (asserts like you have)
   Create a second buildInfoMap object call `SetRMInfos()` again make sure that 
you have two objects in the list.
   Update a value in one of the two maps (like buildVersion is 1.0.0) Call 
`SetRMInfos()` again with the updated map and make sure it is replaced as 
expected.
   That will give us a full test of the code




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to