From 94b6497533190daab27c70c2d2089f0f3486d4b2 Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Wed, 10 Oct 2018 12:17:11 -0400 Subject: [PATCH 1/5] more integration test coverage --- cloud/bq/sanity_integration_test.go | 94 ++++++++++++++++++++++++++++- 1 file changed, 93 insertions(+), 1 deletion(-) diff --git a/cloud/bq/sanity_integration_test.go b/cloud/bq/sanity_integration_test.go index fadc6626..7bacb573 100644 --- a/cloud/bq/sanity_integration_test.go +++ b/cloud/bq/sanity_integration_test.go @@ -94,6 +94,28 @@ func TestAnnotationDetail(t *testing.T) { t.Fatal(err) } + tbl := dsExt.Table("DedupTest") + + at := bq.NewAnnotatedTable(tbl, &dsExt) + // Fetch cache detail - which hits backend + _, err = at.CachedDetail(ctx) + if err != nil { + t.Error(err) + } + // Fetch again, exercising the cached code path. + _, err = at.CachedDetail(ctx) + if err != nil { + t.Error(err) + } +} + +func TestAnnotationMeta(t *testing.T) { + ctx := context.Background() + dsExt, err := dataset.NewDataset(ctx, "mlab-testing", "src") + if err != nil { + t.Fatal(err) + } + tbl := dsExt.Table("DedupTest") meta, err := tbl.Metadata(ctx) if err != nil { @@ -101,11 +123,61 @@ func TestAnnotationDetail(t *testing.T) { } else if meta == nil { t.Error("Meta should not be nil") } + at := bq.NewAnnotatedTable(tbl, &dsExt) - _, err = at.CachedDetail(ctx) + // Fetch cache detail - which hits backend + meta, err = at.CachedMeta(ctx) + if err != nil { + t.Error(err) + } else if meta == nil { + t.Error("Meta should not be nil") + } + // Fetch again, exercising the cached code path. + meta, err = at.CachedMeta(ctx) + if err != nil { + t.Error(err) + } else if meta == nil { + t.Error("Meta should not be nil") + } + +} + +func TestAnnotationPartitionInfo(t *testing.T) { + ctx := context.Background() + dsExt, err := dataset.NewDataset(ctx, "mlab-testing", "src") + if err != nil { + t.Fatal(err) + } + + tbl := dsExt.Table("DedupTest") + + at := bq.NewAnnotatedTable(tbl, &dsExt) + // Fetch cache detail - which hits backend + + _, err = at.CachedPartitionInfo(ctx) + if err != nil { + t.Error(err) + } + // Fetch again, exercising the cached code path. + _, err = at.CachedPartitionInfo(ctx) if err != nil { t.Error(err) } + + badTable := dsExt.Table("non-existant") + + badAT := bq.NewAnnotatedTable(tbl, &dsExt) + // Fetch cache detail - which hits backend + + _, err = badAT.CachedPartitionInfo(ctx) + if err == nil { + t.Error("Should return error") + } + // Fetch again, exercising the already errored code path. + _, err = badAT.CachedPartitionInfo(ctx) + if err == nil { + t.Error("Should return error") + } } func TestGetTablesMatching(t *testing.T) { @@ -149,3 +221,23 @@ func TestAnnotatedTableGetPartitionInfo(t *testing.T) { t.Error("Non-existent partition should return empty PartitionID") } } + +func TestAnnotatedTable(t *testing.T) { + ctx := context.Background() + ds, err := dataset.NewDataset(ctx, "mlab-testing", "go") + if err != nil { + t.Fatal(err) + } + src := ds.Table("TestGetTableStats") + srcAt := bq.NewAnnotatedTable(src, &ds) + // Fetch detail. + _, err = srcAt.CachedDetail(ctx) + if err != nil { + t.Fatal(err) + } + // Fetch again to exercise cached code path. + _, err = srcAt.CachedDetail(ctx) + if err != nil { + t.Fatal(err) + } +} From 3c38a5da72756579c296c6b9de14eb50c2b1306d Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Wed, 10 Oct 2018 12:23:45 -0400 Subject: [PATCH 2/5] better GetPartitionInfo coverage --- cloud/bq/sanity_integration_test.go | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/cloud/bq/sanity_integration_test.go b/cloud/bq/sanity_integration_test.go index 7bacb573..8f1e0520 100644 --- a/cloud/bq/sanity_integration_test.go +++ b/cloud/bq/sanity_integration_test.go @@ -166,7 +166,7 @@ func TestAnnotationPartitionInfo(t *testing.T) { badTable := dsExt.Table("non-existant") - badAT := bq.NewAnnotatedTable(tbl, &dsExt) + badAT := bq.NewAnnotatedTable(badTable, &dsExt) // Fetch cache detail - which hits backend _, err = badAT.CachedPartitionInfo(ctx) @@ -204,12 +204,32 @@ func TestAnnotatedTableGetPartitionInfo(t *testing.T) { tbl := dsExt.Table("DedupTest$19990101") at := bq.NewAnnotatedTable(tbl, &dsExt) + // Test nil context. + _, err = at.GetPartitionInfo(nil) + if err == nil { + t.Error("Should error on nil ctx") + } + + // Test initial fetch info, err := at.GetPartitionInfo(ctx) if err != nil { t.Error(err) } else if info.PartitionID != "19990101" { t.Error("wrong partitionID: " + info.PartitionID) } + // Check cached code path + info, err = at.GetPartitionInfo(ctx) + if err != nil { + t.Error(err) + } else if info.PartitionID != "19990101" { + t.Error("wrong partitionID: " + info.PartitionID) + } + + // Once cache is populated, also ok to send a nil ctx. + _, err = at.GetPartitionInfo(nil) + if err != nil { + t.Error(err) + } // Check behavior for missing partition tbl = dsExt.Table("DedupTest$17760101") @@ -220,6 +240,7 @@ func TestAnnotatedTableGetPartitionInfo(t *testing.T) { } else if info.PartitionID != "" { t.Error("Non-existent partition should return empty PartitionID") } + } func TestAnnotatedTable(t *testing.T) { From 269917ff3e806f6badc0ac3dbad44ba0e72c9b78 Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Wed, 10 Oct 2018 12:47:15 -0400 Subject: [PATCH 3/5] GetPartitionInfo doesnt cache --- cloud/bq/sanity_integration_test.go | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/cloud/bq/sanity_integration_test.go b/cloud/bq/sanity_integration_test.go index 8f1e0520..de1e319f 100644 --- a/cloud/bq/sanity_integration_test.go +++ b/cloud/bq/sanity_integration_test.go @@ -204,32 +204,12 @@ func TestAnnotatedTableGetPartitionInfo(t *testing.T) { tbl := dsExt.Table("DedupTest$19990101") at := bq.NewAnnotatedTable(tbl, &dsExt) - // Test nil context. - _, err = at.GetPartitionInfo(nil) - if err == nil { - t.Error("Should error on nil ctx") - } - - // Test initial fetch info, err := at.GetPartitionInfo(ctx) if err != nil { t.Error(err) } else if info.PartitionID != "19990101" { t.Error("wrong partitionID: " + info.PartitionID) } - // Check cached code path - info, err = at.GetPartitionInfo(ctx) - if err != nil { - t.Error(err) - } else if info.PartitionID != "19990101" { - t.Error("wrong partitionID: " + info.PartitionID) - } - - // Once cache is populated, also ok to send a nil ctx. - _, err = at.GetPartitionInfo(nil) - if err != nil { - t.Error(err) - } // Check behavior for missing partition tbl = dsExt.Table("DedupTest$17760101") From 5bea1f444983d79c25531e646d117d1372e8068f Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Wed, 10 Oct 2018 14:59:59 -0400 Subject: [PATCH 4/5] tweak error handling and tests --- cloud/bq/sanity.go | 14 ++++++++++++-- cloud/bq/sanity_integration_test.go | 6 +++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/cloud/bq/sanity.go b/cloud/bq/sanity.go index 5be389ed..f96c9a0c 100644 --- a/cloud/bq/sanity.go +++ b/cloud/bq/sanity.go @@ -110,6 +110,9 @@ func (at *AnnotatedTable) CachedDetail(ctx context.Context) (*Detail, error) { // TODO - use context // TODO - maybe just embed code here. at.detail, at.err = GetTableDetail(ctx, at.dataset, at.Table) + if at.err != nil { + log.Println(at.FullyQualifiedName(), at.TableID()) + } return at.detail, at.err } @@ -178,6 +181,10 @@ func GetTableDetail(ctx context.Context, dsExt *dataset.Dataset, table bqiface.T // TODO - this should take a context? err := dsExt.QueryAndParse(ctx, queryString, &detail) + if err != nil { + log.Println(err) + log.Println("Query error:", queryString) + } return &detail, err } @@ -239,8 +246,11 @@ func getTableParts(tableName string) (tableNameParts, error) { func (at *AnnotatedTable) GetPartitionInfo(ctx context.Context) (*dataset.PartitionInfo, error) { tableName := at.Table.TableID() parts, err := getTableParts(tableName) - if err != nil || !parts.isPartitioned { - return nil, errors.New("TableID missing partition: " + tableName) + if err != nil { + return nil, err + } + if !parts.isPartitioned { + return nil, errors.New("TableID does not specify partition: " + tableName) } // Assemble the FQ table name, without the partition suffix. fullTable := fmt.Sprintf("%s:%s.%s", at.ProjectID(), at.DatasetID(), parts.prefix) diff --git a/cloud/bq/sanity_integration_test.go b/cloud/bq/sanity_integration_test.go index de1e319f..5c86e74e 100644 --- a/cloud/bq/sanity_integration_test.go +++ b/cloud/bq/sanity_integration_test.go @@ -149,7 +149,7 @@ func TestAnnotationPartitionInfo(t *testing.T) { t.Fatal(err) } - tbl := dsExt.Table("DedupTest") + tbl := dsExt.Table("DedupTest$19990101") at := bq.NewAnnotatedTable(tbl, &dsExt) // Fetch cache detail - which hits backend @@ -225,11 +225,11 @@ func TestAnnotatedTableGetPartitionInfo(t *testing.T) { func TestAnnotatedTable(t *testing.T) { ctx := context.Background() - ds, err := dataset.NewDataset(ctx, "mlab-testing", "go") + ds, err := dataset.NewDataset(ctx, "mlab-testing", "src") if err != nil { t.Fatal(err) } - src := ds.Table("TestGetTableStats") + src := ds.Table("DedupTest") srcAt := bq.NewAnnotatedTable(src, &ds) // Fetch detail. _, err = srcAt.CachedDetail(ctx) From c73f70f67bd66cdb786ea751bc1f68df33a305b4 Mon Sep 17 00:00:00 2001 From: Greg Russell Date: Wed, 10 Oct 2018 19:08:52 -0400 Subject: [PATCH 5/5] cleanup --- cloud/bq/sanity_integration_test.go | 61 ++----------------------- cloud/bq/sanity_test.go | 69 +++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 58 deletions(-) diff --git a/cloud/bq/sanity_integration_test.go b/cloud/bq/sanity_integration_test.go index 5c86e74e..02109b5f 100644 --- a/cloud/bq/sanity_integration_test.go +++ b/cloud/bq/sanity_integration_test.go @@ -55,7 +55,7 @@ func TestGetTableDetail(t *testing.T) { } } -func TestAnnotationTableMeta(t *testing.T) { +func TestCachedMeta(t *testing.T) { // TODO - Make NewDataSet return a pointer, for consistency with bigquery. ctx := context.Background() dsExt, err := dataset.NewDataset(ctx, "mlab-testing", "src") @@ -87,62 +87,7 @@ func TestAnnotationTableMeta(t *testing.T) { } } -func TestAnnotationDetail(t *testing.T) { - ctx := context.Background() - dsExt, err := dataset.NewDataset(ctx, "mlab-testing", "src") - if err != nil { - t.Fatal(err) - } - - tbl := dsExt.Table("DedupTest") - - at := bq.NewAnnotatedTable(tbl, &dsExt) - // Fetch cache detail - which hits backend - _, err = at.CachedDetail(ctx) - if err != nil { - t.Error(err) - } - // Fetch again, exercising the cached code path. - _, err = at.CachedDetail(ctx) - if err != nil { - t.Error(err) - } -} - -func TestAnnotationMeta(t *testing.T) { - ctx := context.Background() - dsExt, err := dataset.NewDataset(ctx, "mlab-testing", "src") - if err != nil { - t.Fatal(err) - } - - tbl := dsExt.Table("DedupTest") - meta, err := tbl.Metadata(ctx) - if err != nil { - t.Error(err) - } else if meta == nil { - t.Error("Meta should not be nil") - } - - at := bq.NewAnnotatedTable(tbl, &dsExt) - // Fetch cache detail - which hits backend - meta, err = at.CachedMeta(ctx) - if err != nil { - t.Error(err) - } else if meta == nil { - t.Error("Meta should not be nil") - } - // Fetch again, exercising the cached code path. - meta, err = at.CachedMeta(ctx) - if err != nil { - t.Error(err) - } else if meta == nil { - t.Error("Meta should not be nil") - } - -} - -func TestAnnotationPartitionInfo(t *testing.T) { +func TestCachedPartitionInfo(t *testing.T) { ctx := context.Background() dsExt, err := dataset.NewDataset(ctx, "mlab-testing", "src") if err != nil { @@ -223,7 +168,7 @@ func TestAnnotatedTableGetPartitionInfo(t *testing.T) { } -func TestAnnotatedTable(t *testing.T) { +func TestCachedDetail(t *testing.T) { ctx := context.Background() ds, err := dataset.NewDataset(ctx, "mlab-testing", "src") if err != nil { diff --git a/cloud/bq/sanity_test.go b/cloud/bq/sanity_test.go index ba48ddb7..1c86e3dc 100644 --- a/cloud/bq/sanity_test.go +++ b/cloud/bq/sanity_test.go @@ -5,8 +5,13 @@ import ( "log" "strings" "testing" + "time" + "cloud.google.com/go/bigquery" + "github.com/GoogleCloudPlatform/google-cloud-go-testing/bigquery/bqiface" + "github.com/m-lab/etl-gardener/cloud" "github.com/m-lab/go/dataset" + "google.golang.org/api/option" ) func init() { @@ -72,3 +77,67 @@ func TestSanityCheckAndCopy(t *testing.T) { t.Fatal(err) } } + +// This defines a Dataset that returns a Table, that returns a canned Metadata. +type testTable struct { + bqiface.Table +} + +func (tbl testTable) Metadata(ctx context.Context) (*bigquery.TableMetadata, error) { + meta := bigquery.TableMetadata{CreationTime: time.Now(), LastModifiedTime: time.Now(), NumBytes: 168, NumRows: 8} + meta.TimePartitioning = &bigquery.TimePartitioning{Expiration: 0 * time.Second} + return &meta, nil +} + +type testDataset struct { + bqiface.Dataset +} + +func (ds *testDataset) Table(name string) bqiface.Table { + tt := testTable{ds.Dataset.Table(name)} + return tt +} + +// creates a Dataset with a dry run client. +func newTestDataset(project, ds string) dataset.Dataset { + ctx := context.Background() + dryRun, _ := cloud.DryRunClient() + c, err := bigquery.NewClient(ctx, project, option.WithHTTPClient(dryRun)) + if err != nil { + panic(err) + } + + bqClient := bqiface.AdaptClient(c) + + return dataset.Dataset{Dataset: &testDataset{bqClient.Dataset(ds)}, BqClient: bqClient} +} + +func TestCachedMeta(t *testing.T) { + ctx := context.Background() + dsExt := newTestDataset("mlab-testing", "etl") + + tbl := dsExt.Table("DedupTest") + meta, err := tbl.Metadata(ctx) + if err != nil { + t.Error(err) + } else if meta == nil { + t.Error("Meta should not be nil") + } + + at := NewAnnotatedTable(tbl, &dsExt) + // Fetch cache detail - which hits backend + meta, err = at.CachedMeta(ctx) + if err != nil { + t.Error(err) + } else if meta == nil { + t.Error("Meta should not be nil") + } + // Fetch again, exercising the cached code path. + meta, err = at.CachedMeta(ctx) + if err != nil { + t.Error(err) + } else if meta == nil { + t.Error("Meta should not be nil") + } + +}