Repository: google/go-cloud
Pull request #844 (2018-11-30 00:59:02)

blob/tests: add more tests for OpenBucket, OpenURL
This improves test coverage for `gcsblob` and `s3blob`.

Updates #675.


Patch:
diff --git a/blob/gcsblob/gcsblob.go b/blob/gcsblob/gcsblob.go
index a6ad8d93..95f3744c 100644
--- a/blob/gcsblob/gcsblob.go
+++ b/blob/gcsblob/gcsblob.go
@@ -122,9 +122,12 @@ type Options struct {
 }
 
 // openBucket returns a GCS Bucket that communicates using the given HTTP client.
-func openBucket(ctx context.Context, bucketName string, client *gcp.HTTPClient, opts *Options) (driver.Bucket, error) {
+func openBucket(ctx context.Context, bucketName string, client *gcp.HTTPClient, opts *Options) (*bucket, error) {
 	if client == nil {
-		return nil, errors.New("OpenBucket requires an HTTP client")
+		return nil, errors.New("gcsblob.OpenBucket: client is required")
+	}
+	if bucketName == "" {
+		return nil, errors.New("gcsblob.OpenBucket: bucketName is required")
 	}
 	c, err := storage.NewClient(ctx, option.WithHTTPClient(&client.Client))
 	if err != nil {
diff --git a/blob/s3blob/s3blob.go b/blob/s3blob/s3blob.go
index 024279c5..36f5147e 100644
--- a/blob/s3blob/s3blob.go
+++ b/blob/s3blob/s3blob.go
@@ -58,28 +58,33 @@ import (
 const defaultPageSize = 1000
 
 func init() {
-	blob.Register("s3", func(ctx context.Context, u *url.URL) (driver.Bucket, error) {
-		q := u.Query()
-		cfg := &aws.Config{}
+	blob.Register("s3", openURL)
+}
 
-		if region := q["region"]; len(region) > 0 {
-			cfg.Region = aws.String(region[0])
-		}
-		sess, err := session.NewSession(cfg)
-		if err != nil {
-			return nil, err
-		}
-		return openBucket(ctx, u.Host, sess, nil)
-	})
+func openURL(ctx context.Context, u *url.URL) (driver.Bucket, error) {
+	q := u.Query()
+	cfg := &aws.Config{}
+
+	if region := q["region"]; len(region) > 0 {
+		cfg.Region = aws.String(region[0])
+	}
+	sess, err := session.NewSession(cfg)
+	if err != nil {
+		return nil, err
+	}
+	return openBucket(ctx, u.Host, sess, nil)
 }
 
 // Options sets options for constructing a *blob.Bucket backed by fileblob.
 type Options struct{}
 
 // openBucket returns an S3 Bucket.
-func openBucket(ctx context.Context, bucketName string, sess client.ConfigProvider, _ *Options) (driver.Bucket, error) {
+func openBucket(ctx context.Context, bucketName string, sess client.ConfigProvider, _ *Options) (*bucket, error) {
 	if sess == nil {
-		return nil, errors.New("sess must be provided to get bucket")
+		return nil, errors.New("s3blob.OpenBucket: sess is required")
+	}
+	if bucketName == "" {
+		return nil, errors.New("s3blob.OpenBucket: bucketName is required")
 	}
 	return &bucket{
 		name:   bucketName,


Test patch:
diff --git a/blob/gcsblob/gcsblob_test.go b/blob/gcsblob/gcsblob_test.go
index 62726620..4c2a2491 100644
--- a/blob/gcsblob/gcsblob_test.go
+++ b/blob/gcsblob/gcsblob_test.go
@@ -122,9 +122,14 @@ func (verifyContentLanguage) BucketCheck(b *blob.Bucket) error {
 }
 
 func (verifyContentLanguage) ErrorCheck(err error) error {
-	// Can't verify this one because the storage library returns a sentinel
-	// error, storage.ErrObjectNotExist, for "not exists" instead of the
-	// supported As type googleapi.Error.
+	// Can't really verify this one because the storage library returns
+	// a sentinel error, storage.ErrObjectNotExist, for "not exists"
+	// instead of the supported As type googleapi.Error.
+	// Call ErrorAs anyway, and expect it to fail.
+	var to *googleapi.Error
+	if blob.ErrorAs(err, &to) {
+		return errors.New("expected ErrorAs to fail")
+	}
 	return nil
 }
 
@@ -204,8 +209,62 @@ func TestBufferSize(t *testing.T) {
 	}
 }
 
-func TestOpenURL(t *testing.T) {
+func TestOpenBucket(t *testing.T) {
+	tests := []struct {
+		description string
+		bucketName  string
+		nilClient   bool
+		want        string
+		wantErr     bool
+	}{
+		{
+			description: "empty bucket name results in error",
+			wantErr:     true,
+		},
+		{
+			description: "nil client results in error",
+			bucketName:  "foo",
+			nilClient:   true,
+			wantErr:     true,
+		},
+		{
+			description: "success",
+			bucketName:  "foo",
+			want:        "foo",
+		},
+	}
+
+	ctx := context.Background()
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			var client *gcp.HTTPClient
+			if !test.nilClient {
+				var done func()
+				client, _, done = setup.NewGCPClient(ctx, t)
+				defer done()
+			}
+
+			// Create driver impl.
+			drv, err := openBucket(ctx, test.bucketName, client, nil)
+			if (err != nil) != test.wantErr {
+				t.Errorf("got err %v want error %v", err, test.wantErr)
+			}
+			if drv != nil {
+				if drv.name != test.want {
+					t.Errorf("got %q want %q", drv.name, test.want)
+				}
+			}
+
+			// Create concrete type.
+			_, err = OpenBucket(ctx, test.bucketName, client, nil)
+			if (err != nil) != test.wantErr {
+				t.Errorf("got err %v want error %v", err, test.wantErr)
+			}
+		})
+	}
+}
 
+func TestOpenURL(t *testing.T) {
 	ctx := context.Background()
 
 	// Create a file for use as a dummy private key file.
@@ -288,7 +347,7 @@ func TestOpenURL(t *testing.T) {
 			}
 			gotB, ok := got.(*bucket)
 			if !ok {
-				t.Fatalf("got %T want *bucket", got)
+				t.Fatalf("got type %T want *bucket", got)
 			}
 			if gotB.name != test.wantName {
 				t.Errorf("got bucket name %q want %q", gotB.name, test.wantName)
diff --git a/blob/s3blob/s3blob_test.go b/blob/s3blob/s3blob_test.go
index ecb7db52..36e77e8d 100644
--- a/blob/s3blob/s3blob_test.go
+++ b/blob/s3blob/s3blob_test.go
@@ -19,10 +19,12 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"net/url"
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/aws/awserr"
+	"github.com/aws/aws-sdk-go/aws/client"
 	"github.com/aws/aws-sdk-go/aws/session"
 	"github.com/aws/aws-sdk-go/service/s3"
 	"github.com/aws/aws-sdk-go/service/s3/s3manager"
@@ -145,3 +147,100 @@ func (verifyContentLanguage) ListObjectCheck(o *blob.ListObject) error {
 	// Nothing to check.
 	return nil
 }
+
+func TestOpenBucket(t *testing.T) {
+	tests := []struct {
+		description string
+		bucketName  string
+		nilSession  bool
+		want        string
+		wantErr     bool
+	}{
+		{
+			description: "empty bucket name results in error",
+			wantErr:     true,
+		},
+		{
+			description: "nil sess results in error",
+			bucketName:  "foo",
+			nilSession:  true,
+			wantErr:     true,
+		},
+		{
+			description: "success",
+			bucketName:  "foo",
+			want:        "foo",
+		},
+	}
+
+	ctx := context.Background()
+	for _, test := range tests {
+		t.Run(test.description, func(t *testing.T) {
+			var sess client.ConfigProvider
+			if !test.nilSession {
+				var done func()
+				sess, _, done = setup.NewAWSSession(t, region)
+				defer done()
+			}
+
+			// Create driver impl.
+			drv, err := openBucket(ctx, test.bucketName, sess, nil)
+			if (err != nil) != test.wantErr {
+				t.Errorf("got err %v want error %v", err, test.wantErr)
+			}
+			if drv != nil {
+				if drv.name != test.want {
+					t.Errorf("got %q want %q", drv.name, test.want)
+				}
+			}
+
+			// Create concrete type.
+			_, err = OpenBucket(ctx, test.bucketName, sess, nil)
+			if (err != nil) != test.wantErr {
+				t.Errorf("got err %v want error %v", err, test.wantErr)
+			}
+		})
+	}
+}
+
+func TestOpenURL(t *testing.T) {
+	ctx := context.Background()
+
+	tests := []struct {
+		url      string
+		wantName string
+		wantErr  bool
+	}{
+		{
+			url:      "s3://mybucket?region=foo",
+			wantName: "mybucket",
+		},
+		{
+			url:      "s3://mybucket2?region=bar",
+			wantName: "mybucket2",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.url, func(t *testing.T) {
+			u, err := url.Parse(test.url)
+			if err != nil {
+				t.Fatal(err)
+			}
+			got, err := openURL(ctx, u)
+			if (err != nil) != test.wantErr {
+				t.Errorf("got err %v want error %v", err, test.wantErr)
+			}
+			if err != nil {
+				return
+			}
+			gotB, ok := got.(*bucket)
+			if !ok {
+				t.Fatalf("got type %T want *bucket", got)
+			}
+			if gotB.name != test.wantName {
+				t.Errorf("got bucket name %q want %q", gotB.name, test.wantName)
+			}
+		})
+	}
+}