[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #472: [YUNIKORN-1428] Add unit tests to ensure multiple users belongs to single group and vice versa cases

[GitBox <gi...@apache.org>]

wilfred-s commented on code in PR #472:
URL: https://github.com/apache/yunikorn-core/pull/472#discussion_r1045400883


##########
pkg/scheduler/objects/application_test.go:
##########
@@ -90,15 +90,15 @@ func TestNewApplication(t *testing.T) {
 	if app.rmEventHandler == nil {
 		t.Fatal("non nil handler was not set in the new app")
 	}
-	assertUserGroupResource(t, getTestUserGroup(), nil)
+	assertUserGroupResource(t, getTestUserGroup("testuser", []string{"testgroup"}), nil, nil, 0)

Review Comment:
   Same as before: if you need the ability to check user and group separately you add a new function for it instead of changing 50+ callers.



##########
pkg/scheduler/objects/application_state_test.go:
##########
@@ -29,7 +29,7 @@ import (
 
 func TestAcceptStateTransition(t *testing.T) {
 	// Accept only from new
-	app := newApplication("app-00001", "default", "root.a")
+	app := newApplication("app-00001", "default", "root.a", "testuser", []string{"testgroup"})

Review Comment:
   Instead of changing all calls (70+ invocations) which already use this same user as they initialise the app indirectly via the calls:
   ```
   newApplication(...)
       newApplicationWithTags(...)
           NewApplication(siApp, getTestUserGroup(), nil, "")
   ```
   Add a new call that allows specifying the user and group.



##########
pkg/scheduler/objects/utilities_test.go:
##########
@@ -224,14 +224,14 @@ func newAllocationAskAll(allocKey, appID, taskGroup string, res *resources.Resou
 	return NewAllocationAskFromSI(ask)
 }
 
-func getTestUserGroup() security.UserGroup {
-	return security.UserGroup{User: "testuser", Groups: []string{"testgroup"}}
+func getTestUserGroup(userName string, groupNameList []string) security.UserGroup {

Review Comment:
   Same here: use a separate function `getUserGroup(userName string, groupNameList []string) security.UserGroup` You can even make `getTestUserGroup()` call that one with the default test user details: `"testuser", []string{"testgroup"}`



-- 
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: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org