From f978ec7b053c54c70a07ae4039b64197fa9af228 Mon Sep 17 00:00:00 2001 From: Dustin Long Date: Fri, 16 Apr 2021 16:17:18 -0400 Subject: [PATCH] fix(load): Fix spec test to exercise LoadDataset --- dsref/spec/load.go | 103 ++++++++++++++++++++++++++------------------- lib/load.go | 3 ++ lib/load_test.go | 28 ++++++++++-- 3 files changed, 87 insertions(+), 47 deletions(-) diff --git a/dsref/spec/load.go b/dsref/spec/load.go index f3d9226dd..8deb61215 100644 --- a/dsref/spec/load.go +++ b/dsref/spec/load.go @@ -13,74 +13,91 @@ import ( // PutDatasetFunc adds a dataset to a system that stores datasets // PutDatasetFunc is required to run the LoaderSpec test. When called the -// Loader should retain the dataset for later loading by the spec test -type PutDatasetFunc func(ds *dataset.Dataset) (path string, err error) +// Loader should retain the dataset for later loading by the spec test, and +// return a full reference to the saved version +type PutDatasetFunc func(ds *dataset.Dataset) (ref *dsref.Ref, err error) // AssertLoaderSpec confirms the expected behaviour of a dsref.Loader // Interface implementation. In addition to this test passing, implementations // MUST be nil-callable. Please add a nil-callable test for each implementation // -// TODO(b5) - loader spec is intentionally a little vague at the moment. I'm not -// sure the interface belongs in this package, and this test isn't working -// network sources. This test serves to confirm basic requirements of a local -// loader function for the moment +// TODO(b5) - this test isn't working network sources. At the moment it confirms +// only basic requirements of a local loader function func AssertLoaderSpec(t *testing.T, r dsref.Loader, putFunc PutDatasetFunc) { - t.Helper() - - var ( - ctx = context.Background() - username = "example_user" - name = "this_is_an_example" - path = "" - ) + ctx := context.Background() ds, err := GenerateExampleDataset(ctx) if err != nil { t.Fatal(err) } - path, err = putFunc(ds) + savedRef, err := putFunc(ds) if err != nil { t.Fatalf("putting dataset: %s", err) } - _, err = r.LoadDataset(ctx, "") - if err == nil { - t.Errorf("expected loading without a reference Path value to fail, got nil") - } + // update our expected dataset with values from the added ref + ds.Peername = savedRef.Username + ds.Name = savedRef.Name + ds.Path = savedRef.Path + // TODO(b5): loading should require initID to be set + // ds.ID = savedRef.InitID - ref := dsref.Ref{ - Username: username, - Name: name, - Path: path, - } - got, err := r.LoadDataset(ctx, ref.String()) - if err != nil { - t.Fatal(err) - } + t.Run("empty_ref", func(t *testing.T) { + if _, err = r.LoadDataset(ctx, ""); err == nil { + t.Errorf("expected loading an empty string to fail, did not get an error") + } + }) - if got.BodyFile() == nil { - t.Errorf("expected body file to be open & ready to read") - } + t.Run("full_reference_provided", func(t *testing.T) { + got, err := r.LoadDataset(ctx, savedRef.String()) + if err != nil { + t.Fatal(err) + } - if username != got.Peername { - t.Errorf("load Dataset didn't set dataset.Peername field to given reference. want: %q got: %q", username, got.Peername) - } - if name != got.Name { - t.Errorf("load Dataset didn't set dataset.Name field to given reference. want: %q got: %q", name, got.Name) - } + if got.BodyFile() == nil { + t.Errorf("expected body file to be open & ready to read") + } - ds.Peername = username - ds.Name = name - ds.Path = path - if diff := cmp.Diff(ds, got, cmpopts.IgnoreUnexported(dataset.Dataset{}, dataset.Meta{})); diff != "" { - t.Errorf("result mismatch (-want +got):\n%s", diff) - } + if savedRef.Username != got.Peername { + t.Errorf("load Dataset didn't set dataset.Peername field to given reference. want: %q got: %q", savedRef.Username, got.Peername) + } + if savedRef.Name != got.Name { + t.Errorf("load Dataset didn't set dataset.Name field to given reference. want: %q got: %q", savedRef.Name, got.Name) + } + + if diff := cmp.Diff(ds, got, cmpopts.IgnoreUnexported(dataset.Dataset{}, dataset.Meta{})); diff != "" { + t.Errorf("result mismatch (-want +got):\n%s", diff) + } + }) + + t.Run("alias_provided", func(t *testing.T) { + got, err := r.LoadDataset(ctx, savedRef.Alias()) + if err != nil { + t.Fatal(err) + } + + if got.BodyFile() == nil { + t.Errorf("expected body file to be open & ready to read") + } + + if savedRef.Username != got.Peername { + t.Errorf("load Dataset didn't set dataset.Peername field to given reference. want: %q got: %q", savedRef.Username, got.Peername) + } + if savedRef.Name != got.Name { + t.Errorf("load Dataset didn't set dataset.Name field to given reference. want: %q got: %q", savedRef.Name, got.Name) + } + + if diff := cmp.Diff(ds, got, cmpopts.IgnoreUnexported(dataset.Dataset{}, dataset.Meta{})); diff != "" { + t.Errorf("result mismatch (-want +got):\n%s", diff) + } + }) } // GenerateExampleDataset creates an example dataset document func GenerateExampleDataset(ctx context.Context) (*dataset.Dataset, error) { ds := &dataset.Dataset{ + Name: "example_loader_spec_test", Commit: &dataset.Commit{ Title: "initial commit", }, diff --git a/lib/load.go b/lib/load.go index 092a128e5..44483f3ae 100644 --- a/lib/load.go +++ b/lib/load.go @@ -75,6 +75,9 @@ func (d *datasetLoader) loadLocalDataset(ctx context.Context, ref dsref.Ref) (*d // Set transient info on the returned dataset ds.Name = ref.Name ds.Peername = ref.Username + // TODO(dustmop): When dscache / dscollect is in use, enable this since resolved + // references should always have it set + // ds.ID = ref.InitID if err = base.OpenDataset(ctx, d.inst.repo.Filesystem(), ds); err != nil { log.Debugf("Get dataset, base.OpenDataset failed, error: %s", err) diff --git a/lib/load_test.go b/lib/load_test.go index b22203a4a..0b6dc989a 100644 --- a/lib/load_test.go +++ b/lib/load_test.go @@ -5,13 +5,12 @@ import ( "github.com/qri-io/dataset" "github.com/qri-io/qri/base/dsfs" + "github.com/qri-io/qri/dsref" dsrefspec "github.com/qri-io/qri/dsref/spec" "github.com/qri-io/qri/event" ) func TestLoadDataset(t *testing.T) { - t.Skip("TODO(dustmop): Change in LoadDataset semantics breaks this test, figure out why") - tr := newTestRunner(t) defer tr.Delete() @@ -27,8 +26,19 @@ func TestLoadDataset(t *testing.T) { } loader = &datasetLoader{inst: tr.Instance} - dsrefspec.AssertLoaderSpec(t, loader, func(ds *dataset.Dataset) (string, error) { - return dsfs.CreateDataset( + dsrefspec.AssertLoaderSpec(t, loader, func(ds *dataset.Dataset) (*dsref.Ref, error) { + // Allocate an initID for this dataset + initID, err := tr.Instance.logbook.WriteDatasetInit(tr.Ctx, ds.Name) + if err != nil { + return nil, err + } + // Create the dataset in the provided storage + ref := &dsref.Ref{ + InitID: initID, + Username: tr.Instance.repo.Profiles().Owner().Peername, + Name: ds.Name, + } + path, err := dsfs.CreateDataset( tr.Ctx, fs, fs.DefaultWriteFS(), @@ -38,5 +48,15 @@ func TestLoadDataset(t *testing.T) { tr.Instance.repo.Profiles().Owner().PrivKey, dsfs.SaveSwitches{}, ) + if err != nil { + return nil, err + } + // Save the reference that the loader will use to laod + ref.Path = path + ds.Path = path + if err = tr.Instance.logbook.WriteVersionSave(tr.Ctx, initID, ds, nil); err != nil { + return nil, err + } + return ref, nil }) }