From c2b270e808340468fd66743a7caafa0371077665 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Stemmer?= Date: Sat, 8 Apr 2017 22:37:14 +0100 Subject: [PATCH 1/2] Always remove temp dir after test in vcs_source_test Some tests in vcs_source_test did not always delete the temp dir they created. The git-source tests for example only removed the temp dirs for certain failure conditions, but not when the test passed. This is fixed by deferring the cleanup function right after creating the temp dirs for each test. --- vcs_source_test.go | 41 ++++++++++++----------------------------- 1 file changed, 12 insertions(+), 29 deletions(-) diff --git a/vcs_source_test.go b/vcs_source_test.go index 76f03fca3e..9f9df79c40 100644 --- a/vcs_source_test.go +++ b/vcs_source_test.go @@ -38,19 +38,17 @@ func testGitSourceInteractions(t *testing.T) { if err != nil { t.Errorf("Failed to create temp dir: %s", err) } - rf := func() { - err := removeAll(cpath) - if err != nil { + defer func() { + if err := removeAll(cpath); err != nil { t.Errorf("removeAll failed: %s", err) } - } + }() n := "github.com/sdboyer/gpkt" un := "https://" + n u, err := url.Parse(un) if err != nil { t.Errorf("URL was bad, lolwut? errtext: %s", err) - rf() t.FailNow() } mb := maybeGitSource{ @@ -62,7 +60,6 @@ func testGitSourceInteractions(t *testing.T) { isrc, state, err := mb.try(ctx, cpath, newMemoryCache(), superv) if err != nil { t.Errorf("Unexpected error while setting up gitSource for test repo: %s", err) - rf() t.FailNow() } @@ -74,14 +71,12 @@ func testGitSourceInteractions(t *testing.T) { err = isrc.initLocal(ctx) if err != nil { t.Errorf("Error on cloning git repo: %s", err) - rf() t.FailNow() } src, ok := isrc.(*gitSource) if !ok { t.Errorf("Expected a gitSource, got a %T", isrc) - rf() t.FailNow() } @@ -92,7 +87,6 @@ func testGitSourceInteractions(t *testing.T) { pvlist, err := src.listVersions(ctx) if err != nil { t.Errorf("Unexpected error getting version pairs from git repo: %s", err) - rf() t.FailNow() } @@ -145,12 +139,11 @@ func testGopkginSourceInteractions(t *testing.T) { if err != nil { t.Errorf("Failed to create temp dir: %s", err) } - rf := func() { - err := removeAll(cpath) - if err != nil { + defer func() { + if err := removeAll(cpath); err != nil { t.Errorf("removeAll failed: %s", err) } - } + }() tfunc := func(opath, n string, major uint64, evl []Version) { un := "https://" + n @@ -181,7 +174,6 @@ func testGopkginSourceInteractions(t *testing.T) { err = isrc.initLocal(ctx) if err != nil { t.Errorf("Error on cloning git repo: %s", err) - rf() t.FailNow() } @@ -275,7 +267,6 @@ func testGopkginSourceInteractions(t *testing.T) { }() wg.Wait() - rf() } func testBzrSourceInteractions(t *testing.T) { @@ -291,19 +282,17 @@ func testBzrSourceInteractions(t *testing.T) { if err != nil { t.Errorf("Failed to create temp dir: %s", err) } - rf := func() { - err := removeAll(cpath) - if err != nil { + defer func() { + if err := removeAll(cpath); err != nil { t.Errorf("removeAll failed: %s", err) } - } + }() n := "launchpad.net/govcstestbzrrepo" un := "https://" + n u, err := url.Parse(un) if err != nil { t.Errorf("URL was bad, lolwut? errtext: %s", err) - rf() t.FailNow() } mb := maybeBzrSource{ @@ -315,7 +304,6 @@ func testBzrSourceInteractions(t *testing.T) { isrc, state, err := mb.try(ctx, cpath, newMemoryCache(), superv) if err != nil { t.Errorf("Unexpected error while setting up bzrSource for test repo: %s", err) - rf() t.FailNow() } @@ -327,14 +315,12 @@ func testBzrSourceInteractions(t *testing.T) { err = isrc.initLocal(ctx) if err != nil { t.Errorf("Error on cloning git repo: %s", err) - rf() t.FailNow() } src, ok := isrc.(*bzrSource) if !ok { t.Errorf("Expected a bzrSource, got a %T", isrc) - rf() t.FailNow() } @@ -410,12 +396,11 @@ func testHgSourceInteractions(t *testing.T) { if err != nil { t.Errorf("Failed to create temp dir: %s", err) } - rf := func() { - err := removeAll(cpath) - if err != nil { + defer func() { + if err := removeAll(cpath); err != nil { t.Errorf("removeAll failed: %s", err) } - } + }() tfunc := func(n string, evl []Version) { un := "https://" + n @@ -444,7 +429,6 @@ func testHgSourceInteractions(t *testing.T) { err = isrc.initLocal(ctx) if err != nil { t.Errorf("Error on cloning git repo: %s", err) - rf() t.FailNow() } @@ -530,7 +514,6 @@ func testHgSourceInteractions(t *testing.T) { }) <-donech - rf() } // Fail a test if the specified binaries aren't installed. From 1b6303f571c85830ab48428d52c98b383962abcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=ABl=20Stemmer?= Date: Sat, 8 Apr 2017 22:32:46 +0100 Subject: [PATCH 2/2] Replace Errorf followed by FailNow with Fatalf When running tests, calling t.Errorf followed by a t.FailNow is pretty much the same as just calling t.Fatalf. I've also change a couple of cute error messages. --- constraint_test.go | 6 ++---- hash_test.go | 15 +++++---------- manager_test.go | 18 ++++++------------ pkgtree/pkgtree_test.go | 15 +++++---------- result_test.go | 3 +-- rootdata_test.go | 6 ++---- vcs_source_test.go | 33 +++++++++++---------------------- version_queue_test.go | 3 +-- 8 files changed, 33 insertions(+), 66 deletions(-) diff --git a/constraint_test.go b/constraint_test.go index 16f54b9de9..f6d295c46a 100644 --- a/constraint_test.go +++ b/constraint_test.go @@ -590,8 +590,7 @@ func TestSemverConstraintOps(t *testing.T) { // still an incomparable type c1, err := NewSemverConstraint("=1.0.0") if err != nil { - t.Errorf("Failed to create constraint: %s", err) - t.FailNow() + t.Fatalf("Failed to create constraint: %s", err) } if !c1.MatchesAny(any) { @@ -610,8 +609,7 @@ func TestSemverConstraintOps(t *testing.T) { c1, err = NewSemverConstraint(">= 1.0.0") if err != nil { - t.Errorf("Failed to create constraint: %s", err) - t.FailNow() + t.Fatalf("Failed to create constraint: %s", err) } if c1.Matches(v1) { diff --git a/hash_test.go b/hash_test.go index 1721c33bbc..ad9466eb61 100644 --- a/hash_test.go +++ b/hash_test.go @@ -21,8 +21,7 @@ func TestHashInputs(t *testing.T) { s, err := Prepare(params, newdepspecSM(fix.ds, nil)) if err != nil { - t.Errorf("Unexpected error while prepping solver: %s", err) - t.FailNow() + t.Fatalf("Unexpected error while prepping solver: %s", err) } dig := s.HashInputs() @@ -73,8 +72,7 @@ func TestHashInputsReqsIgs(t *testing.T) { s, err := Prepare(params, newdepspecSM(fix.ds, nil)) if err != nil { - t.Errorf("Unexpected error while prepping solver: %s", err) - t.FailNow() + t.Fatalf("Unexpected error while prepping solver: %s", err) } dig := s.HashInputs() @@ -116,8 +114,7 @@ func TestHashInputsReqsIgs(t *testing.T) { s, err = Prepare(params, newdepspecSM(fix.ds, nil)) if err != nil { - t.Errorf("Unexpected error while prepping solver: %s", err) - t.FailNow() + t.Fatalf("Unexpected error while prepping solver: %s", err) } dig = s.HashInputs() @@ -157,8 +154,7 @@ func TestHashInputsReqsIgs(t *testing.T) { s, err = Prepare(params, newdepspecSM(fix.ds, nil)) if err != nil { - t.Errorf("Unexpected error while prepping solver: %s", err) - t.FailNow() + t.Fatalf("Unexpected error while prepping solver: %s", err) } dig = s.HashInputs() @@ -522,8 +518,7 @@ func TestHashInputsOverrides(t *testing.T) { s, err := Prepare(params, newdepspecSM(basefix.ds, nil)) if err != nil { - t.Errorf("(fix: %q) Unexpected error while prepping solver: %s", fix.name, err) - t.FailNow() + t.Fatalf("(fix: %q) Unexpected error while prepping solver: %s", fix.name, err) } h := sha256.New() diff --git a/manager_test.go b/manager_test.go index 10ad35721e..e30d3b3b68 100644 --- a/manager_test.go +++ b/manager_test.go @@ -43,14 +43,12 @@ func sv(s string) *semver.Version { func mkNaiveSM(t *testing.T) (*SourceMgr, func()) { cpath, err := ioutil.TempDir("", "smcache") if err != nil { - t.Errorf("Failed to create temp dir: %s", err) - t.FailNow() + t.Fatalf("Failed to create temp dir: %s", err) } sm, err := NewSourceManager(cpath) if err != nil { - t.Errorf("Unexpected error on SourceManager creation: %s", err) - t.FailNow() + t.Fatalf("Unexpected error on SourceManager creation: %s", err) } return sm, func() { @@ -68,8 +66,7 @@ func remakeNaiveSM(osm *SourceMgr, t *testing.T) (*SourceMgr, func()) { sm, err := NewSourceManager(cpath) if err != nil { - t.Errorf("unexpected error on SourceManager recreation: %s", err) - t.FailNow() + t.Fatalf("unexpected error on SourceManager recreation: %s", err) } return sm, func() { @@ -115,8 +112,7 @@ func TestSourceManagerInit(t *testing.T) { } if _, err = os.Stat(path.Join(cpath, "sm.lock")); !os.IsNotExist(err) { - t.Errorf("Global cache lock file not cleared correctly on Release()") - t.FailNow() + t.Fatalf("Global cache lock file not cleared correctly on Release()") } // Set another one up at the same spot now, just to be sure @@ -140,14 +136,12 @@ func TestSourceInit(t *testing.T) { cpath, err := ioutil.TempDir("", "smcache") if err != nil { - t.Errorf("Failed to create temp dir: %s", err) - t.FailNow() + t.Fatalf("Failed to create temp dir: %s", err) } sm, err := NewSourceManager(cpath) if err != nil { - t.Errorf("Unexpected error on SourceManager creation: %s", err) - t.FailNow() + t.Fatalf("Unexpected error on SourceManager creation: %s", err) } defer func() { diff --git a/pkgtree/pkgtree_test.go b/pkgtree/pkgtree_test.go index 2edaac8b8e..7196ed160a 100644 --- a/pkgtree/pkgtree_test.go +++ b/pkgtree/pkgtree_test.go @@ -1352,8 +1352,7 @@ func TestListPackagesNoPerms(t *testing.T) { } tmp, err := ioutil.TempDir("", "listpkgsnp") if err != nil { - t.Errorf("Failed to create temp dir: %s", err) - t.FailNow() + t.Fatalf("Failed to create temp dir: %s", err) } defer os.RemoveAll(tmp) @@ -1364,13 +1363,11 @@ func TestListPackagesNoPerms(t *testing.T) { // chmod the simple dir and m1p/b.go file so they can't be read err = os.Chmod(filepath.Join(workdir, "simple"), 0) if err != nil { - t.Error("Error while chmodding simple dir", err) - t.FailNow() + t.Fatalf("Error while chmodding simple dir: %s", err) } os.Chmod(filepath.Join(workdir, "m1p", "b.go"), 0) if err != nil { - t.Error("Error while chmodding b.go file", err) - t.FailNow() + t.Fatalf("Error while chmodding b.go file: %s", err) } want := PackageTree{ @@ -1398,12 +1395,10 @@ func TestListPackagesNoPerms(t *testing.T) { got, err := ListPackages(workdir, "ren") if err != nil { - t.Errorf("Unexpected err from ListPackages: %s", err) - t.FailNow() + t.Fatalf("Unexpected err from ListPackages: %s", err) } if want.ImportRoot != got.ImportRoot { - t.Errorf("Expected ImportRoot %s, got %s", want.ImportRoot, got.ImportRoot) - t.FailNow() + t.Fatalf("Expected ImportRoot %s, got %s", want.ImportRoot, got.ImportRoot) } if !reflect.DeepEqual(got, want) { diff --git a/result_test.go b/result_test.go index 8642ae2628..b5a59ec6bf 100644 --- a/result_test.go +++ b/result_test.go @@ -50,8 +50,7 @@ func testWriteDepTree(t *testing.T) { tmp, err := ioutil.TempDir("", "writetree") if err != nil { - t.Errorf("Failed to create temp dir: %s", err) - t.FailNow() + t.Fatalf("Failed to create temp dir: %s", err) } defer os.RemoveAll(tmp) diff --git a/rootdata_test.go b/rootdata_test.go index 970e14b384..15e7e7e634 100644 --- a/rootdata_test.go +++ b/rootdata_test.go @@ -17,8 +17,7 @@ func TestRootdataExternalImports(t *testing.T) { is, err := Prepare(params, newdepspecSM(fix.ds, nil)) if err != nil { - t.Errorf("Unexpected error while prepping solver: %s", err) - t.FailNow() + t.Fatalf("Unexpected error while prepping solver: %s", err) } rd := is.(*solver).rd @@ -71,8 +70,7 @@ func TestGetApplicableConstraints(t *testing.T) { is, err := Prepare(params, newdepspecSM(fix.ds, nil)) if err != nil { - t.Errorf("Unexpected error while prepping solver: %s", err) - t.FailNow() + t.Fatalf("Unexpected error while prepping solver: %s", err) } rd := is.(*solver).rd diff --git a/vcs_source_test.go b/vcs_source_test.go index 9f9df79c40..0794c1bc03 100644 --- a/vcs_source_test.go +++ b/vcs_source_test.go @@ -48,8 +48,7 @@ func testGitSourceInteractions(t *testing.T) { un := "https://" + n u, err := url.Parse(un) if err != nil { - t.Errorf("URL was bad, lolwut? errtext: %s", err) - t.FailNow() + t.Fatalf("Error parsing URL %s: %s", un, err) } mb := maybeGitSource{ url: u, @@ -59,8 +58,7 @@ func testGitSourceInteractions(t *testing.T) { superv := newSupervisor(ctx) isrc, state, err := mb.try(ctx, cpath, newMemoryCache(), superv) if err != nil { - t.Errorf("Unexpected error while setting up gitSource for test repo: %s", err) - t.FailNow() + t.Fatalf("Unexpected error while setting up gitSource for test repo: %s", err) } wantstate := sourceIsSetUp | sourceExistsUpstream | sourceHasLatestVersionList @@ -70,14 +68,12 @@ func testGitSourceInteractions(t *testing.T) { err = isrc.initLocal(ctx) if err != nil { - t.Errorf("Error on cloning git repo: %s", err) - t.FailNow() + t.Fatalf("Error on cloning git repo: %s", err) } src, ok := isrc.(*gitSource) if !ok { - t.Errorf("Expected a gitSource, got a %T", isrc) - t.FailNow() + t.Fatalf("Expected a gitSource, got a %T", isrc) } if un != src.upstreamURL() { @@ -86,8 +82,7 @@ func testGitSourceInteractions(t *testing.T) { pvlist, err := src.listVersions(ctx) if err != nil { - t.Errorf("Unexpected error getting version pairs from git repo: %s", err) - t.FailNow() + t.Fatalf("Unexpected error getting version pairs from git repo: %s", err) } vlist := hidePair(pvlist) @@ -173,8 +168,7 @@ func testGopkginSourceInteractions(t *testing.T) { err = isrc.initLocal(ctx) if err != nil { - t.Errorf("Error on cloning git repo: %s", err) - t.FailNow() + t.Fatalf("Error on cloning git repo: %s", err) } src, ok := isrc.(*gopkginSource) @@ -292,8 +286,7 @@ func testBzrSourceInteractions(t *testing.T) { un := "https://" + n u, err := url.Parse(un) if err != nil { - t.Errorf("URL was bad, lolwut? errtext: %s", err) - t.FailNow() + t.Fatalf("Error parsing URL %s: %s", un, err) } mb := maybeBzrSource{ url: u, @@ -303,8 +296,7 @@ func testBzrSourceInteractions(t *testing.T) { superv := newSupervisor(ctx) isrc, state, err := mb.try(ctx, cpath, newMemoryCache(), superv) if err != nil { - t.Errorf("Unexpected error while setting up bzrSource for test repo: %s", err) - t.FailNow() + t.Fatalf("Unexpected error while setting up bzrSource for test repo: %s", err) } wantstate := sourceIsSetUp | sourceExistsUpstream @@ -314,14 +306,12 @@ func testBzrSourceInteractions(t *testing.T) { err = isrc.initLocal(ctx) if err != nil { - t.Errorf("Error on cloning git repo: %s", err) - t.FailNow() + t.Fatalf("Error on cloning git repo: %s", err) } src, ok := isrc.(*bzrSource) if !ok { - t.Errorf("Expected a bzrSource, got a %T", isrc) - t.FailNow() + t.Fatalf("Expected a bzrSource, got a %T", isrc) } if state != wantstate { @@ -428,8 +418,7 @@ func testHgSourceInteractions(t *testing.T) { err = isrc.initLocal(ctx) if err != nil { - t.Errorf("Error on cloning git repo: %s", err) - t.FailNow() + t.Fatalf("Error on cloning git repo: %s", err) } src, ok := isrc.(*hgSource) diff --git a/version_queue_test.go b/version_queue_test.go index 2abc906ac8..337497c882 100644 --- a/version_queue_test.go +++ b/version_queue_test.go @@ -136,8 +136,7 @@ func TestVersionQueueAdvance(t *testing.T) { // First with no prefv or lockv vq, err := newVersionQueue(id, nil, nil, fb) if err != nil { - t.Errorf("Unexpected err on vq create: %s", err) - t.FailNow() + t.Fatalf("Unexpected err on vq create: %s", err) } for k, v := range fakevl[1:] {