Skip to content

Commit cd07570

Browse files
tritonetritone
authored
test(storage): improve object cleanup (#8262)
Use the test helper mustDeleteObject function and make it resilient to transient failures. Also switch to using t.Helper and t.Cleanup when possible for test helper funcs. We should probably move other tests to do this at some point; could be a small fixit project. Fixes #8259
1 parent c58aaf0 commit cd07570

File tree

1 file changed

+44
-47
lines changed

1 file changed

+44
-47
lines changed

‎storage/integration_test.go‎

Lines changed: 44 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,6 @@ import (
3838
"net/http"
3939
"net/http/httputil"
4040
"os"
41-
"path/filepath"
42-
"runtime"
4341
"sort"
4442
"strconv"
4543
"strings"
@@ -272,7 +270,9 @@ func multiTransportTest(ctx context.Context, t *testing.T,
272270
opts ...option.ClientOption) {
273271
for transport, client := range initTransportClients(ctx, t, opts...) {
274272
t.Run(transport, func(t *testing.T) {
275-
defer client.Close()
273+
t.Cleanup(func() {
274+
client.Close()
275+
})
276276

277277
if reason := ctx.Value(skipTransportTestKey(transport)); reason != nil {
278278
t.Skip("transport", fmt.Sprintf("%q", transport), "explicitly skipped:", reason)
@@ -1577,6 +1577,8 @@ func TestIntegration_ObjectCompose(t *testing.T) {
15771577
func TestIntegration_Copy(t *testing.T) {
15781578
ctx := skipJSONReads(context.Background(), "no reads in test")
15791579
multiTransportTest(ctx, t, func(t *testing.T, ctx context.Context, bucket string, prefix string, client *Client) {
1580+
h := testHelper{t}
1581+
15801582
bucketFrom := client.Bucket(bucket)
15811583
bucketInSameRegion := client.Bucket(prefix + uidSpace.New())
15821584
bucketInDifferentRegion := client.Bucket(prefix + uidSpace.New())
@@ -1585,13 +1587,17 @@ func TestIntegration_Copy(t *testing.T) {
15851587
if err := bucketInSameRegion.Create(ctx, testutil.ProjID(), nil); err != nil {
15861588
t.Fatalf("bucket.Create: %v", err)
15871589
}
1588-
defer bucketInSameRegion.Delete(ctx)
1590+
t.Cleanup(func() {
1591+
h.mustDeleteBucket(bucketInSameRegion)
1592+
})
15891593

15901594
// Create new bucket
15911595
if err := bucketInDifferentRegion.Create(ctx, testutil.ProjID(), &BucketAttrs{Location: "NORTHAMERICA-NORTHEAST2"}); err != nil {
15921596
t.Fatalf("bucket.Create: %v", err)
15931597
}
1594-
defer bucketInDifferentRegion.Delete(ctx)
1598+
t.Cleanup(func() {
1599+
h.mustDeleteBucket(bucketInDifferentRegion)
1600+
})
15951601

15961602
// We use a larger object size to be able to trigger multiple rewrite calls
15971603
minObjectSize := 2500000 // 2.5 Mb
@@ -1610,12 +1616,9 @@ func TestIntegration_Copy(t *testing.T) {
16101616
if err := w.Close(); err != nil {
16111617
t.Fatalf("w.Close: %v", err)
16121618
}
1613-
1614-
defer func() {
1615-
if err := obj.Delete(ctx); err != nil {
1616-
t.Errorf("obj.Delete: %v", err)
1617-
}
1618-
}()
1619+
t.Cleanup(func() {
1620+
h.mustDeleteObject(obj)
1621+
})
16191622

16201623
attrs, err := obj.Attrs(ctx)
16211624
if err != nil {
@@ -1687,11 +1690,9 @@ func TestIntegration_Copy(t *testing.T) {
16871690
if err != nil {
16881691
t.Fatalf("Copier.Run failed with %v", err)
16891692
}
1690-
defer func() {
1691-
if err := copyObj.Delete(ctx); err != nil {
1692-
t.Errorf("copyObj.Delete: %v", err)
1693-
}
1694-
}()
1693+
t.Cleanup(func() {
1694+
h.mustDeleteObject(copyObj)
1695+
})
16951696

16961697
// Check copied object is in the correct bucket with the correct name
16971698
if attrs.Bucket != test.toBucket.name || attrs.Name != test.toObj {
@@ -5159,83 +5160,91 @@ type testHelper struct {
51595160
}
51605161

51615162
func (h testHelper) mustCreate(b *BucketHandle, projID string, attrs *BucketAttrs) {
5163+
h.t.Helper()
51625164
if err := b.Create(context.Background(), projID, attrs); err != nil {
5163-
h.t.Fatalf("%s: bucket create: %v", loc(), err)
5165+
h.t.Fatalf("bucket create: %v", err)
51645166
}
51655167
}
51665168

51675169
func (h testHelper) mustDeleteBucket(b *BucketHandle) {
5170+
h.t.Helper()
51685171
if err := b.Delete(context.Background()); err != nil {
5169-
h.t.Fatalf("%s: bucket delete: %v", loc(), err)
5172+
h.t.Fatalf("bucket delete: %v", err)
51705173
}
51715174
}
51725175

51735176
func (h testHelper) mustBucketAttrs(b *BucketHandle) *BucketAttrs {
5177+
h.t.Helper()
51745178
attrs, err := b.Attrs(context.Background())
51755179
if err != nil {
5176-
h.t.Fatalf("%s: bucket attrs: %v", loc(), err)
5180+
h.t.Fatalf("bucket attrs: %v", err)
51775181
}
51785182
return attrs
51795183
}
51805184

51815185
// updating a bucket is conditionally idempotent on metageneration, so we pass that in to enable retries
51825186
func (h testHelper) mustUpdateBucket(b *BucketHandle, ua BucketAttrsToUpdate, metageneration int64) *BucketAttrs {
5187+
h.t.Helper()
51835188
attrs, err := b.If(BucketConditions{MetagenerationMatch: metageneration}).Update(context.Background(), ua)
51845189
if err != nil {
5185-
h.t.Fatalf("%s: update: %v", loc(), err)
5190+
h.t.Fatalf("update: %v", err)
51865191
}
51875192
return attrs
51885193
}
51895194

51905195
func (h testHelper) mustObjectAttrs(o *ObjectHandle) *ObjectAttrs {
5196+
h.t.Helper()
51915197
attrs, err := o.Attrs(context.Background())
51925198
if err != nil {
5193-
h.t.Fatalf("%s: object attrs: %v", loc(), err)
5199+
h.t.Fatalf("object attrs: %v", err)
51945200
}
51955201
return attrs
51965202
}
51975203

51985204
func (h testHelper) mustDeleteObject(o *ObjectHandle) {
5199-
if err := o.Delete(context.Background()); err != nil {
5200-
h.t.Fatalf("%s: delete object %s from bucket %s: %v", loc(), o.ObjectName(), o.BucketName(), err)
5205+
h.t.Helper()
5206+
if err := o.Retryer(WithPolicy(RetryAlways)).Delete(context.Background()); err != nil {
5207+
var apiErr *apierror.APIError
5208+
if ok := errors.As(err, &apiErr); ok {
5209+
// Object may already be deleted with retry; if so skip.
5210+
if apiErr.HTTPCode() == 404 || apiErr.GRPCStatus().Code() == codes.NotFound {
5211+
return
5212+
}
5213+
}
5214+
h.t.Fatalf("delete object %s from bucket %s: %v", o.ObjectName(), o.BucketName(), err)
52015215
}
52025216
}
52035217

52045218
// updating an object is conditionally idempotent on metageneration, so we pass that in to enable retries
52055219
func (h testHelper) mustUpdateObject(o *ObjectHandle, ua ObjectAttrsToUpdate, metageneration int64) *ObjectAttrs {
5220+
h.t.Helper()
52065221
attrs, err := o.If(Conditions{MetagenerationMatch: metageneration}).Update(context.Background(), ua)
52075222
if err != nil {
5208-
h.t.Fatalf("%s: update: %v", loc(), err)
5223+
h.t.Fatalf("update: %v", err)
52095224
}
52105225
return attrs
52115226
}
52125227

52135228
func (h testHelper) mustWrite(w *Writer, data []byte) {
5229+
h.t.Helper()
52145230
if _, err := w.Write(data); err != nil {
52155231
w.Close()
5216-
h.t.Fatalf("%s: write: %v", loc(), err)
5232+
h.t.Fatalf("write: %v", err)
52175233
}
52185234
if err := w.Close(); err != nil {
5219-
h.t.Fatalf("%s: close write: %v", loc(), err)
5235+
h.t.Fatalf("close write: %v", err)
52205236
}
52215237
}
52225238

52235239
func (h testHelper) mustRead(obj *ObjectHandle) []byte {
5240+
h.t.Helper()
52245241
data, err := readObject(context.Background(), obj)
52255242
if err != nil {
5226-
h.t.Fatalf("%s: read: %v", loc(), err)
5243+
h.t.Fatalf("read: %v", err)
52275244
}
52285245
return data
52295246
}
52305247

5231-
func (h testHelper) mustNewReader(obj *ObjectHandle) *Reader {
5232-
r, err := obj.NewReader(context.Background())
5233-
if err != nil {
5234-
h.t.Fatalf("%s: new reader: %v", loc(), err)
5235-
}
5236-
return r
5237-
}
5238-
52395248
func writeObject(ctx context.Context, obj *ObjectHandle, contentType string, contents []byte) error {
52405249
w := obj.Retryer(WithPolicy(RetryAlways)).NewWriter(ctx)
52415250
w.ContentType = contentType
@@ -5249,18 +5258,6 @@ func writeObject(ctx context.Context, obj *ObjectHandle, contentType string, con
52495258
return w.Close()
52505259
}
52515260

5252-
// loc returns a string describing the file and line of its caller's call site. In
5253-
// other words, if a test function calls a helper, and the helper calls loc, then the
5254-
// string will refer to the line on which the test function called the helper.
5255-
// TODO(jba): use t.Helper once we drop go 1.6.
5256-
func loc() string {
5257-
_, file, line, ok := runtime.Caller(2)
5258-
if !ok {
5259-
return "???"
5260-
}
5261-
return fmt.Sprintf("%s:%d", filepath.Base(file), line)
5262-
}
5263-
52645261
func readObject(ctx context.Context, obj *ObjectHandle) ([]byte, error) {
52655262
r, err := obj.NewReader(ctx)
52665263
if err != nil {

0 commit comments

Comments
 (0)