From 3350760d85714bd94291e017ab49c9e5554e05c3 Mon Sep 17 00:00:00 2001 From: Li Zhijian Date: Wed, 9 Sep 2020 14:12:52 +0800 Subject: [PATCH 1/3] testing: Use T.cleanup() to cleanup tempdir Previously, after 'make test', some many temp dir have not been removed. Since this commit, temp dir created by ensure.HelmHome() ensure.TempDir() and ensure.TempFile() will be removed automatically in the end of testing - Use T.cleanup to remove the tempdir - Remove unnecessary defer Signed-off-by: Li Zhijian --- cmd/helm/create_test.go | 6 +++--- cmd/helm/dependency_update_test.go | 2 +- cmd/helm/repo_update_test.go | 2 +- cmd/helm/root_test.go | 2 +- internal/test/ensure/ensure.go | 13 ++++--------- pkg/action/package_test.go | 4 ---- pkg/chartutil/save_test.go | 1 - pkg/downloader/chart_downloader_test.go | 2 +- pkg/lint/rules/dependencies_test.go | 2 -- pkg/lint/rules/template_test.go | 2 -- pkg/lint/rules/values_test.go | 4 ---- pkg/plugin/installer/http_installer_test.go | 6 +++--- pkg/plugin/installer/vcs_installer_test.go | 6 +++--- pkg/repo/repotest/server_test.go | 4 ++-- 14 files changed, 19 insertions(+), 37 deletions(-) diff --git a/cmd/helm/create_test.go b/cmd/helm/create_test.go index 1db6bed52..a65036711 100644 --- a/cmd/helm/create_test.go +++ b/cmd/helm/create_test.go @@ -31,7 +31,7 @@ import ( ) func TestCreateCmd(t *testing.T) { - defer ensure.HelmHome(t)() + ensure.HelmHome(t) cname := "testchart" dir := ensure.TempDir(t) defer testChdir(t, dir)() @@ -62,7 +62,7 @@ func TestCreateCmd(t *testing.T) { } func TestCreateStarterCmd(t *testing.T) { - defer ensure.HelmHome(t)() + ensure.HelmHome(t) cname := "testchart" defer resetEnv()() os.MkdirAll(helmpath.CachePath(), 0755) @@ -128,7 +128,7 @@ func TestCreateStarterCmd(t *testing.T) { func TestCreateStarterAbsoluteCmd(t *testing.T) { defer resetEnv()() - defer ensure.HelmHome(t)() + ensure.HelmHome(t) cname := "testchart" // Create a starter. diff --git a/cmd/helm/dependency_update_test.go b/cmd/helm/dependency_update_test.go index 896018735..e11d48774 100644 --- a/cmd/helm/dependency_update_test.go +++ b/cmd/helm/dependency_update_test.go @@ -152,7 +152,7 @@ func TestDependencyUpdateCmd(t *testing.T) { func TestDependencyUpdateCmd_DontDeleteOldChartsOnError(t *testing.T) { defer resetEnv()() - defer ensure.HelmHome(t)() + ensure.HelmHome(t) srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz") if err != nil { diff --git a/cmd/helm/repo_update_test.go b/cmd/helm/repo_update_test.go index f4936b367..611557ccd 100644 --- a/cmd/helm/repo_update_test.go +++ b/cmd/helm/repo_update_test.go @@ -73,7 +73,7 @@ func TestUpdateCustomCacheCmd(t *testing.T) { func TestUpdateCharts(t *testing.T) { defer resetEnv()() - defer ensure.HelmHome(t)() + ensure.HelmHome(t) ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*") if err != nil { diff --git a/cmd/helm/root_test.go b/cmd/helm/root_test.go index 075544971..65e6d66c7 100644 --- a/cmd/helm/root_test.go +++ b/cmd/helm/root_test.go @@ -77,7 +77,7 @@ func TestRootCmd(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - defer ensure.HelmHome(t)() + ensure.HelmHome(t) for k, v := range tt.envvars { os.Setenv(k, v) diff --git a/internal/test/ensure/ensure.go b/internal/test/ensure/ensure.go index 3c0e4575c..f15098797 100644 --- a/internal/test/ensure/ensure.go +++ b/internal/test/ensure/ensure.go @@ -27,7 +27,7 @@ import ( ) // HelmHome sets up a Helm Home in a temp dir. -func HelmHome(t *testing.T) func() { +func HelmHome(t *testing.T) { t.Helper() base := TempDir(t) os.Setenv(xdg.CacheHomeEnvVar, base) @@ -36,9 +36,6 @@ func HelmHome(t *testing.T) func() { os.Setenv(helmpath.CacheHomeEnvVar, "") os.Setenv(helmpath.ConfigHomeEnvVar, "") os.Setenv(helmpath.DataHomeEnvVar, "") - return func() { - os.RemoveAll(base) - } } // TempDir ensures a scratch test directory for unit testing purposes. @@ -48,6 +45,8 @@ func TempDir(t *testing.T) string { if err != nil { t.Fatal(err) } + t.Cleanup(func() { os.RemoveAll(d) }) + return d } @@ -55,11 +54,7 @@ func TempDir(t *testing.T) string { // // It returns the path to the directory (to which you will still need to join the filename) // -// You must clean up the directory that is returned. -// -// tempdir := TempFile(t, "foo", []byte("bar")) -// defer os.RemoveAll(tempdir) -// filename := filepath.Join(tempdir, "foo") +// The whole TempDir will be Remove automatically func TempFile(t *testing.T, name string, data []byte) string { path := TempDir(t) filename := filepath.Join(path, name) diff --git a/pkg/action/package_test.go b/pkg/action/package_test.go index 5c5fed571..8e6533059 100644 --- a/pkg/action/package_test.go +++ b/pkg/action/package_test.go @@ -18,7 +18,6 @@ package action import ( "io/ioutil" - "os" "path" "testing" @@ -30,7 +29,6 @@ import ( func TestPassphraseFileFetcher(t *testing.T) { secret := "secret" directory := ensure.TempFile(t, "passphrase-file", []byte(secret)) - defer os.RemoveAll(directory) fetcher, err := passphraseFileFetcher(path.Join(directory, "passphrase-file"), nil) if err != nil { @@ -50,7 +48,6 @@ func TestPassphraseFileFetcher(t *testing.T) { func TestPassphraseFileFetcher_WithLineBreak(t *testing.T) { secret := "secret" directory := ensure.TempFile(t, "passphrase-file", []byte(secret+"\n\n.")) - defer os.RemoveAll(directory) fetcher, err := passphraseFileFetcher(path.Join(directory, "passphrase-file"), nil) if err != nil { @@ -69,7 +66,6 @@ func TestPassphraseFileFetcher_WithLineBreak(t *testing.T) { func TestPassphraseFileFetcher_WithInvalidStdin(t *testing.T) { directory := ensure.TempDir(t) - defer os.RemoveAll(directory) stdin, err := ioutil.TempFile(directory, "non-existing") if err != nil { diff --git a/pkg/chartutil/save_test.go b/pkg/chartutil/save_test.go index 3a45b2992..c92ff8dcf 100644 --- a/pkg/chartutil/save_test.go +++ b/pkg/chartutil/save_test.go @@ -37,7 +37,6 @@ import ( func TestSave(t *testing.T) { tmp := ensure.TempDir(t) - defer os.RemoveAll(tmp) for _, dest := range []string{tmp, path.Join(tmp, "newdir")} { t.Run("outDir="+dest, func(t *testing.T) { diff --git a/pkg/downloader/chart_downloader_test.go b/pkg/downloader/chart_downloader_test.go index 334d7aaa1..0ae016592 100644 --- a/pkg/downloader/chart_downloader_test.go +++ b/pkg/downloader/chart_downloader_test.go @@ -280,7 +280,7 @@ func TestDownloadTo_TLS(t *testing.T) { } func TestDownloadTo_VerifyLater(t *testing.T) { - defer ensure.HelmHome(t)() + ensure.HelmHome(t) dest := ensure.TempDir(t) defer os.RemoveAll(dest) diff --git a/pkg/lint/rules/dependencies_test.go b/pkg/lint/rules/dependencies_test.go index 075190eac..7d06d96bc 100644 --- a/pkg/lint/rules/dependencies_test.go +++ b/pkg/lint/rules/dependencies_test.go @@ -16,7 +16,6 @@ limitations under the License. package rules import ( - "os" "path/filepath" "testing" @@ -80,7 +79,6 @@ func TestValidateDependencyInMetadata(t *testing.T) { func TestDependencies(t *testing.T) { tmp := ensure.TempDir(t) - defer os.RemoveAll(tmp) c := chartWithBadDependencies() err := chartutil.SaveDir(&c, tmp) diff --git a/pkg/lint/rules/template_test.go b/pkg/lint/rules/template_test.go index eb076a1bf..aefc136b8 100644 --- a/pkg/lint/rules/template_test.go +++ b/pkg/lint/rules/template_test.go @@ -178,7 +178,6 @@ func TestDeprecatedAPIFails(t *testing.T) { }, } tmpdir := ensure.TempDir(t) - defer os.RemoveAll(tmpdir) if err := chartutil.SaveDir(&mychart, tmpdir); err != nil { t.Fatal(err) @@ -235,7 +234,6 @@ func TestStrictTemplateParsingMapError(t *testing.T) { }, } dir := ensure.TempDir(t) - defer os.RemoveAll(dir) if err := chartutil.SaveDir(&ch, dir); err != nil { t.Fatal(err) } diff --git a/pkg/lint/rules/values_test.go b/pkg/lint/rules/values_test.go index 6d93d630e..86ee6a4ee 100644 --- a/pkg/lint/rules/values_test.go +++ b/pkg/lint/rules/values_test.go @@ -67,7 +67,6 @@ func TestValidateValuesFileWellFormed(t *testing.T) { not:well[]{}formed ` tmpdir := ensure.TempFile(t, "values.yaml", []byte(badYaml)) - defer os.RemoveAll(tmpdir) valfile := filepath.Join(tmpdir, "values.yaml") if err := validateValuesFile(valfile, map[string]interface{}{}); err == nil { t.Fatal("expected values file to fail parsing") @@ -77,7 +76,6 @@ func TestValidateValuesFileWellFormed(t *testing.T) { func TestValidateValuesFileSchema(t *testing.T) { yaml := "username: admin\npassword: swordfish" tmpdir := ensure.TempFile(t, "values.yaml", []byte(yaml)) - defer os.RemoveAll(tmpdir) createTestingSchema(t, tmpdir) valfile := filepath.Join(tmpdir, "values.yaml") @@ -90,7 +88,6 @@ func TestValidateValuesFileSchemaFailure(t *testing.T) { // 1234 is an int, not a string. This should fail. yaml := "username: 1234\npassword: swordfish" tmpdir := ensure.TempFile(t, "values.yaml", []byte(yaml)) - defer os.RemoveAll(tmpdir) createTestingSchema(t, tmpdir) valfile := filepath.Join(tmpdir, "values.yaml") @@ -109,7 +106,6 @@ func TestValidateValuesFileSchemaOverrides(t *testing.T) { "password": "swordfish", } tmpdir := ensure.TempFile(t, "values.yaml", []byte(yaml)) - defer os.RemoveAll(tmpdir) createTestingSchema(t, tmpdir) valfile := filepath.Join(tmpdir, "values.yaml") diff --git a/pkg/plugin/installer/http_installer_test.go b/pkg/plugin/installer/http_installer_test.go index e89fea29d..812e4e8e6 100644 --- a/pkg/plugin/installer/http_installer_test.go +++ b/pkg/plugin/installer/http_installer_test.go @@ -80,7 +80,7 @@ func mockArchiveServer() *httptest.Server { } func TestHTTPInstaller(t *testing.T) { - defer ensure.HelmHome(t)() + ensure.HelmHome(t) srv := mockArchiveServer() defer srv.Close() @@ -129,7 +129,7 @@ func TestHTTPInstaller(t *testing.T) { } func TestHTTPInstallerNonExistentVersion(t *testing.T) { - defer ensure.HelmHome(t)() + ensure.HelmHome(t) srv := mockArchiveServer() defer srv.Close() source := srv.URL + "/plugins/fake-plugin-0.0.1.tar.gz" @@ -165,7 +165,7 @@ func TestHTTPInstallerUpdate(t *testing.T) { srv := mockArchiveServer() defer srv.Close() source := srv.URL + "/plugins/fake-plugin-0.0.1.tar.gz" - defer ensure.HelmHome(t)() + ensure.HelmHome(t) if err := os.MkdirAll(helmpath.DataPath("plugins"), 0755); err != nil { t.Fatalf("Could not create %s: %s", helmpath.DataPath("plugins"), err) diff --git a/pkg/plugin/installer/vcs_installer_test.go b/pkg/plugin/installer/vcs_installer_test.go index 6785264b3..0bb0b6780 100644 --- a/pkg/plugin/installer/vcs_installer_test.go +++ b/pkg/plugin/installer/vcs_installer_test.go @@ -49,7 +49,7 @@ func (r *testRepo) UpdateVersion(version string) error { } func TestVCSInstaller(t *testing.T) { - defer ensure.HelmHome(t)() + ensure.HelmHome(t) if err := os.MkdirAll(helmpath.DataPath("plugins"), 0755); err != nil { t.Fatalf("Could not create %s: %s", helmpath.DataPath("plugins"), err) @@ -102,7 +102,7 @@ func TestVCSInstaller(t *testing.T) { } func TestVCSInstallerNonExistentVersion(t *testing.T) { - defer ensure.HelmHome(t)() + ensure.HelmHome(t) source := "https://github.com/adamreese/helm-env" version := "0.2.0" @@ -124,7 +124,7 @@ func TestVCSInstallerNonExistentVersion(t *testing.T) { } } func TestVCSInstallerUpdate(t *testing.T) { - defer ensure.HelmHome(t)() + ensure.HelmHome(t) source := "https://github.com/adamreese/helm-env" diff --git a/pkg/repo/repotest/server_test.go b/pkg/repo/repotest/server_test.go index 1ad979fdc..2849d7da6 100644 --- a/pkg/repo/repotest/server_test.go +++ b/pkg/repo/repotest/server_test.go @@ -31,7 +31,7 @@ import ( // Young'n, in these here parts, we test our tests. func TestServer(t *testing.T) { - defer ensure.HelmHome(t)() + ensure.HelmHome(t) rootDir := ensure.TempDir(t) defer os.RemoveAll(rootDir) @@ -99,7 +99,7 @@ func TestServer(t *testing.T) { } func TestNewTempServer(t *testing.T) { - defer ensure.HelmHome(t)() + ensure.HelmHome(t) srv, err := NewTempServerWithCleanup(t, "testdata/examplechart-0.1.0.tgz") if err != nil { From 6230cb94a920676232ad56490a89b32c422720bb Mon Sep 17 00:00:00 2001 From: Li Zhijian Date: Wed, 14 Oct 2020 17:10:46 +0800 Subject: [PATCH 2/3] testing: Don't cleanup tempfiles if the test failed As suggestion by Paul Brousseau, It might be useful to leave the directory in place if the test fails. Signed-off-by: Li Zhijian --- internal/test/ensure/ensure.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/test/ensure/ensure.go b/internal/test/ensure/ensure.go index f15098797..569024467 100644 --- a/internal/test/ensure/ensure.go +++ b/internal/test/ensure/ensure.go @@ -45,7 +45,11 @@ func TempDir(t *testing.T) string { if err != nil { t.Fatal(err) } - t.Cleanup(func() { os.RemoveAll(d) }) + t.Cleanup(func() { + if !t.Failed() { + os.RemoveAll(d) + } + }) return d } From 673488743139d732b90efc58cef57ee7217db5d4 Mon Sep 17 00:00:00 2001 From: Li Zhijian Date: Mon, 28 Sep 2020 14:27:25 +0800 Subject: [PATCH 3/3] testing: setup a temp HelmHome for a few tests these tests will dirty the HelmHome Signed-off-by: Li Zhijian --- cmd/helm/dependency_build_test.go | 2 ++ cmd/helm/dependency_update_test.go | 1 + cmd/helm/pull_test.go | 2 ++ cmd/helm/repo_add_test.go | 1 + cmd/helm/repo_remove_test.go | 1 + pkg/downloader/manager_test.go | 3 +++ pkg/plugin/installer/local_installer_test.go | 2 ++ 7 files changed, 12 insertions(+) diff --git a/cmd/helm/dependency_build_test.go b/cmd/helm/dependency_build_test.go index 33198a9dd..4d66ac1b6 100644 --- a/cmd/helm/dependency_build_test.go +++ b/cmd/helm/dependency_build_test.go @@ -22,6 +22,7 @@ import ( "strings" "testing" + "helm.sh/helm/v3/internal/test/ensure" "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/provenance" "helm.sh/helm/v3/pkg/repo" @@ -29,6 +30,7 @@ import ( ) func TestDependencyBuildCmd(t *testing.T) { + ensure.HelmHome(t) srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz") defer srv.Stop() if err != nil { diff --git a/cmd/helm/dependency_update_test.go b/cmd/helm/dependency_update_test.go index e11d48774..f7c559e3f 100644 --- a/cmd/helm/dependency_update_test.go +++ b/cmd/helm/dependency_update_test.go @@ -33,6 +33,7 @@ import ( ) func TestDependencyUpdateCmd(t *testing.T) { + ensure.HelmHome(t) srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz") if err != nil { t.Fatal(err) diff --git a/cmd/helm/pull_test.go b/cmd/helm/pull_test.go index 51cdfdfa4..98eccad04 100644 --- a/cmd/helm/pull_test.go +++ b/cmd/helm/pull_test.go @@ -22,10 +22,12 @@ import ( "path/filepath" "testing" + "helm.sh/helm/v3/internal/test/ensure" "helm.sh/helm/v3/pkg/repo/repotest" ) func TestPullCmd(t *testing.T) { + ensure.HelmHome(t) srv, err := repotest.NewTempServerWithCleanup(t, "testdata/testcharts/*.tgz*") if err != nil { t.Fatal(err) diff --git a/cmd/helm/repo_add_test.go b/cmd/helm/repo_add_test.go index f3bc54985..a14badd3c 100644 --- a/cmd/helm/repo_add_test.go +++ b/cmd/helm/repo_add_test.go @@ -143,6 +143,7 @@ func TestRepoAddConcurrentDirNotExist(t *testing.T) { } func repoAddConcurrent(t *testing.T, testName, repoFile string) { + ensure.HelmHome(t) ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*") if err != nil { t.Fatal(err) diff --git a/cmd/helm/repo_remove_test.go b/cmd/helm/repo_remove_test.go index cb5c6e9ab..d93547188 100644 --- a/cmd/helm/repo_remove_test.go +++ b/cmd/helm/repo_remove_test.go @@ -30,6 +30,7 @@ import ( ) func TestRepoRemove(t *testing.T) { + ensure.HelmHome(t) ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*") if err != nil { t.Fatal(err) diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index fc8d9abb2..346dbbd25 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -21,6 +21,7 @@ import ( "reflect" "testing" + "helm.sh/helm/v3/internal/test/ensure" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/getter" @@ -182,6 +183,7 @@ func TestGetRepoNames(t *testing.T) { } func TestUpdateBeforeBuild(t *testing.T) { + ensure.HelmHome(t) // Set up a fake repo srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*") if err != nil { @@ -326,6 +328,7 @@ func TestUpdateWithNoRepo(t *testing.T) { // Parent chart includes local-subchart 0.1.0 subchart from a fake repository, by default. // If each of these main fields (name, version, repository) is not supplied by dep param, default value will be used. func checkBuildWithOptionalFields(t *testing.T, chartName string, dep chart.Dependency) { + ensure.HelmHome(t) // Set up a fake repo srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*") if err != nil { diff --git a/pkg/plugin/installer/local_installer_test.go b/pkg/plugin/installer/local_installer_test.go index 0d3de11d1..75c7fc597 100644 --- a/pkg/plugin/installer/local_installer_test.go +++ b/pkg/plugin/installer/local_installer_test.go @@ -21,12 +21,14 @@ import ( "path/filepath" "testing" + "helm.sh/helm/v3/internal/test/ensure" "helm.sh/helm/v3/pkg/helmpath" ) var _ Installer = new(LocalInstaller) func TestLocalInstaller(t *testing.T) { + ensure.HelmHome(t) // Make a temp dir tdir, err := ioutil.TempDir("", "helm-installer-") if err != nil {