Skip to content

Commit

Permalink
fix: use testify instead of t.Fatal or t.Error in server package
Browse files Browse the repository at this point in the history 
Signed-off-by: Matthieu MOREL <[email protected]>
  • Loading branch information
@mmorel-35
mmorel-35 committed Dec 4, 2024
1 parent 2b32307 commit bd41130
Show file tree
Hide file tree
Showing 18 changed files with 129 additions and 296 deletions.
21 changes: 5 additions & 16 deletions server/auth/jwt_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package auth

import (
"context"
"errors"
"fmt"
"testing"
"time"
Expand Down Expand Up @@ -103,12 +102,8 @@ func testJWTInfo(t *testing.T, opts map[string]string) {
t.Fatalf("%#v", aerr)
}
ai, ok := jwt.info(ctx, token, 123)
if !ok {
t.Fatalf("failed to authenticate with token %s", token)
}
if ai.Revision != 123 {
t.Fatalf("expected revision 123, got %d", ai.Revision)
}
require.Truef(t, ok, "failed to authenticate with token %s", token)
require.Equalf(t, 123, ai.Revision, "expected revision 123, got %d", ai.Revision)
ai, ok = jwt.info(ctx, "aaa", 120)
if ok || ai != nil {
t.Fatalf("expected aaa to fail to authenticate, got %+v", ai)
Expand All @@ -128,21 +123,15 @@ func testJWTInfo(t *testing.T, opts map[string]string) {
}

ai, ok := verify.info(ctx, token, 123)
if !ok {
t.Fatalf("failed to authenticate with token %s", token)
}
if ai.Revision != 123 {
t.Fatalf("expected revision 123, got %d", ai.Revision)
}
require.Truef(t, ok, "failed to authenticate with token %s", token)
require.Equalf(t, uint64(123), ai.Revision, "expected revision 123, got %d", ai.Revision)
ai, ok = verify.info(ctx, "aaa", 120)
if ok || ai != nil {
t.Fatalf("expected aaa to fail to authenticate, got %+v", ai)
}

_, aerr := verify.assign(ctx, "abc", 123)
if !errors.Is(aerr, ErrVerifyOnly) {
t.Fatalf("unexpected error when attempting to sign with public key: %v", aerr)
}
require.ErrorIsf(t, aerr, ErrVerifyOnly, "unexpected error when attempting to sign with public key: %v", aerr)
})
}
}
Expand Down
152 changes: 44 additions & 108 deletions server/auth/store_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"time"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/zap/zaptest"
"golang.org/x/crypto/bcrypt"
"google.golang.org/grpc/metadata"
Expand Down Expand Up @@ -64,9 +65,7 @@ func TestNewAuthStoreRevision(t *testing.T) {
defer as.Close()
new := as.Revision()

if old != new {
t.Fatalf("expected revision %d, got %d", old, new)
}
require.Equalf(t, old, new, "expected revision %d, got %d", old, new)
}

// TestNewAuthStoreBcryptCost ensures that NewAuthStore uses default when given bcrypt-cost is invalid
Expand All @@ -80,9 +79,7 @@ func TestNewAuthStoreBcryptCost(t *testing.T) {
for _, invalidCost := range invalidCosts {
as := NewAuthStore(zaptest.NewLogger(t), newBackendMock(), tp, invalidCost)
defer as.Close()
if as.BcryptCost() != bcrypt.DefaultCost {
t.Fatalf("expected DefaultCost when bcryptcost is invalid")
}
require.Equalf(t, bcrypt.DefaultCost, as.BcryptCost(), "expected DefaultCost when bcryptcost is invalid")
}
}

Expand Down Expand Up @@ -162,22 +159,17 @@ func TestUserAdd(t *testing.T) {
const userName = "foo"
ua := &pb.AuthUserAddRequest{Name: userName, Options: &authpb.UserAddOptions{NoPassword: false}}
_, err := as.UserAdd(ua) // add an existing user
if err == nil {
t.Fatalf("expected %v, got %v", ErrUserAlreadyExist, err)
}
if !errors.Is(err, ErrUserAlreadyExist) {
t.Fatalf("expected %v, got %v", ErrUserAlreadyExist, err)
}
require.Errorf(t, err, "expected %v, got %v", ErrUserAlreadyExist, err)
require.ErrorIsf(t, err, ErrUserAlreadyExist, "expected %v, got %v", ErrUserAlreadyExist, err)

ua = &pb.AuthUserAddRequest{Name: "", Options: &authpb.UserAddOptions{NoPassword: false}}
_, err = as.UserAdd(ua) // add a user with empty name
if !errors.Is(err, ErrUserEmpty) {
t.Fatal(err)
}

if _, ok := as.rangePermCache[userName]; !ok {
t.Fatalf("user %s should be added but it doesn't exist in rangePermCache", userName)
}
_, ok := as.rangePermCache[userName]
require.Truef(t, ok, "user %s should be added but it doesn't exist in rangePermCache", userName)
}

func TestRecover(t *testing.T) {
Expand All @@ -188,9 +180,7 @@ func TestRecover(t *testing.T) {
as.enabled = false
as.Recover(as.be)

if !as.IsAuthEnabled() {
t.Fatalf("expected auth enabled got disabled")
}
require.Truef(t, as.IsAuthEnabled(), "expected auth enabled got disabled")
}

func TestRecoverWithEmptyRangePermCache(t *testing.T) {
Expand All @@ -202,19 +192,13 @@ func TestRecoverWithEmptyRangePermCache(t *testing.T) {
as.rangePermCache = map[string]*unifiedRangePermissions{}
as.Recover(as.be)

if !as.IsAuthEnabled() {
t.Fatalf("expected auth enabled got disabled")
}
require.Truef(t, as.IsAuthEnabled(), "expected auth enabled got disabled")

if len(as.rangePermCache) != 3 {
t.Fatalf("rangePermCache should have permission information for 3 users (\"root\" and \"foo\",\"foo-no-user-options\"), but has %d information", len(as.rangePermCache))
}
if _, ok := as.rangePermCache["root"]; !ok {
t.Fatal("user \"root\" should be created by setupAuthStore() but doesn't exist in rangePermCache")
}
if _, ok := as.rangePermCache["foo"]; !ok {
t.Fatal("user \"foo\" should be created by setupAuthStore() but doesn't exist in rangePermCache")
}
require.Lenf(t, as.rangePermCache, 3, "rangePermCache should have permission information for 3 users (\"root\" and \"foo\",\"foo-no-user-options\"), but has %d information", len(as.rangePermCache))
_, ok := as.rangePermCache["root"]
require.Truef(t, ok, "user \"root\" should be created by setupAuthStore() but doesn't exist in rangePermCache")
_, ok = as.rangePermCache["foo"]
require.Truef(t, ok, "user \"foo\" should be created by setupAuthStore() but doesn't exist in rangePermCache")
}

func TestCheckPassword(t *testing.T) {
Expand All @@ -223,12 +207,8 @@ func TestCheckPassword(t *testing.T) {

// auth a non-existing user
_, err := as.CheckPassword("foo-test", "bar")
if err == nil {
t.Fatalf("expected %v, got %v", ErrAuthFailed, err)
}
if !errors.Is(err, ErrAuthFailed) {
t.Fatalf("expected %v, got %v", ErrAuthFailed, err)
}
require.Errorf(t, err, "expected %v, got %v", ErrAuthFailed, err)
require.ErrorIsf(t, err, ErrAuthFailed, "expected %v, got %v", ErrAuthFailed, err)

// auth an existing user with correct password
_, err = as.CheckPassword("foo", "bar")
Expand All @@ -238,12 +218,8 @@ func TestCheckPassword(t *testing.T) {

// auth an existing user but with wrong password
_, err = as.CheckPassword("foo", "")
if err == nil {
t.Fatalf("expected %v, got %v", ErrAuthFailed, err)
}
if !errors.Is(err, ErrAuthFailed) {
t.Fatalf("expected %v, got %v", ErrAuthFailed, err)
}
require.Errorf(t, err, "expected %v, got %v", ErrAuthFailed, err)
require.ErrorIsf(t, err, ErrAuthFailed, "expected %v, got %v", ErrAuthFailed, err)
}

func TestUserDelete(t *testing.T) {
Expand All @@ -260,16 +236,11 @@ func TestUserDelete(t *testing.T) {

// delete a non-existing user
_, err = as.UserDelete(ud)
if err == nil {
t.Fatalf("expected %v, got %v", ErrUserNotFound, err)
}
if !errors.Is(err, ErrUserNotFound) {
t.Fatalf("expected %v, got %v", ErrUserNotFound, err)
}
require.Errorf(t, err, "expected %v, got %v", ErrUserNotFound, err)
require.ErrorIsf(t, err, ErrUserNotFound, "expected %v, got %v", ErrUserNotFound, err)

if _, ok := as.rangePermCache[userName]; ok {
t.Fatalf("user %s should be deleted but it exists in rangePermCache", userName)
}
_, ok := as.rangePermCache[userName]
require.Falsef(t, ok, "user %s should be deleted but it exists in rangePermCache", userName)
}

func TestUserDeleteAndPermCache(t *testing.T) {
Expand All @@ -286,13 +257,10 @@ func TestUserDeleteAndPermCache(t *testing.T) {

// delete a non-existing user
_, err = as.UserDelete(ud)
if !errors.Is(err, ErrUserNotFound) {
t.Fatalf("expected %v, got %v", ErrUserNotFound, err)
}
require.ErrorIsf(t, err, ErrUserNotFound, "expected %v, got %v", ErrUserNotFound, err)

if _, ok := as.rangePermCache[deletedUserName]; ok {
t.Fatalf("user %s should be deleted but it exists in rangePermCache", deletedUserName)
}
_, ok := as.rangePermCache[deletedUserName]
require.Falsef(t, ok, "user %s should be deleted but it exists in rangePermCache", deletedUserName)

// add a new user
const newUser = "bar"
Expand All @@ -302,9 +270,8 @@ func TestUserDeleteAndPermCache(t *testing.T) {
t.Fatal(err)
}

if _, ok := as.rangePermCache[newUser]; !ok {
t.Fatalf("user %s should exist but it doesn't exist in rangePermCache", deletedUserName)
}
_, ok = as.rangePermCache[newUser]
require.Truef(t, ok, "user %s should exist but it doesn't exist in rangePermCache", deletedUserName)
}

func TestUserChangePassword(t *testing.T) {
Expand All @@ -330,12 +297,8 @@ func TestUserChangePassword(t *testing.T) {

// change a non-existing user
_, err = as.UserChangePassword(&pb.AuthUserChangePasswordRequest{Name: "foo-test", HashedPassword: encodePassword("bar")})
if err == nil {
t.Fatalf("expected %v, got %v", ErrUserNotFound, err)
}
if !errors.Is(err, ErrUserNotFound) {
t.Fatalf("expected %v, got %v", ErrUserNotFound, err)
}
require.Errorf(t, err, "expected %v, got %v", ErrUserNotFound, err)
require.ErrorIsf(t, err, ErrUserNotFound, "expected %v, got %v", ErrUserNotFound, err)

// change a user（user option is nil) password
_, err = as.UserChangePassword(&pb.AuthUserChangePasswordRequest{Name: "foo-no-user-options", HashedPassword: encodePassword("bar")})
Expand Down Expand Up @@ -393,21 +356,15 @@ func TestHasRole(t *testing.T) {

// checks role reflects correctly
hr := as.HasRole("foo", "role-test")
if !hr {
t.Fatal("expected role granted, got false")
}
require.Truef(t, hr, "expected role granted, got false")

// checks non existent role
hr = as.HasRole("foo", "non-existent-role")
if hr {
t.Fatal("expected role not found, got true")
}
require.Falsef(t, hr, "expected role not found, got true")

// checks non existent user
hr = as.HasRole("nouser", "role-test")
if hr {
t.Fatal("expected user not found got true")
}
require.Falsef(t, hr, "expected user not found got true")
}

func TestIsOpPermitted(t *testing.T) {
Expand Down Expand Up @@ -470,9 +427,7 @@ func TestGetUser(t *testing.T) {
if err != nil {
t.Fatal(err)
}
if u == nil {
t.Fatal("expect user not nil, got nil")
}
require.NotNilf(t, u, "expect user not nil, got nil")
expected := []string{"role-test"}

assert.Equal(t, expected, u.Roles)
Expand Down Expand Up @@ -801,18 +756,13 @@ func TestUserRevokePermission(t *testing.T) {
t.Fatal(err)
}

if _, ok := as.rangePermCache[userName]; !ok {
t.Fatalf("User %s should have its entry in rangePermCache", userName)
}
_, ok := as.rangePermCache[userName]
require.Truef(t, ok, "User %s should have its entry in rangePermCache", userName)
unifiedPerm := as.rangePermCache[userName]
pt1 := adt.NewBytesAffinePoint([]byte("WriteKeyBegin"))
if !unifiedPerm.writePerms.Contains(pt1) {
t.Fatal("rangePermCache should contain WriteKeyBegin")
}
require.Truef(t, unifiedPerm.writePerms.Contains(pt1), "rangePermCache should contain WriteKeyBegin")
pt2 := adt.NewBytesAffinePoint([]byte("OutOfRange"))
if unifiedPerm.writePerms.Contains(pt2) {
t.Fatal("rangePermCache should not contain OutOfRange")
}
require.Falsef(t, unifiedPerm.writePerms.Contains(pt2), "rangePermCache should not contain OutOfRange")

u, err := as.UserGet(&pb.AuthUserGetRequest{Name: userName})
if err != nil {
Expand Down Expand Up @@ -1003,12 +953,8 @@ func TestRecoverFromSnapshot(t *testing.T) {

ua := &pb.AuthUserAddRequest{Name: "foo", Options: &authpb.UserAddOptions{NoPassword: false}}
_, err := as.UserAdd(ua) // add an existing user
if err == nil {
t.Fatalf("expected %v, got %v", ErrUserAlreadyExist, err)
}
if !errors.Is(err, ErrUserAlreadyExist) {
t.Fatalf("expected %v, got %v", ErrUserAlreadyExist, err)
}
require.Errorf(t, err, "expected %v, got %v", ErrUserAlreadyExist, err)
require.ErrorIsf(t, err, ErrUserAlreadyExist, "expected %v, got %v", ErrUserAlreadyExist, err)

ua = &pb.AuthUserAddRequest{Name: "", Options: &authpb.UserAddOptions{NoPassword: false}}
_, err = as.UserAdd(ua) // add a user with empty name
Expand All @@ -1025,9 +971,7 @@ func TestRecoverFromSnapshot(t *testing.T) {
as2 := NewAuthStore(zaptest.NewLogger(t), as.be, tp, bcrypt.MinCost)
defer as2.Close()

if !as2.IsAuthEnabled() {
t.Fatal("recovering authStore from existing backend failed")
}
require.Truef(t, as2.IsAuthEnabled(), "recovering authStore from existing backend failed")

ul, err := as.UserList(&pb.AuthUserListRequest{})
if err != nil {
Expand Down Expand Up @@ -1167,9 +1111,7 @@ func testAuthInfoFromCtxWithRoot(t *testing.T, opts string) {
if aerr != nil {
t.Fatal(err)
}
if ai == nil {
t.Fatal("expected non-nil *AuthInfo")
}
require.NotNilf(t, ai, "expected non-nil *AuthInfo")
if ai.Username != "root" {
t.Errorf("expected user name 'root', got %+v", ai)
}
Expand All @@ -1188,9 +1130,7 @@ func TestUserNoPasswordAdd(t *testing.T) {

ctx := context.WithValue(context.WithValue(context.TODO(), AuthenticateParamIndex{}, uint64(1)), AuthenticateParamSimpleTokenPrefix{}, "dummy")
_, err = as.Authenticate(ctx, username, "")
if !errors.Is(err, ErrAuthFailed) {
t.Fatalf("expected %v, got %v", ErrAuthFailed, err)
}
require.ErrorIsf(t, err, ErrAuthFailed, "expected %v, got %v", ErrAuthFailed, err)
}

func TestUserAddWithOldLog(t *testing.T) {
Expand Down Expand Up @@ -1227,10 +1167,6 @@ func TestUserChangePasswordWithOldLog(t *testing.T) {

// change a non-existing user
_, err = as.UserChangePassword(&pb.AuthUserChangePasswordRequest{Name: "foo-test", HashedPassword: encodePassword("bar")})
if err == nil {
t.Fatalf("expected %v, got %v", ErrUserNotFound, err)
}
if !errors.Is(err, ErrUserNotFound) {
t.Fatalf("expected %v, got %v", ErrUserNotFound, err)
}
require.Errorf(t, err, "expected %v, got %v", ErrUserNotFound, err)
require.ErrorIsf(t, err, ErrUserNotFound, "expected %v, got %v", ErrUserNotFound, err)
}
13 changes: 4 additions & 9 deletions server/config/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"net/url"
"testing"

"github.com/stretchr/testify/require"
"go.uber.org/zap/zaptest"

"go.etcd.io/etcd/client/pkg/v3/types"
Expand All @@ -28,9 +29,7 @@ func mustNewURLs(t *testing.T, urls []string) []url.URL {
return nil
}
u, err := types.NewURLs(urls)
if err != nil {
t.Fatalf("error creating new URLs from %q: %v", urls, err)
}
require.NoErrorf(t, err, "error creating new URLs from %q: %v", urls, err)
return u
}

Expand All @@ -48,9 +47,7 @@ func TestConfigVerifyBootstrapWithoutClusterAndDiscoveryURLFail(t *testing.T) {

func TestConfigVerifyExistingWithDiscoveryURLFail(t *testing.T) {
cluster, err := types.NewURLsMap("node1=http://127.0.0.1:2380")
if err != nil {
t.Fatalf("NewCluster error: %v", err)
}
require.NoErrorf(t, err, "NewCluster error: %v", err)
c := &ServerConfig{
Name: "node1",
DiscoveryURL: "http://127.0.0.1:2379/abcdefg",
Expand Down Expand Up @@ -139,9 +136,7 @@ func TestConfigVerifyLocalMember(t *testing.T) {

for i, tt := range tests {
cluster, err := types.NewURLsMap(tt.clusterSetting)
if err != nil {
t.Fatalf("#%d: Got unexpected error: %v", i, err)
}
require.NoErrorf(t, err, "#%d: Got unexpected error: %v", i, err)
cfg := ServerConfig{
Name: "node1",
InitialPeerURLsMap: cluster,
Expand Down
Loading

0 comments on commit bd41130

Please sign in to comment.