From 1201edd0531b05ea75a48c1053639151c944e1d0 Mon Sep 17 00:00:00 2001 From: Chris Cotter Date: Mon, 1 Apr 2024 21:03:48 +0000 Subject: [PATCH 1/2] test(storage): add WriteTo integration test cases Update the most important integration tests for Reader to cover both Read and WriteTo. Also remove refs to deprecated ioutil. --- storage/integration_test.go | 349 +++++++++++++++++++++--------------- 1 file changed, 208 insertions(+), 141 deletions(-) diff --git a/storage/integration_test.go b/storage/integration_test.go index 0c6aedaf16ed..8b9a054cf1f8 100644 --- a/storage/integration_test.go +++ b/storage/integration_test.go @@ -30,7 +30,6 @@ import ( "fmt" "hash/crc32" "io" - "io/ioutil" "log" "math" "math/rand" @@ -291,6 +290,29 @@ func multiTransportTest(ctx context.Context, t *testing.T, } } +// Use two different reading funcs for some tests to cover both Read and WriteTo. +type readCase struct { + desc string + readFunc (func(io.Reader) ([]byte, error)) +} + +var readCases = []readCase{ + { + desc: "Read", + readFunc: func(r io.Reader) ([]byte, error) { + return io.ReadAll(r) + }, + }, + { + desc: "WriteTo", + readFunc: func(r io.Reader) ([]byte, error) { + b := new(bytes.Buffer) + _, err := io.Copy(b, r) + return b.Bytes(), err + }, + }, +} + func TestIntegration_BucketCreateDelete(t *testing.T) { ctx := skipJSONReads(context.Background(), "no reads in test") multiTransportTest(ctx, t, func(t *testing.T, ctx context.Context, _ string, prefix string, client *Client) { @@ -996,24 +1018,32 @@ func TestIntegration_ObjectsRangeReader(t *testing.T) { for _, last5 := range last5s { t.Run(last5.name, func(t *testing.T) { - wantBuf := contents[len(contents)-5:] - r, err := obj.NewRangeReader(ctx, last5.start, last5.length) - if err != nil { - t.Fatalf("Failed to make range read: %v", err) - } - defer r.Close() + // Test Read and WriteTo. + for _, c := range readCases { + t.Run(c.desc, func(t *testing.T) { + wantBuf := contents[len(contents)-5:] + r, err := obj.NewRangeReader(ctx, last5.start, last5.length) + if err != nil { + t.Fatalf("Failed to make range read: %v", err) + } + defer r.Close() - if got, want := r.Attrs.StartOffset, int64(len(contents))-5; got != want { - t.Errorf("StartOffset mismatch, got %d want %d", got, want) - } + if got, want := r.Attrs.StartOffset, int64(len(contents))-5; got != want { + t.Errorf("StartOffset mismatch, got %d want %d", got, want) + } - gotBuf := &bytes.Buffer{} - nr, _ := io.Copy(gotBuf, r) - if got, want := nr, int64(5); got != want { - t.Errorf("Body length mismatch, got %d want %d", got, want) - } else if diff := cmp.Diff(gotBuf.String(), string(wantBuf)); diff != "" { - t.Errorf("Content read does not match - got(-),want(+):\n%s", diff) + gotBuf, err := c.readFunc(r) + if err != nil { + t.Fatalf("reading object: %v", err) + } + if got, want := len(gotBuf), 5; got != want { + t.Errorf("Body length mismatch, got %d want %d", got, want) + } else if diff := cmp.Diff(string(gotBuf), string(wantBuf)); diff != "" { + t.Errorf("Content read does not match - got(-),want(+):\n%s", diff) + } + }) } + }) } }) @@ -1664,9 +1694,9 @@ func TestIntegration_ObjectCompose(t *testing.T) { t.Fatalf("new reader: %v", err) } - slurp, err := ioutil.ReadAll(r) + slurp, err := io.ReadAll(r) if err != nil { - t.Fatalf("ioutil.ReadAll: %v", err) + t.Fatalf("io.ReadAll: %v", err) } defer r.Close() if !bytes.Equal(slurp, wantContents) { @@ -1877,7 +1907,7 @@ func TestIntegration_Encoding(t *testing.T) { if err != nil { t.Fatalf("NewReader(gzip-test): %v", err) } - n, err := io.Copy(ioutil.Discard, r) + n, err := io.Copy(io.Discard, r) if err != nil { t.Errorf("io.Copy, download: %v", err) } @@ -2567,8 +2597,6 @@ func TestIntegration_WriterChunksize(t *testing.T) { func TestIntegration_ZeroSizedObject(t *testing.T) { t.Parallel() multiTransportTest(context.Background(), t, func(t *testing.T, ctx context.Context, bucket, _ string, client *Client) { - h := testHelper{t} - obj := client.Bucket(bucket).Object("zero") // Check writing it works as expected. @@ -2578,10 +2606,22 @@ func TestIntegration_ZeroSizedObject(t *testing.T) { } defer obj.Delete(ctx) - // Check we can read it too. - body := h.mustRead(obj) - if len(body) != 0 { - t.Errorf("Body is %v, want empty []byte{}", body) + // Check we can read it too. Test both with WriteTo and Read. + for _, c := range readCases { + t.Run(c.desc, func(t *testing.T) { + r, err := obj.NewReader(ctx) + if err != nil { + t.Fatalf("NewReader: %v", err) + } + body, err := c.readFunc(r) + if err != nil { + t.Fatalf("reading object: %v", err) + } + if len(body) != 0 { + t.Errorf("Body is %v, want empty []byte{}", body) + } + }) + } }) } @@ -3587,23 +3627,33 @@ func TestIntegration_ReadCRC(t *testing.T) { wantCheck: false, }, } { - obj := test.obj.ReadCompressed(test.readCompressed) - r, err := obj.NewRangeReader(ctx, test.offset, test.length) - if err != nil { - if test.wantErr { - continue + t.Run(test.desc, func(t *testing.T) { + // Test both Read and WriteTo. + for _, c := range readCases { + t.Run(c.desc, func(t *testing.T) { + obj := test.obj.ReadCompressed(test.readCompressed) + r, err := obj.NewRangeReader(ctx, test.offset, test.length) + if err != nil { + if test.wantErr { + return + } + t.Fatalf("%s: %v", test.desc, err) + } + if got, want := r.checkCRC, test.wantCheck; got != want { + t.Errorf("%s, checkCRC: got %t, want %t", test.desc, got, want) + } + _, err = c.readFunc(r) + _ = r.Close() + if err != nil { + t.Fatalf("%s: %v", test.desc, err) + } + }) + } - t.Fatalf("%s: %v", test.desc, err) - } - if got, want := r.checkCRC, test.wantCheck; got != want { - t.Errorf("%s, checkCRC: got %t, want %t", test.desc, got, want) - } - _, err = ioutil.ReadAll(r) - _ = r.Close() - if err != nil { - t.Fatalf("%s: %v", test.desc, err) - } + + }) } + }) } @@ -4472,49 +4522,57 @@ func TestIntegration_Reader(t *testing.T) { } contents[obj] = c } + // Test Reader. Cache control and last-modified are tested separately, as // the JSON and XML APIs return different values for these. for _, obj := range objects { - rc, err := b.Object(obj).NewReader(ctx) - if err != nil { - t.Errorf("Can't create a reader for %v, errored with %v", obj, err) - continue - } - if !rc.checkCRC { - t.Errorf("%v: not checking CRC", obj) - } + t.Run(obj, func(t *testing.T) { + // Test both Read and WriteTo. + for _, c := range readCases { + t.Run(c.desc, func(t *testing.T) { + rc, err := b.Object(obj).NewReader(ctx) + if err != nil { + t.Fatalf("Can't create a reader for %v, errored with %v", obj, err) + } + if !rc.checkCRC { + t.Errorf("%v: not checking CRC", obj) + } - slurp, err := ioutil.ReadAll(rc) - if err != nil { - t.Errorf("Can't ReadAll object %v, errored with %v", obj, err) - } - if got, want := slurp, contents[obj]; !bytes.Equal(got, want) { - t.Errorf("Contents (%q) = %q; want %q", obj, got, want) - } - if got, want := rc.Size(), len(contents[obj]); got != int64(want) { - t.Errorf("Size (%q) = %d; want %d", obj, got, want) - } - if got, want := rc.ContentType(), "text/plain"; got != want { - t.Errorf("ContentType (%q) = %q; want %q", obj, got, want) - } - rc.Close() + slurp, err := c.readFunc(rc) + if err != nil { + t.Errorf("Can't read object %v, errored with %v", obj, err) + } + if got, want := slurp, contents[obj]; !bytes.Equal(got, want) { + t.Errorf("Contents (%q) = %q; want %q", obj, got, want) + } + if got, want := rc.Size(), len(contents[obj]); got != int64(want) { + t.Errorf("Size (%q) = %d; want %d", obj, got, want) + } + if got, want := rc.ContentType(), "text/plain"; got != want { + t.Errorf("ContentType (%q) = %q; want %q", obj, got, want) + } + rc.Close() - // Check early close. - buf := make([]byte, 1) - rc, err = b.Object(obj).NewReader(ctx) - if err != nil { - t.Fatalf("%v: %v", obj, err) - } - _, err = rc.Read(buf) - if err != nil { - t.Fatalf("%v: %v", obj, err) - } - if got, want := buf, contents[obj][:1]; !bytes.Equal(got, want) { - t.Errorf("Contents[0] (%q) = %q; want %q", obj, got, want) - } - if err := rc.Close(); err != nil { - t.Errorf("%v Close: %v", obj, err) - } + // Check early close. + buf := make([]byte, 1) + rc, err = b.Object(obj).NewReader(ctx) + if err != nil { + t.Fatalf("%v: %v", obj, err) + } + _, err = rc.Read(buf) + if err != nil { + t.Fatalf("%v: %v", obj, err) + } + if got, want := buf, contents[obj][:1]; !bytes.Equal(got, want) { + t.Errorf("Contents[0] (%q) = %q; want %q", obj, got, want) + } + if err := rc.Close(); err != nil { + t.Errorf("%v Close: %v", obj, err) + } + }) + } + + }) } obj := objects[0] @@ -4536,40 +4594,44 @@ func TestIntegration_Reader(t *testing.T) { {"-object length offset", -objlen, -1, objlen}, {"-half of object length offset", -(objlen / 2), -1, objlen / 2}, } { - rc, err := b.Object(obj).NewRangeReader(ctx, r.offset, r.length) - if err != nil { - t.Errorf("%+v: Can't create a range reader for %v, errored with %v", r.desc, obj, err) - continue - } - if rc.Size() != objlen { - t.Errorf("%+v: Reader has a content-size of %d, want %d", r.desc, rc.Size(), objlen) - } - if rc.Remain() != r.want { - t.Errorf("%+v: Reader's available bytes reported as %d, want %d", r.desc, rc.Remain(), r.want) - } - slurp, err := ioutil.ReadAll(rc) - if err != nil { - t.Errorf("%+v: can't ReadAll object %v, errored with %v", r, obj, err) - continue - } - if len(slurp) != int(r.want) { - t.Errorf("%+v: RangeReader (%d, %d): Read %d bytes, wanted %d bytes", r.desc, r.offset, r.length, len(slurp), r.want) - continue - } + t.Run(r.desc, func(t *testing.T) { - switch { - case r.offset < 0: // The case of reading the last N bytes. - start := objlen + r.offset - if got, want := slurp, contents[obj][start:]; !bytes.Equal(got, want) { - t.Errorf("RangeReader (%d, %d) = %q; want %q", r.offset, r.length, got, want) - } + for _, c := range readCases { + t.Run(c.desc, func(t *testing.T) { + rc, err := b.Object(obj).NewRangeReader(ctx, r.offset, r.length) + if err != nil { + t.Fatalf("%+v: Can't create a range reader for %v, errored with %v", r.desc, obj, err) + } + if rc.Size() != objlen { + t.Errorf("%+v: Reader has a content-size of %d, want %d", r.desc, rc.Size(), objlen) + } + if rc.Remain() != r.want { + t.Errorf("%+v: Reader's available bytes reported as %d, want %d", r.desc, rc.Remain(), r.want) + } + slurp, err := c.readFunc(rc) + if err != nil { + t.Fatalf("%+v: can't read object %v, errored with %v", r, obj, err) + } + if len(slurp) != int(r.want) { + t.Fatalf("%+v: RangeReader (%d, %d): Read %d bytes, wanted %d bytes", r.desc, r.offset, r.length, len(slurp), r.want) + } - default: - if got, want := slurp, contents[obj][r.offset:r.offset+r.want]; !bytes.Equal(got, want) { - t.Errorf("RangeReader (%d, %d) = %q; want %q", r.offset, r.length, got, want) + switch { + case r.offset < 0: // The case of reading the last N bytes. + start := objlen + r.offset + if got, want := slurp, contents[obj][start:]; !bytes.Equal(got, want) { + t.Errorf("RangeReader (%d, %d) = %q; want %q", r.offset, r.length, got, want) + } + + default: + if got, want := slurp, contents[obj][r.offset:r.offset+r.want]; !bytes.Equal(got, want) { + t.Errorf("RangeReader (%d, %d) = %q; want %q", r.offset, r.length, got, want) + } + } + rc.Close() + }) } - } - rc.Close() + }) } objName := objects[0] @@ -4902,33 +4964,38 @@ func TestIntegration_NewReaderWithContentEncodingGzip(t *testing.T) { t.Fatalf("ContentType mismtach:\nGot: %q\nWant: %q", g, w) } - rWhole, err := obj.NewReader(ctx) - if err != nil { - t.Fatalf("Failed to create wholesome reader: %v", err) - } - blobWhole, err := ioutil.ReadAll(rWhole) - rWhole.Close() - if err != nil { - t.Fatalf("Failed to read the whole body: %v", err) - } - if g, w := blobWhole, original; !bytes.Equal(g, w) { - t.Fatalf("Body mismatch\nGot:\n%s\n\nWant:\n%s", g, w) - } + // Test both Read and WriteTo. + for _, c := range readCases { + t.Run(c.desc, func(t *testing.T) { + rWhole, err := obj.NewReader(ctx) + if err != nil { + t.Fatalf("Failed to create wholesome reader: %v", err) + } + blobWhole, err := c.readFunc(rWhole) + rWhole.Close() + if err != nil { + t.Fatalf("Failed to read the whole body: %v", err) + } + if g, w := blobWhole, original; !bytes.Equal(g, w) { + t.Fatalf("Body mismatch\nGot:\n%s\n\nWant:\n%s", g, w) + } - // Now try a range read, which should return the whole body anyways since - // for decompressive transcoding, range requests ARE IGNORED by Cloud Storage. - r2kBTo3kB, err := obj.NewRangeReader(ctx, 2<<10, 3<<10) - if err != nil { - t.Fatalf("Failed to create range reader: %v", err) - } - blob2kBTo3kB, err := ioutil.ReadAll(r2kBTo3kB) - r2kBTo3kB.Close() - if err != nil { - t.Fatalf("Failed to read with the 2kB to 3kB range request: %v", err) - } - // The ENTIRE body MUST be served back regardless of the requested range. - if g, w := blob2kBTo3kB, original; !bytes.Equal(g, w) { - t.Fatalf("Body mismatch\nGot:\n%s\n\nWant:\n%s", g, w) + // Now try a range read, which should return the whole body anyways since + // for decompressive transcoding, range requests ARE IGNORED by Cloud Storage. + r2kBTo3kB, err := obj.NewRangeReader(ctx, 2<<10, 3<<10) + if err != nil { + t.Fatalf("Failed to create range reader: %v", err) + } + blob2kBTo3kB, err := c.readFunc(r2kBTo3kB) + r2kBTo3kB.Close() + if err != nil { + t.Fatalf("Failed to read with the 2kB to 3kB range request: %v", err) + } + // The ENTIRE body MUST be served back regardless of the requested range. + if g, w := blob2kBTo3kB, original; !bytes.Equal(g, w) { + t.Fatalf("Body mismatch\nGot:\n%s\n\nWant:\n%s", g, w) + } + }) } }) } @@ -5508,7 +5575,7 @@ func verifyPostPolicy(pv4 *PostPolicyV4, obj *ObjectHandle, bytesToWrite []byte, blob, _ := httputil.DumpResponse(res, true) return fmt.Errorf("Status code in response mismatch: got %d want %d\nBody: %s", g, w, blob) } - io.Copy(ioutil.Discard, res.Body) + io.Copy(io.Discard, res.Body) // Verify that the file was properly uploaded // by reading back its attributes and content @@ -5528,7 +5595,7 @@ func verifyPostPolicy(pv4 *PostPolicyV4, obj *ObjectHandle, bytesToWrite []byte, if err != nil { return fmt.Errorf("Failed to create a reader: %v", err) } - gotBody, err := ioutil.ReadAll(rd) + gotBody, err := io.ReadAll(rd) if err != nil { return fmt.Errorf("Failed to read the body: %v", err) } @@ -5692,7 +5759,7 @@ func readObject(ctx context.Context, obj *ObjectHandle) ([]byte, error) { return nil, err } defer r.Close() - return ioutil.ReadAll(r) + return io.ReadAll(r) } // cleanupBuckets deletes the bucket used for testing, as well as old @@ -5804,7 +5871,7 @@ func getURL(url string, headers map[string][]string) ([]byte, error) { return nil, err } defer res.Body.Close() - bytes, err := ioutil.ReadAll(res.Body) + bytes, err := io.ReadAll(res.Body) if err != nil { return nil, err } @@ -5826,7 +5893,7 @@ func putURL(url string, headers map[string][]string, payload io.Reader) ([]byte, return nil, err } defer res.Body.Close() - bytes, err := ioutil.ReadAll(res.Body) + bytes, err := io.ReadAll(res.Body) if err != nil { return nil, err } @@ -5837,7 +5904,7 @@ func putURL(url string, headers map[string][]string, payload io.Reader) ([]byte, } func keyFileEmail(filename string) (string, error) { - bytes, err := ioutil.ReadFile(filename) + bytes, err := os.ReadFile(filename) if err != nil { return "", err } From 34d81c41cd1952443c6a60e6bc57c83b253ce50a Mon Sep 17 00:00:00 2001 From: Chris Cotter Date: Wed, 3 Apr 2024 02:05:44 +0000 Subject: [PATCH 2/2] fix review commnets --- storage/integration_test.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/storage/integration_test.go b/storage/integration_test.go index 8b9a054cf1f8..63015152df9a 100644 --- a/storage/integration_test.go +++ b/storage/integration_test.go @@ -298,10 +298,8 @@ type readCase struct { var readCases = []readCase{ { - desc: "Read", - readFunc: func(r io.Reader) ([]byte, error) { - return io.ReadAll(r) - }, + desc: "Read", + readFunc: io.ReadAll, }, { desc: "WriteTo", @@ -2621,7 +2619,6 @@ func TestIntegration_ZeroSizedObject(t *testing.T) { t.Errorf("Body is %v, want empty []byte{}", body) } }) - } }) } @@ -3650,12 +3647,9 @@ func TestIntegration_ReadCRC(t *testing.T) { }) } - }) } - }) - } func TestIntegration_CancelWrite(t *testing.T) {