[#180] node: Refactor panics in unit test #182

Merged
fyrchik merged 1 commit from aarifullin/frostfs-node:feature/180-factor_out_panics into master 2023-03-29 10:33:07 +00:00
30 changed files with 76 additions and 79 deletions

View file

@ -32,6 +32,6 @@ func TestApiclientSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -56,6 +56,6 @@ func TestContractsSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -32,6 +32,6 @@ func TestControlSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -167,6 +167,6 @@ func TestEngineSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -49,6 +49,6 @@ func TestGRPCSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -25,6 +25,6 @@ func TestLoggerSection_Level(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -34,6 +34,6 @@ func TestMetricsSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -40,6 +40,6 @@ func TestMorphSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -162,6 +162,6 @@ func TestNodeSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -29,6 +29,6 @@ func TestObjectSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -26,6 +26,6 @@ func TestPolicerSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -34,6 +34,6 @@ func TestProfilerSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -28,6 +28,6 @@ func TestReplicatorSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -4,8 +4,10 @@ import (
"bufio"
"os"
"strings"
"testing"
"git.frostfs.info/TrueCloudLab/frostfs-node/cmd/frostfs-node/config"
"github.com/stretchr/testify/require"
)
func fromFile(path string) *config.Config {
@ -18,10 +20,10 @@ func fromFile(path string) *config.Config {
)
}
func fromEnvFile(path string) *config.Config {
func fromEnvFile(t testing.TB, path string) *config.Config {
var p config.Prm
loadEnv(path) // github.com/joho/godotenv can do that as well
loadEnv(t, path) // github.com/joho/godotenv can do that as well
return config.New(p)
}
@ -43,8 +45,8 @@ func ForEachFileType(pref string, f func(*config.Config)) {
}
// ForEnvFileType creates config from `<pref>.env` file.
func ForEnvFileType(pref string, f func(*config.Config)) {
f(fromEnvFile(pref + ".env"))
func ForEnvFileType(t testing.TB, pref string, f func(*config.Config)) {
f(fromEnvFile(t, pref+".env"))
}
// EmptyConfig returns config without any values and sections.
@ -55,11 +57,9 @@ func EmptyConfig() *config.Config {
}
// loadEnv reads .env file, parses `X=Y` records and sets OS ENVs.
func loadEnv(path string) {
func loadEnv(t testing.TB, path string) {
f, err := os.Open(path)
if err != nil {
panic("can't open .env file")
}
require.NoError(t, err, "can't open .env file")
defer f.Close()
@ -73,8 +73,6 @@ func loadEnv(path string) {
v = strings.Trim(v, `"`)
err = os.Setenv(k, v)
if err != nil {
panic("can't set environment variable")
}
require.NoError(t, err, "can't set environment variable")
}
}

View file

@ -39,6 +39,6 @@ func TestTreeSection(t *testing.T) {
configtest.ForEachFileType(path, fileConfigTest)
t.Run("ENV", func(t *testing.T) {
configtest.ForEnvFileType(path, fileConfigTest)
configtest.ForEnvFileType(t, path, fileConfigTest)
})
}

View file

@ -64,7 +64,7 @@ func TestBlobovnicza(t *testing.T) {
WithPath(p),
WithObjectSizeLimit(objSizeLim),
WithFullSizeLimit(sizeLim),
WithLogger(test.NewLogger(false)),
WithLogger(test.NewLogger(t, false)),
)
defer os.Remove(p)

View file

@ -28,7 +28,7 @@ func TestGetRange(t *testing.T, cons Constructor, min, max uint64) {
var start, stop uint64 = 11, 100
if uint64(len(payload)) < stop {
panic("unexpected: invalid test object generated")
t.Fatalf("unexpected: invalid test object generated")
}
var gPrm common.GetRangePrm

View file

@ -103,7 +103,7 @@ func BenchmarkSubstorageReadPerf(b *testing.B) {
// Fill database
for i := 0; i < tt.size; i++ {
obj := objGen.Next()
addr := testutil.AddressFromObject(obj)
addr := testutil.AddressFromObject(b, obj)
raw, err := obj.Marshal()
require.NoError(b, err)
if _, err := st.Put(common.PutPrm{
@ -159,7 +159,7 @@ func BenchmarkSubstorageWritePerf(b *testing.B) {
b.RunParallel(func(pb *testing.PB) {
for pb.Next() {
obj := gen.Next()
addr := testutil.AddressFromObject(obj)
addr := testutil.AddressFromObject(b, obj)
raw, err := obj.Marshal()
require.NoError(b, err)
if _, err := st.Put(common.PutPrm{
@ -202,7 +202,7 @@ func BenchmarkSubstorageIteratePerf(b *testing.B) {
// Fill database
for i := 0; i < tt.size; i++ {
obj := objGen.Next()
addr := testutil.AddressFromObject(obj)
addr := testutil.AddressFromObject(b, obj)
raw, err := obj.Marshal()
require.NoError(b, err)
if _, err := st.Put(common.PutPrm{

View file

@ -53,7 +53,7 @@ func TestDeleteBigObject(t *testing.T) {
s2 := testNewShard(t, 2)
s3 := testNewShard(t, 3)
e := testNewEngineWithShards(s1, s2, s3)
e := testNewEngineWithShards(t, s1, s2, s3)
e.log = &logger.Logger{Logger: zaptest.NewLogger(t)}
defer e.Close()

View file

@ -48,7 +48,7 @@ func benchmarkExists(b *testing.B, shardNum int) {
shards[i] = testNewShard(b, i)
}
e := testNewEngineWithShards(shards...)
e := testNewEngineWithShards(b, shards...)
b.Cleanup(func() {
_ = e.Close()
_ = os.RemoveAll(b.Name())
@ -73,14 +73,12 @@ func benchmarkExists(b *testing.B, shardNum int) {
}
}
func testNewEngineWithShards(shards ...*shard.Shard) *StorageEngine {
func testNewEngineWithShards(t testing.TB, shards ...*shard.Shard) *StorageEngine {
engine := New()
for _, s := range shards {
pool, err := ants.NewPool(10, ants.WithNonblocking(true))
if err != nil {
panic(err)
}
require.NoError(t, err)
engine.shards[s.ID().String()] = hashedShard{
shardWrapper: shardWrapper{
@ -172,5 +170,5 @@ func testNewEngineWithShardNum(t *testing.T, num int) *StorageEngine {
shards = append(shards, testNewShard(t, i))
}
return testNewEngineWithShards(shards...)
return testNewEngineWithShards(t, shards...)
}

View file

@ -44,7 +44,7 @@ func TestHeadRaw(t *testing.T) {
s1 := testNewShard(t, 1)
s2 := testNewShard(t, 2)
e := testNewEngineWithShards(s1, s2)
e := testNewEngineWithShards(t, s1, s2)
defer e.Close()
var putPrmLeft shard.PutPrm

View file

@ -59,7 +59,7 @@ func TestStorageEngine_Inhume(t *testing.T) {
s1 := testNewShard(t, 1)
s2 := testNewShard(t, 2)
e := testNewEngineWithShards(s1, s2)
e := testNewEngineWithShards(t, s1, s2)
defer e.Close()
var putChild shard.PutPrm

View file

@ -16,7 +16,7 @@ import (
func TestListWithCursor(t *testing.T) {
s1 := testNewShard(t, 1)
s2 := testNewShard(t, 2)
e := testNewEngineWithShards(s1, s2)
e := testNewEngineWithShards(t, s1, s2)
t.Cleanup(func() {
e.Close()

View file

@ -2,10 +2,12 @@ package testutil
import (
"encoding/binary"
"testing"
cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id"
"git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object"
oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id"
"github.com/stretchr/testify/require"
"go.uber.org/atomic"
"golang.org/x/exp/rand"
)
@ -94,17 +96,16 @@ func (g *OverwriteObjGenerator) Next() *object.Object {
return generateObjectWithOIDWithCIDWithSize(id, cid.ID{}, g.ObjSize)
}
func AddressFromObject(obj *object.Object) oid.Address {
func AddressFromObject(t testing.TB, obj *object.Object) oid.Address {
var addr oid.Address
if id, isSet := obj.ID(); isSet {
id, isSet := obj.ID()
require.True(t, isSet, "object ID is not set")
aarifullin marked this conversation as resolved Outdated

Can we remove an if then?

id, isSet := obj.ID()
require.True(t, isSet, "object ID is not set")

addr.SetObject(id)
Can we remove an if then? ``` id, isSet := obj.ID() require.True(t, isSet, "object ID is not set") addr.SetObject(id) ```
addr.SetObject(id)
} else {
panic("object ID is not set")
}
if cid, isSet := obj.ContainerID(); isSet {
cid, isSet := obj.ContainerID()
require.True(t, isSet, "container ID is not set")
addr.SetContainer(cid)
} else {
panic("container ID is not set")
}
return addr
}

View file

@ -39,7 +39,7 @@ func TestSeqObjGenerator(t *testing.T) {
for i := 1; i <= 10; i++ {
obj := gen.Next()
id, isSet := obj.ID()
addrs = append(addrs, AddressFromObject(obj).EncodeToString())
addrs = append(addrs, AddressFromObject(t, obj).EncodeToString())
require.True(t, isSet)
require.Equal(t, gen.ObjSize, uint64(len(obj.Payload())))

View file

@ -246,7 +246,7 @@ func TestGetLocalOnly(t *testing.T) {
newSvc := func(storage *testStorage) *Service {
svc := &Service{cfg: new(cfg)}
svc.log = test.NewLogger(false)
svc.log = test.NewLogger(t, false)
svc.localStorage = storage
return svc
@ -507,7 +507,7 @@ func TestGetRemoteSmall(t *testing.T) {
newSvc := func(b *testPlacementBuilder, c *testClientCache) *Service {
svc := &Service{cfg: new(cfg)}
svc.log = test.NewLogger(false)
svc.log = test.NewLogger(t, false)
svc.localStorage = newTestStorage()
const curEpoch = 13
@ -1640,7 +1640,7 @@ func TestGetFromPastEpoch(t *testing.T) {
c22.addResult(addr, obj, nil)
svc := &Service{cfg: new(cfg)}
svc.log = test.NewLogger(false)
svc.log = test.NewLogger(t, false)
svc.localStorage = newTestStorage()
const curEpoch = 13

View file

@ -152,7 +152,7 @@ func TestGetLocalOnly(t *testing.T) {
newSvc := func(storage *testStorage) *Service {
svc := &Service{cfg: new(cfg)}
svc.log = test.NewLogger(false)
svc.log = test.NewLogger(t, false)
svc.localStorage = storage
return svc
@ -254,7 +254,7 @@ func TestGetRemoteSmall(t *testing.T) {
newSvc := func(b *testPlacementBuilder, c *testClientCache) *Service {
svc := &Service{cfg: new(cfg)}
svc.log = test.NewLogger(false)
svc.log = test.NewLogger(t, false)
svc.localStorage = newTestStorage()
const curEpoch = 13
@ -363,7 +363,7 @@ func TestGetFromPastEpoch(t *testing.T) {
c22.addResult(idCnr, ids22, nil)
svc := &Service{cfg: new(cfg)}
svc.log = test.NewLogger(false)
svc.log = test.NewLogger(t, false)
svc.localStorage = newTestStorage()
const curEpoch = 13
@ -476,7 +476,7 @@ func TestGetWithSessionToken(t *testing.T) {
w := new(simpleIDWriter)
svc := &Service{cfg: new(cfg)}
svc.log = test.NewLogger(false)
svc.log = test.NewLogger(t, false)
svc.localStorage = localStorage
const curEpoch = 13

View file

@ -118,7 +118,10 @@ func TestGetSubTree(t *testing.T) {
})
}
var errSubTreeSend = errors.New("test error")
var (
errSubTreeSend = errors.New("send finished with error")
ale64bit marked this conversation as resolved Outdated

even if it's a test error, can we use a more meaningful message?

even if it's a test error, can we use a more meaningful message?

Done

Done

In general error strings should not be capitalized. From https://github.com/golang/go/wiki/CodeReviewComments#error-strings

As far as I understand, we follow this rule.

*In general error strings should not be capitalized*. From https://github.com/golang/go/wiki/CodeReviewComments#error-strings As far as I understand, we follow this rule.

Yes, it's my fault. Here Send is name of the method. Just coincidence :) I'll fix it anyway

Yes, it's my fault. Here Send is name of the method. Just coincidence :) I'll fix it anyway
errSubTreeSendAfterError = errors.New("send was invoked after an error occurred")
)
type subTreeAcc struct {
grpc.ServerStream // to satisfy the interface
@ -127,6 +130,8 @@ type subTreeAcc struct {
errIndex int
}
var _ TreeService_GetSubTreeServer = &subTreeAcc{}
func (s *subTreeAcc) Send(r *GetSubTreeResponse) error {
s.seen = append(s.seen, r)
if s.errIndex >= 0 {
@ -134,7 +139,7 @@ func (s *subTreeAcc) Send(r *GetSubTreeResponse) error {
return errSubTreeSend
}
if s.errIndex >= 0 && len(s.seen) > s.errIndex {
panic("called Send after an error was returned")
return errSubTreeSendAfterError
}
}
return nil

View file

@ -1,7 +1,10 @@
package test
import (
"testing"
"git.frostfs.info/TrueCloudLab/frostfs-node/pkg/util/logger"
"github.com/stretchr/testify/require"
"go.uber.org/zap"
"go.uber.org/zap/zapcore"
)
@ -11,7 +14,7 @@ const sampling = 1000
// NewLogger creates a new logger.
//
// If debug, development logger is created.
func NewLogger(debug bool) *logger.Logger {
func NewLogger(t testing.TB, debug bool) *logger.Logger {
var l logger.Logger
l.Logger = zap.L()
@ -25,10 +28,7 @@ func NewLogger(debug bool) *logger.Logger {
cfg.EncoderConfig.EncodeLevel = zapcore.CapitalColorLevelEncoder
log, err := cfg.Build()
if err != nil {
panic("could not prepare logger: " + err.Error())
}
require.NoError(t, err, "could not prepare logger")
aarifullin marked this conversation as resolved Outdated

require.NoError?

`require.NoError`?
l.Logger = log
}

View file

@ -6,7 +6,9 @@ import (
"crypto/rand"
"crypto/x509"
"encoding/hex"
"strconv"
"testing"
"github.com/stretchr/testify/require"
)
// Keys is a list of test private keys in hex format.
@ -114,29 +116,22 @@ var Keys = []string{
}
// DecodeKey creates a test private key.
func DecodeKey(i int) *ecdsa.PrivateKey {
func DecodeKey(t testing.TB, i int) *ecdsa.PrivateKey {
if i < 0 {
key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
if err != nil {
panic("could not generate uniq key")
}
require.NoError(t, err, "could not generate uniq key")
return key
}
if current, size := i, len(Keys); current >= size {
panic("add more test keys, used " + strconv.Itoa(current) + " from " + strconv.Itoa(size))
t.Fatalf("add more test keys, used %d from %d", current, size)
}
buf, err := hex.DecodeString(Keys[i])
if err != nil {
panic("could not hex.Decode: " + err.Error())
}
require.NoError(t, err, "could not to decode hex string")
key, err := x509.ParseECPrivateKey(buf)
if err != nil {
panic("could x509.ParseECPrivateKey: " + err.Error())
}
require.NoError(t, err, "could not to parse ec private key")
return key
}