Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use testify instead of t.Fatal or t.Error in server package (part 1) #18971

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: use testify instead of t.Fatal or t.Error in server package (part 1) #18971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 5 additions & 16 deletions server/auth/jwt_test.go
View file
Open in desktop
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, uint64(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
Open in desktop
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
Open in desktop
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
Loading