[GitHub] [yunikorn-core] wilfred-s commented on a diff in pull request #492: [YUNIKORN-1446] Adopting table-driven style in the acl unit test

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

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


##########
pkg/common/security/acl_test.go:
##########
@@ -19,119 +19,206 @@
 package security
 
 import (
+	"errors"
+	"fmt"
 	"testing"
-
-	"gotest.tools/assert"
 )
 
-func TestACLCreate(t *testing.T) {
-	_, err := NewACL("")
-	if err != nil {
-		t.Errorf("parsing failed for string: ''")
-	}
-	_, err = NewACL(" ")
-	if err != nil {
-		t.Errorf("parsing failed for string: ' '")
-	}
-	_, err = NewACL("user1")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1'")
-	}
-	_, err = NewACL("user1,user2")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2'")
-	}
-	_, err = NewACL("user1,user2 ")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 '")
-	}
-	_, err = NewACL("user1,user2 group1")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 group1'")
-	}
-	_, err = NewACL("user1,user2 group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 group1,group2'")
-	}
-	_, err = NewACL("user2 group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user2 group1,group2'")
-	}
-	_, err = NewACL(" group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: ' group1,group2'")
-	}
-	_, err = NewACL("* group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: '* group1,group2'")
-	}
-	_, err = NewACL("user1,user2 *")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 *'")
-	}
-	_, err = NewACL("*")
-	if err != nil {
-		t.Errorf("parsing failed for string: '*'")
-	}
-	_, err = NewACL("* ")
-	if err != nil {
-		t.Errorf("parsing failed for string: '* '")
-	}
-	_, err = NewACL(" *")
-	if err != nil {
-		t.Errorf("parsing failed for string: ' *'")
+func IsSameACL(got, expected ACL) error {
+	if got.allAllowed != expected.allAllowed {
+		return errors.New("allAllowed is not same")
+	}
+	if expected.users != nil && got.users != nil {
+		for username, expectedAllow := range expected.users {
+			if gotAllow, ok := got.users[username]; !ok {
+				return fmt.Errorf("username %s does not exist", username)
+			} else if expectedAllow != gotAllow {
+				return fmt.Errorf("username %s is not same", username)
+			}
+		}
+	} else if (expected.users != nil && got.users == nil) || (expected.users == nil && got.users != nil) {
+		return errors.New("users of an acl is not nil, expect one and got one should be same")
 	}
+
+	if expected.groups != nil && got.groups != nil {

Review Comment:
   Same as for users



##########
pkg/common/security/acl_test.go:
##########
@@ -19,119 +19,206 @@
 package security
 
 import (
+	"errors"
+	"fmt"
 	"testing"
-
-	"gotest.tools/assert"
 )
 
-func TestACLCreate(t *testing.T) {
-	_, err := NewACL("")
-	if err != nil {
-		t.Errorf("parsing failed for string: ''")
-	}
-	_, err = NewACL(" ")
-	if err != nil {
-		t.Errorf("parsing failed for string: ' '")
-	}
-	_, err = NewACL("user1")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1'")
-	}
-	_, err = NewACL("user1,user2")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2'")
-	}
-	_, err = NewACL("user1,user2 ")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 '")
-	}
-	_, err = NewACL("user1,user2 group1")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 group1'")
-	}
-	_, err = NewACL("user1,user2 group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 group1,group2'")
-	}
-	_, err = NewACL("user2 group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user2 group1,group2'")
-	}
-	_, err = NewACL(" group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: ' group1,group2'")
-	}
-	_, err = NewACL("* group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: '* group1,group2'")
-	}
-	_, err = NewACL("user1,user2 *")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 *'")
-	}
-	_, err = NewACL("*")
-	if err != nil {
-		t.Errorf("parsing failed for string: '*'")
-	}
-	_, err = NewACL("* ")
-	if err != nil {
-		t.Errorf("parsing failed for string: '* '")
-	}
-	_, err = NewACL(" *")
-	if err != nil {
-		t.Errorf("parsing failed for string: ' *'")
+func IsSameACL(got, expected ACL) error {
+	if got.allAllowed != expected.allAllowed {
+		return errors.New("allAllowed is not same")
+	}
+	if expected.users != nil && got.users != nil {
+		for username, expectedAllow := range expected.users {
+			if gotAllow, ok := got.users[username]; !ok {
+				return fmt.Errorf("username %s does not exist", username)
+			} else if expectedAllow != gotAllow {
+				return fmt.Errorf("username %s is not same", username)
+			}
+		}
+	} else if (expected.users != nil && got.users == nil) || (expected.users == nil && got.users != nil) {
+		return errors.New("users of an acl is not nil, expect one and got one should be same")

Review Comment:
   You are not catching the case that `got` has more users than `expected`
   Simplify and change check ordering:
   1. check if length of both maps are equal: if not return
   2. check equality of the members: let the range handle nil maps. 
   3. Combine the user not found and the allow flag difference in one check:
   ```
   gotAllow, ok := got.users[username]; !ok || gotAllow != expectedAllow
   ```



##########
pkg/common/security/acl_test.go:
##########
@@ -19,119 +19,206 @@
 package security
 
 import (
+	"errors"
+	"fmt"
 	"testing"
-
-	"gotest.tools/assert"
 )
 
-func TestACLCreate(t *testing.T) {
-	_, err := NewACL("")
-	if err != nil {
-		t.Errorf("parsing failed for string: ''")
-	}
-	_, err = NewACL(" ")
-	if err != nil {
-		t.Errorf("parsing failed for string: ' '")
-	}
-	_, err = NewACL("user1")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1'")
-	}
-	_, err = NewACL("user1,user2")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2'")
-	}
-	_, err = NewACL("user1,user2 ")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 '")
-	}
-	_, err = NewACL("user1,user2 group1")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 group1'")
-	}
-	_, err = NewACL("user1,user2 group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 group1,group2'")
-	}
-	_, err = NewACL("user2 group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user2 group1,group2'")
-	}
-	_, err = NewACL(" group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: ' group1,group2'")
-	}
-	_, err = NewACL("* group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: '* group1,group2'")
-	}
-	_, err = NewACL("user1,user2 *")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 *'")
-	}
-	_, err = NewACL("*")
-	if err != nil {
-		t.Errorf("parsing failed for string: '*'")
-	}
-	_, err = NewACL("* ")
-	if err != nil {
-		t.Errorf("parsing failed for string: '* '")
-	}
-	_, err = NewACL(" *")
-	if err != nil {
-		t.Errorf("parsing failed for string: ' *'")
+func IsSameACL(got, expected ACL) error {
+	if got.allAllowed != expected.allAllowed {
+		return errors.New("allAllowed is not same")
+	}
+	if expected.users != nil && got.users != nil {
+		for username, expectedAllow := range expected.users {
+			if gotAllow, ok := got.users[username]; !ok {
+				return fmt.Errorf("username %s does not exist", username)
+			} else if expectedAllow != gotAllow {
+				return fmt.Errorf("username %s is not same", username)
+			}
+		}
+	} else if (expected.users != nil && got.users == nil) || (expected.users == nil && got.users != nil) {
+		return errors.New("users of an acl is not nil, expect one and got one should be same")
 	}
+
+	if expected.groups != nil && got.groups != nil {
+		for groupname, expectedAllow := range expected.groups {
+			if gotAllow, ok := got.groups[groupname]; !ok {
+				return fmt.Errorf("groupname %s does not exist", groupname)
+			} else if expectedAllow != gotAllow {
+				return fmt.Errorf("groupname %s is not same", groupname)
+			}
+		}
+	} else if (expected.groups != nil && got.groups == nil) || (expected.groups == nil && got.groups != nil) {
+		return errors.New("groups of an acl is not nil, expect one and got one should be same")
+	}
+	return nil
 }
 
-func TestACLSpecialCase(t *testing.T) {
-	acl, err := NewACL("  ")
-	if err == nil {
-		t.Errorf("parsing passed for string: '  '")
-	}
+func TestACLCreate(t *testing.T) {
+	tests := []struct {
+		input    string
+		expected ACL
+	}{
+		{
+			"",
+			ACL{allAllowed: false}},
+		{
+			" ",
+			ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: false}},
+		{
+			"user1",
+			ACL{users: map[string]bool{"user1": true}, allAllowed: false}},
+		{
+			"user1,user2",
+			ACL{users: map[string]bool{"user1": true, "user2": true}, allAllowed: false}},
+		{
+			"user1,user2 ",
+			ACL{users: map[string]bool{"user1": true, "user2": true}, groups: make(map[string]bool), allAllowed: false}},
+		{
+			"user1,user2 group1",
+			ACL{users: map[string]bool{"user1": true, "user2": true}, groups: map[string]bool{"group1": true}, allAllowed: false},
+		},
+		{
+			"user1,user2 group1,group2",
+			ACL{users: map[string]bool{"user1": true, "user2": true}, groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+		},
+		{
+			"user2 group1,group2",
+			ACL{users: map[string]bool{"user2": true}, groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+		},
+		{
+			" group1,group2",
+			ACL{users: make(map[string]bool), groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+		},
+		{
+			"* group1,group2",
+			ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+		},
+		{
+			"user1,user2 *",
+			ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+		},
+		{
+			"*",
+			ACL{users: make(map[string]bool), allAllowed: true},
+		},
+		{
+			"* ",
+			ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+		},
+		{
+			" *",
+			ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+		},
+		{
+			"dotted.user",
+			ACL{users: map[string]bool{"dotted.user": true}, allAllowed: false},
+		},
+		{
+			"user,user",
+			ACL{users: map[string]bool{"user": true}, allAllowed: false},
+		},
+		{
+			" dotted.group",
+			ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: false},
+		},
+		{
+			" group,group",
+			ACL{users: make(map[string]bool), groups: map[string]bool{"group": true}, allAllowed: false},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.input, func(t *testing.T) {
+			got, err := NewACL(tt.input)
+			if err != nil {
+				t.Errorf("parsing failed for string: %s", tt.input)
+			}
 
-	acl, err = NewACL("dotted.user")
-	if err != nil || len(acl.users) != 1 {
-		t.Errorf("parsing failed for string: 'dotted.user' acl has incorrect user list: %v", acl)
-	}
-	acl, err = NewACL("user,user")
-	if err != nil || len(acl.users) != 1 {
-		t.Errorf("parsing failed for string: 'user,user' acl has incorrect user list: %v", acl)
+			if err = IsSameACL(got, tt.expected); err != nil {
+				t.Error(err.Error())
+			}
+		})
 	}
-	acl, err = NewACL(" dotted.group")
-	if err != nil || len(acl.groups) > 0 {
-		t.Errorf("parsing failed for string: ' dotted.group' acl has incorrect group list: %v", acl)
-	}
-	acl, err = NewACL(" group,group")
-	if err != nil || len(acl.groups) != 1 {
-		t.Errorf("parsing failed for string: 'group,group' acl has incorrect group list: %v", acl)
+}
+
+func TestNewACLErrorCase(t *testing.T) {
+	tests := []struct {
+		caseName string
+		acl      string
+	}{
+		{"spaces", "  "},
+		{"number of spaces is higher than 1 is not allowed", " a b "},
+	}
+	for _, tt := range tests {
+		t.Run(tt.caseName, func(t *testing.T) {
+			if _, err := NewACL(tt.acl); err == nil {
+				t.Errorf("parsing %s string should be failed", tt.acl)
+			}
+		})
 	}
 }
 
 func TestACLAccess(t *testing.T) {
-	acl, err := NewACL("user1,user2 group1,group2")
-	assert.NilError(t, err, "parsing failed for string: 'user1,user2 group1,group2'")
-	user := UserGroup{User: "", Groups: nil}
-	assert.Assert(t, !acl.CheckAccess(user), "no user, should have been denied")
-	user = UserGroup{User: "user1", Groups: nil}
-	assert.Assert(t, acl.CheckAccess(user), "user1 (no groups) should have been allowed")
-	user = UserGroup{User: "user3", Groups: []string{"group1"}}
-	assert.Assert(t, acl.CheckAccess(user), "group1 should have been allowed")
-	user = UserGroup{User: "user3", Groups: []string{"group3", "group1"}}
-	assert.Assert(t, acl.CheckAccess(user), "group1 (2nd group) should have been allowed")
-	user = UserGroup{User: "user3", Groups: []string{"group3"}}
-	assert.Assert(t, !acl.CheckAccess(user), "user3/group3 should have been denied")
-
-	acl, err = NewACL("*")
-	assert.NilError(t, err, "parsing failed for string: '*'")
-	user = UserGroup{User: "", Groups: nil}
-	assert.Assert(t, acl.CheckAccess(user), "no user, wildcard should have been allowed")
-	user = UserGroup{User: "user1", Groups: []string{"group1"}}
-	assert.Assert(t, acl.CheckAccess(user), "user1/group1, wildcard should have been allowed")
-
-	acl, err = NewACL("")
-	assert.NilError(t, err, "parsing failed for string: ''")
-	user = UserGroup{User: "", Groups: nil}
-	assert.Assert(t, !acl.CheckAccess(user), "no user, empty ACL always deny")
-	user = UserGroup{User: "user1", Groups: []string{"group1"}}
-	assert.Assert(t, !acl.CheckAccess(user), "user1/group1, empty ACL always deny")
+	tests := []struct {
+		acl      string
+		visitor  UserGroup
+		expected bool
+	}{
+		{
+			"user1,user2 group1,group2",
+			UserGroup{User: "", Groups: nil},
+			false,
+		},
+		{
+			"user1,user2 group1,group2",
+			UserGroup{User: "user1", Groups: nil},
+			true,
+		},
+		{
+			"user1,user2 group1,group2",
+			UserGroup{User: "user3", Groups: []string{"group1"}},
+			true,
+		},
+		{
+			"user1,user2 group1,group2",
+			UserGroup{User: "user3", Groups: []string{"group3", "group1"}},
+			true,
+		},
+		{
+			"user1,user2 group1,group2",
+			UserGroup{User: "user3", Groups: []string{"group3"}},
+			false,
+		},
+		{
+			"*",
+			UserGroup{User: "", Groups: nil},
+			true,
+		},
+		{
+			"*",
+			UserGroup{User: "user1", Groups: []string{"group1"}},
+			true,
+		},
+		{
+			acl:      "",
+			visitor:  UserGroup{User: "", Groups: nil},
+			expected: false,

Review Comment:
   nit: remove struct field names



##########
pkg/common/security/acl_test.go:
##########
@@ -19,119 +19,206 @@
 package security
 
 import (
+	"errors"
+	"fmt"
 	"testing"
-
-	"gotest.tools/assert"
 )
 
-func TestACLCreate(t *testing.T) {
-	_, err := NewACL("")
-	if err != nil {
-		t.Errorf("parsing failed for string: ''")
-	}
-	_, err = NewACL(" ")
-	if err != nil {
-		t.Errorf("parsing failed for string: ' '")
-	}
-	_, err = NewACL("user1")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1'")
-	}
-	_, err = NewACL("user1,user2")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2'")
-	}
-	_, err = NewACL("user1,user2 ")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 '")
-	}
-	_, err = NewACL("user1,user2 group1")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 group1'")
-	}
-	_, err = NewACL("user1,user2 group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 group1,group2'")
-	}
-	_, err = NewACL("user2 group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user2 group1,group2'")
-	}
-	_, err = NewACL(" group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: ' group1,group2'")
-	}
-	_, err = NewACL("* group1,group2")
-	if err != nil {
-		t.Errorf("parsing failed for string: '* group1,group2'")
-	}
-	_, err = NewACL("user1,user2 *")
-	if err != nil {
-		t.Errorf("parsing failed for string: 'user1,user2 *'")
-	}
-	_, err = NewACL("*")
-	if err != nil {
-		t.Errorf("parsing failed for string: '*'")
-	}
-	_, err = NewACL("* ")
-	if err != nil {
-		t.Errorf("parsing failed for string: '* '")
-	}
-	_, err = NewACL(" *")
-	if err != nil {
-		t.Errorf("parsing failed for string: ' *'")
+func IsSameACL(got, expected ACL) error {
+	if got.allAllowed != expected.allAllowed {
+		return errors.New("allAllowed is not same")
+	}
+	if expected.users != nil && got.users != nil {
+		for username, expectedAllow := range expected.users {
+			if gotAllow, ok := got.users[username]; !ok {
+				return fmt.Errorf("username %s does not exist", username)
+			} else if expectedAllow != gotAllow {
+				return fmt.Errorf("username %s is not same", username)
+			}
+		}
+	} else if (expected.users != nil && got.users == nil) || (expected.users == nil && got.users != nil) {
+		return errors.New("users of an acl is not nil, expect one and got one should be same")
 	}
+
+	if expected.groups != nil && got.groups != nil {
+		for groupname, expectedAllow := range expected.groups {
+			if gotAllow, ok := got.groups[groupname]; !ok {
+				return fmt.Errorf("groupname %s does not exist", groupname)
+			} else if expectedAllow != gotAllow {
+				return fmt.Errorf("groupname %s is not same", groupname)
+			}
+		}
+	} else if (expected.groups != nil && got.groups == nil) || (expected.groups == nil && got.groups != nil) {
+		return errors.New("groups of an acl is not nil, expect one and got one should be same")
+	}
+	return nil
 }
 
-func TestACLSpecialCase(t *testing.T) {
-	acl, err := NewACL("  ")
-	if err == nil {
-		t.Errorf("parsing passed for string: '  '")
-	}
+func TestACLCreate(t *testing.T) {
+	tests := []struct {
+		input    string
+		expected ACL
+	}{
+		{
+			"",
+			ACL{allAllowed: false}},
+		{
+			" ",
+			ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: false}},
+		{
+			"user1",
+			ACL{users: map[string]bool{"user1": true}, allAllowed: false}},
+		{
+			"user1,user2",
+			ACL{users: map[string]bool{"user1": true, "user2": true}, allAllowed: false}},
+		{
+			"user1,user2 ",
+			ACL{users: map[string]bool{"user1": true, "user2": true}, groups: make(map[string]bool), allAllowed: false}},
+		{
+			"user1,user2 group1",
+			ACL{users: map[string]bool{"user1": true, "user2": true}, groups: map[string]bool{"group1": true}, allAllowed: false},
+		},
+		{
+			"user1,user2 group1,group2",
+			ACL{users: map[string]bool{"user1": true, "user2": true}, groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+		},
+		{
+			"user2 group1,group2",
+			ACL{users: map[string]bool{"user2": true}, groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+		},
+		{
+			" group1,group2",
+			ACL{users: make(map[string]bool), groups: map[string]bool{"group1": true, "group2": true}, allAllowed: false},
+		},
+		{
+			"* group1,group2",
+			ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+		},
+		{
+			"user1,user2 *",
+			ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+		},
+		{
+			"*",
+			ACL{users: make(map[string]bool), allAllowed: true},
+		},
+		{
+			"* ",
+			ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+		},
+		{
+			" *",
+			ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: true},
+		},
+		{
+			"dotted.user",
+			ACL{users: map[string]bool{"dotted.user": true}, allAllowed: false},
+		},
+		{
+			"user,user",
+			ACL{users: map[string]bool{"user": true}, allAllowed: false},
+		},
+		{
+			" dotted.group",
+			ACL{users: make(map[string]bool), groups: make(map[string]bool), allAllowed: false},
+		},
+		{
+			" group,group",
+			ACL{users: make(map[string]bool), groups: map[string]bool{"group": true}, allAllowed: false},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.input, func(t *testing.T) {
+			got, err := NewACL(tt.input)
+			if err != nil {
+				t.Errorf("parsing failed for string: %s", tt.input)
+			}
 
-	acl, err = NewACL("dotted.user")
-	if err != nil || len(acl.users) != 1 {
-		t.Errorf("parsing failed for string: 'dotted.user' acl has incorrect user list: %v", acl)
-	}
-	acl, err = NewACL("user,user")
-	if err != nil || len(acl.users) != 1 {
-		t.Errorf("parsing failed for string: 'user,user' acl has incorrect user list: %v", acl)
+			if err = IsSameACL(got, tt.expected); err != nil {
+				t.Error(err.Error())
+			}
+		})
 	}
-	acl, err = NewACL(" dotted.group")
-	if err != nil || len(acl.groups) > 0 {
-		t.Errorf("parsing failed for string: ' dotted.group' acl has incorrect group list: %v", acl)
-	}
-	acl, err = NewACL(" group,group")
-	if err != nil || len(acl.groups) != 1 {
-		t.Errorf("parsing failed for string: 'group,group' acl has incorrect group list: %v", acl)
+}
+
+func TestNewACLErrorCase(t *testing.T) {
+	tests := []struct {
+		caseName string
+		acl      string
+	}{
+		{"spaces", "  "},
+		{"number of spaces is higher than 1 is not allowed", " a b "},
+	}
+	for _, tt := range tests {
+		t.Run(tt.caseName, func(t *testing.T) {
+			if _, err := NewACL(tt.acl); err == nil {
+				t.Errorf("parsing %s string should be failed", tt.acl)
+			}
+		})
 	}
 }
 
 func TestACLAccess(t *testing.T) {
-	acl, err := NewACL("user1,user2 group1,group2")
-	assert.NilError(t, err, "parsing failed for string: 'user1,user2 group1,group2'")
-	user := UserGroup{User: "", Groups: nil}
-	assert.Assert(t, !acl.CheckAccess(user), "no user, should have been denied")
-	user = UserGroup{User: "user1", Groups: nil}
-	assert.Assert(t, acl.CheckAccess(user), "user1 (no groups) should have been allowed")
-	user = UserGroup{User: "user3", Groups: []string{"group1"}}
-	assert.Assert(t, acl.CheckAccess(user), "group1 should have been allowed")
-	user = UserGroup{User: "user3", Groups: []string{"group3", "group1"}}
-	assert.Assert(t, acl.CheckAccess(user), "group1 (2nd group) should have been allowed")
-	user = UserGroup{User: "user3", Groups: []string{"group3"}}
-	assert.Assert(t, !acl.CheckAccess(user), "user3/group3 should have been denied")
-
-	acl, err = NewACL("*")
-	assert.NilError(t, err, "parsing failed for string: '*'")
-	user = UserGroup{User: "", Groups: nil}
-	assert.Assert(t, acl.CheckAccess(user), "no user, wildcard should have been allowed")
-	user = UserGroup{User: "user1", Groups: []string{"group1"}}
-	assert.Assert(t, acl.CheckAccess(user), "user1/group1, wildcard should have been allowed")
-
-	acl, err = NewACL("")
-	assert.NilError(t, err, "parsing failed for string: ''")
-	user = UserGroup{User: "", Groups: nil}
-	assert.Assert(t, !acl.CheckAccess(user), "no user, empty ACL always deny")
-	user = UserGroup{User: "user1", Groups: []string{"group1"}}
-	assert.Assert(t, !acl.CheckAccess(user), "user1/group1, empty ACL always deny")
+	tests := []struct {
+		acl      string
+		visitor  UserGroup
+		expected bool
+	}{
+		{
+			"user1,user2 group1,group2",
+			UserGroup{User: "", Groups: nil},
+			false,
+		},
+		{
+			"user1,user2 group1,group2",
+			UserGroup{User: "user1", Groups: nil},
+			true,
+		},
+		{
+			"user1,user2 group1,group2",
+			UserGroup{User: "user3", Groups: []string{"group1"}},
+			true,
+		},
+		{
+			"user1,user2 group1,group2",
+			UserGroup{User: "user3", Groups: []string{"group3", "group1"}},
+			true,
+		},
+		{
+			"user1,user2 group1,group2",
+			UserGroup{User: "user3", Groups: []string{"group3"}},
+			false,
+		},
+		{
+			"*",
+			UserGroup{User: "", Groups: nil},
+			true,
+		},
+		{
+			"*",
+			UserGroup{User: "user1", Groups: []string{"group1"}},
+			true,
+		},
+		{
+			acl:      "",
+			visitor:  UserGroup{User: "", Groups: nil},
+			expected: false,
+		},
+		{
+			"",
+			UserGroup{User: "user1", Groups: []string{"group1"}},
+			false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(fmt.Sprintf("vistor %v, acl %s", tt.visitor, tt.acl), func(t *testing.T) {
+			acl, err := NewACL(tt.acl)
+			if err != nil {
+				t.Error(err.Error())

Review Comment:
   nit: explain that parse error is unexpected as you did before



-- 
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