From 1418e7f7bf06eb642f153fc72ed5ee433e5ecb16 Mon Sep 17 00:00:00 2001 From: rabadin Date: Fri, 6 Sep 2024 10:02:03 +0200 Subject: [PATCH] Add more unit-tests This branch's aim is to improve the unit tests to get to > 80% test coverage. The only non-test / mechanical change is in the download logic: this was changed to print all the errors encountered when failing to download multiple assets as opposed to just the first one. This is in `internal/lock.go`. --- Makefile | 3 +- cmd/add.go | 48 ++++++---- cmd/add_test.go | 25 +++++ cmd/delete.go | 32 ++++--- cmd/delete_test.go | 21 +++++ cmd/dowload_test.go | 118 ++++++++++++++++++++++++ cmd/download.go | 48 ++++++---- cmd/root.go | 39 ++++---- cmd/root_test.go | 16 ++++ cmd/version.go | 15 ++- internal/lock.go | 19 ++-- internal/lock_test.go | 15 +-- internal/resource.go | 9 ++ internal/resource_test.go | 3 +- internal/sri_test.go | 5 +- main.go | 3 +- internal/utils_test.go => test/utils.go | 8 +- 17 files changed, 331 insertions(+), 96 deletions(-) create mode 100644 cmd/add_test.go create mode 100644 cmd/delete_test.go create mode 100644 cmd/dowload_test.go create mode 100644 cmd/root_test.go rename internal/utils_test.go => test/utils.go (83%) diff --git a/Makefile b/Makefile index a194de8..e50a015 100644 --- a/Makefile +++ b/Makefile @@ -30,7 +30,8 @@ build: dep @go build -o grabit test: dep - @go test -race -coverprofile=coverage.out -v ./... + @go test -race -v -coverpkg=./... -coverprofile=coverage.out ./... + @go tool cover -func coverage.out | tail -n 1 check: dep @golangci-lint run --sort-results ./... diff --git a/cmd/add.go b/cmd/add.go index 8765aaa..28c3444 100644 --- a/cmd/add.go +++ b/cmd/add.go @@ -8,33 +8,47 @@ import ( "github.com/spf13/cobra" ) -func init() { - rootCmd.AddCommand(addCmd) +func addAdd(cmd *cobra.Command) { + addCmd := &cobra.Command{ + Use: "add", + Short: "Add new resource", + Args: cobra.MinimumNArgs(1), + RunE: runAdd, + } addCmd.Flags().String("algo", internal.RecommendedAlgo, "Integrity algorithm") addCmd.Flags().String("filename", "", "Target file name to use when downloading the resource") addCmd.Flags().StringArray("tag", []string{}, "Resource tags") + cmd.AddCommand(addCmd) } -var addCmd = &cobra.Command{ - Use: "add", - Short: "Add new resource", - Args: cobra.MinimumNArgs(1), - Run: runAdd, -} - -func runAdd(cmd *cobra.Command, args []string) { +func runAdd(cmd *cobra.Command, args []string) error { lockFile, err := cmd.Flags().GetString("lock-file") - FatalIfNotNil(err) + if err != nil { + return err + } lock, err := internal.NewLock(lockFile, true) - FatalIfNotNil(err) + if err != nil { + return err + } algo, err := cmd.Flags().GetString("algo") - FatalIfNotNil(err) + if err != nil { + return err + } tags, err := cmd.Flags().GetStringArray("tag") - FatalIfNotNil(err) + if err != nil { + return err + } filename, err := cmd.Flags().GetString("filename") - FatalIfNotNil(err) + if err != nil { + return err + } err = lock.AddResource(args, algo, tags, filename) - FatalIfNotNil(err) + if err != nil { + return err + } err = lock.Save() - FatalIfNotNil(err) + if err != nil { + return err + } + return nil } diff --git a/cmd/add_test.go b/cmd/add_test.go new file mode 100644 index 0000000..a7ba460 --- /dev/null +++ b/cmd/add_test.go @@ -0,0 +1,25 @@ +package cmd + +import ( + "fmt" + "net/http" + "testing" + + "github.com/cisco-open/grabit/test" + "github.com/stretchr/testify/assert" +) + +func TestRunAdd(t *testing.T) { + handler := func(w http.ResponseWriter, r *http.Request) { + _, err := w.Write([]byte(`abcdef`)) + if err != nil { + t.Fatal(err) + } + } + port, server := test.HttpHandler(handler) + defer server.Close() + cmd := NewRootCmd() + cmd.SetArgs([]string{"-f", test.TmpFile(t, ""), "add", fmt.Sprintf("http://localhost:%d/test.html", port)}) + err := cmd.Execute() + assert.Nil(t, err) +} diff --git a/cmd/delete.go b/cmd/delete.go index 43f263f..27c5722 100644 --- a/cmd/delete.go +++ b/cmd/delete.go @@ -8,25 +8,31 @@ import ( "github.com/spf13/cobra" ) -func init() { - rootCmd.AddCommand(delCmd) -} - -var delCmd = &cobra.Command{ - Use: "delete", - Short: "Delete existing resources", - Args: cobra.MinimumNArgs(1), - Run: runDel, +func addDelete(cmd *cobra.Command) { + var delCmd = &cobra.Command{ + Use: "delete", + Short: "Delete existing resources", + Args: cobra.MinimumNArgs(1), + RunE: runDel, + } + cmd.AddCommand(delCmd) } -func runDel(cmd *cobra.Command, args []string) { +func runDel(cmd *cobra.Command, args []string) error { lockFile, err := cmd.Flags().GetString("lock-file") - FatalIfNotNil(err) + if err != nil { + return err + } lock, err := internal.NewLock(lockFile, false) - FatalIfNotNil(err) + if err != nil { + return err + } for _, r := range args { lock.DeleteResource(r) } err = lock.Save() - FatalIfNotNil(err) + if err != nil { + return err + } + return nil } diff --git a/cmd/delete_test.go b/cmd/delete_test.go new file mode 100644 index 0000000..7f312be --- /dev/null +++ b/cmd/delete_test.go @@ -0,0 +1,21 @@ +package cmd + +import ( + "testing" + + "github.com/cisco-open/grabit/test" + "github.com/stretchr/testify/assert" +) + +func TestRunDelete(t *testing.T) { + testfilepath := test.TmpFile(t, ` + [[Resource]] + Urls = ['http://localhost:123456/test.html'] + Integrity = 'sha256-asdasdasd' + Tags = ['tag1', 'tag2'] +`) + cmd := NewRootCmd() + cmd.SetArgs([]string{"-f", testfilepath, "delete", "http://localhost:123456/test.html"}) + err := cmd.Execute() + assert.Nil(t, err) +} diff --git a/cmd/dowload_test.go b/cmd/dowload_test.go new file mode 100644 index 0000000..1ddf109 --- /dev/null +++ b/cmd/dowload_test.go @@ -0,0 +1,118 @@ +package cmd + +import ( + "fmt" + "net/http" + "os" + "testing" + + "github.com/cisco-open/grabit/test" + "github.com/stretchr/testify/assert" +) + +func TestRunDownload(t *testing.T) { + handler := func(w http.ResponseWriter, r *http.Request) { + _, err := w.Write([]byte(`abcdef`)) + if err != nil { + t.Fatal(err) + } + } + port, server := test.HttpHandler(handler) + defer server.Close() + testfilepath := test.TmpFile(t, fmt.Sprintf(` + [[Resource]] + Urls = ['http://localhost:%d/test.html'] + Integrity = 'sha256-vvV+x/U6bUC+tkCngKY5yDvCmsipgW8fxsXG3Nk8RyE=' +`, port)) + outputDir := test.TmpDir(t) + cmd := NewRootCmd() + cmd.SetArgs([]string{"-f", testfilepath, "download", "--dir", outputDir}) + err := cmd.Execute() + assert.Nil(t, err) +} + +func TestRunDownloadWithTags(t *testing.T) { + handler := func(w http.ResponseWriter, r *http.Request) { + _, err := w.Write([]byte(`abcdef`)) + if err != nil { + t.Fatal(err) + } + } + port, server := test.HttpHandler(handler) + defer server.Close() + testfilepath := test.TmpFile(t, fmt.Sprintf(` + [[Resource]] + Urls = ['http://localhost:%d/test.html'] + Integrity = 'sha256-vvV+x/U6bUC+tkCngKY5yDvCmsipgW8fxsXG3Nk8RyE=' + Tags = ['tag'] + + [[Resource]] + Urls = ['http://localhost:%d/test2.html'] + Integrity = 'sha256-vvV+x/U6bUC+tkCngKY5yDvCmsipgW8fxsXG3Nk8RyE=' + Tags = ['tag1', 'tag2'] +`, port, port)) + outputDir := test.TmpDir(t) + cmd := NewRootCmd() + cmd.SetArgs([]string{"-f", testfilepath, "download", "--tag", "tag", "--dir", outputDir}) + err := cmd.Execute() + assert.Nil(t, err) + files, err := os.ReadDir(outputDir) + assert.Nil(t, err) + actualFiles := []string{} + for _, file := range files { + actualFiles = append(actualFiles, file.Name()) + } + assert.ElementsMatch(t, []string{"test.html"}, actualFiles) +} + +func TestRunDownloadWithoutTags(t *testing.T) { + handler := func(w http.ResponseWriter, r *http.Request) { + _, err := w.Write([]byte(`abcdef`)) + if err != nil { + t.Fatal(err) + } + } + port, server := test.HttpHandler(handler) + defer server.Close() + testfilepath := test.TmpFile(t, fmt.Sprintf(` + [[Resource]] + Urls = ['http://localhost:%d/test.html'] + Integrity = 'sha256-vvV+x/U6bUC+tkCngKY5yDvCmsipgW8fxsXG3Nk8RyE=' + Tags = ['tag'] + + [[Resource]] + Urls = ['http://localhost:%d/test2.html'] + Integrity = 'sha256-vvV+x/U6bUC+tkCngKY5yDvCmsipgW8fxsXG3Nk8RyE=' + Tags = ['tag1', 'tag2'] +`, port, port)) + outputDir := test.TmpDir(t) + cmd := NewRootCmd() + cmd.SetArgs([]string{"-f", testfilepath, "download", "--notag", "tag", "--dir", outputDir}) + err := cmd.Execute() + assert.Nil(t, err) + files, err := os.ReadDir(outputDir) + assert.Nil(t, err) + actualFiles := []string{} + for _, file := range files { + actualFiles = append(actualFiles, file.Name()) + } + assert.ElementsMatch(t, []string{"test2.html"}, actualFiles) +} + +func TestRunDownloadMultipleErrors(t *testing.T) { + testfilepath := test.TmpFile(t, ` + [[Resource]] + Urls = ['http://localhost:1234/test.html'] + Integrity = 'sha256-vvV+x/U6bUC+tkCngKY5yDvCmsipgW8fxsXG3Nk8RyE=' + + [[Resource]] + Urls = ['http://cannot-be-resolved.no:12/test.html'] + Integrity = 'sha256-vvV+x/U6bUC+tkCngKY5yDvCmsipgW8fxsXG3Nk8RyE=' +`) + cmd := NewRootCmd() + cmd.SetArgs([]string{"-f", testfilepath, "download"}) + err := cmd.Execute() + assert.NotNil(t, err) + assert.Contains(t, err.Error(), "failed to download") + assert.Contains(t, err.Error(), "lookup cannot-be-resolved.no: no such host") +} diff --git a/cmd/download.go b/cmd/download.go index 0756ebd..2e29611 100644 --- a/cmd/download.go +++ b/cmd/download.go @@ -8,34 +8,48 @@ import ( "github.com/spf13/cobra" ) -func init() { - rootCmd.AddCommand(downloadCmd) +func addDownload(cmd *cobra.Command) { + downloadCmd := &cobra.Command{ + Use: "download", + Short: "Download defined resources", + Args: cobra.NoArgs, + RunE: runFetch, + } downloadCmd.Flags().String("dir", ".", "Target directory where to store the files") downloadCmd.Flags().StringArray("tag", []string{}, "Only download the resources with the given tag") downloadCmd.Flags().StringArray("notag", []string{}, "Only download the resources without the given tag") downloadCmd.Flags().String("perm", "", "Optional permissions for the downloaded files (e.g. '644')") + cmd.AddCommand(downloadCmd) } -var downloadCmd = &cobra.Command{ - Use: "download", - Short: "Download defined resources", - Args: cobra.NoArgs, - Run: runFetch, -} - -func runFetch(cmd *cobra.Command, args []string) { +func runFetch(cmd *cobra.Command, args []string) error { lockFile, err := cmd.Flags().GetString("lock-file") - FatalIfNotNil(err) + if err != nil { + return err + } lock, err := internal.NewLock(lockFile, false) - FatalIfNotNil(err) + if err != nil { + return err + } dir, err := cmd.Flags().GetString("dir") - FatalIfNotNil(err) + if err != nil { + return err + } tags, err := cmd.Flags().GetStringArray("tag") - FatalIfNotNil(err) + if err != nil { + return err + } notags, err := cmd.Flags().GetStringArray("notag") - FatalIfNotNil(err) + if err != nil { + return err + } perm, err := cmd.Flags().GetString("perm") - FatalIfNotNil(err) + if err != nil { + return err + } err = lock.Download(dir, tags, notags, perm) - FatalIfNotNil(err) + if err != nil { + return err + } + return nil } diff --git a/cmd/root.go b/cmd/root.go index 3840e23..854dab2 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -13,9 +13,19 @@ import ( "github.com/spf13/cobra" ) -var rootCmd = &cobra.Command{ - Use: "grabit", - Short: "Grabit downloads files from remote locations and verifies their integrity", +func NewRootCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "grabit", + Short: "Grabit downloads files from remote locations and verifies their integrity", + SilenceUsage: true, + } + cmd.PersistentFlags().StringP("lock-file", "f", filepath.Join(getPwd(), GRAB_LOCK), "lockfile path (default: $PWD/grabit.lock") + cmd.PersistentFlags().StringP("log-level", "l", "info", "log level (trace, debug, info, warn, error, fatal)") + addDelete(cmd) + addDownload(cmd) + addAdd(cmd) + addVersion(cmd) + return cmd } func getPwd() string { @@ -28,16 +38,8 @@ func getPwd() string { var GRAB_LOCK = "grabit.lock" -func init() { - cobra.OnInitialize(initLog) - rootCmd.PersistentFlags().StringP("lock-file", "f", filepath.Join(getPwd(), GRAB_LOCK), "lockfile path (default: $PWD/grabit.lock") - rootCmd.PersistentFlags().StringP("log-level", "l", "info", "log level (trace, debug, info, warn, error, fatal)") -} - -func initLog() { +func initLog(ll string) { zerolog.SetGlobalLevel(zerolog.InfoLevel) - ll, err := rootCmd.Flags().GetString("log-level") - FatalIfNotNil(err) switch strings.ToLower(ll) { case "trace": zerolog.SetGlobalLevel(zerolog.TraceLevel) @@ -56,7 +58,12 @@ func initLog() { } } -func Execute() { +func Execute(rootCmd *cobra.Command) { + ll, err := rootCmd.PersistentFlags().GetString("log-level") + if err != nil { + log.Fatal().Msg(err.Error()) + } + initLog(ll) if err := rootCmd.Execute(); err != nil { if strings.Contains(err.Error(), "unknown flag") { // exit code 126: Command invoked cannot execute @@ -65,9 +72,3 @@ func Execute() { log.Fatal().Msg(err.Error()) } } - -func FatalIfNotNil(err error) { - if err != nil { - log.Fatal().Msg(err.Error()) - } -} diff --git a/cmd/root_test.go b/cmd/root_test.go new file mode 100644 index 0000000..25d86a6 --- /dev/null +++ b/cmd/root_test.go @@ -0,0 +1,16 @@ +package cmd + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestRunRoot(t *testing.T) { + rootCmd := NewRootCmd() + buf := new(bytes.Buffer) + rootCmd.SetOutput(buf) + Execute(rootCmd) + assert.Contains(t, buf.String(), "and verifies their integrity") +} diff --git a/cmd/version.go b/cmd/version.go index 7099d13..8bab221 100644 --- a/cmd/version.go +++ b/cmd/version.go @@ -10,14 +10,13 @@ import ( "github.com/spf13/cobra" ) -func init() { - rootCmd.AddCommand(versionCmd) -} - -var versionCmd = &cobra.Command{ - Use: "version", - Short: "Display grabit version", - Run: runVersion, +func addVersion(cmd *cobra.Command) { + var versionCmd = &cobra.Command{ + Use: "version", + Short: "Display grabit version", + Run: runVersion, + } + cmd.AddCommand(versionCmd) } func runVersion(cmd *cobra.Command, args []string) { diff --git a/internal/lock.go b/internal/lock.go index da54c52..ae6707b 100644 --- a/internal/lock.go +++ b/internal/lock.go @@ -6,6 +6,7 @@ package internal import ( "bufio" "context" + "errors" "fmt" "os" "strconv" @@ -157,15 +158,21 @@ func (l *Lock) Download(dir string, tags []string, notags []string, perm string) }() } done := 0 - for err := range errorCh { + errs := []error{} + for range total { + err = <-errorCh if err != nil { - return err - } - done += 1 - if done == total { - return nil + errs = append(errs, err) + } else { + done += 1 } } + if done == total { + return nil + } + if len(errs) > 0 { + return errors.Join(errs...) + } return nil } diff --git a/internal/lock_test.go b/internal/lock_test.go index 3ee8ecb..eb347ed 100644 --- a/internal/lock_test.go +++ b/internal/lock_test.go @@ -10,6 +10,7 @@ import ( "path/filepath" "testing" + "github.com/cisco-open/grabit/test" "github.com/stretchr/testify/assert" ) @@ -19,7 +20,7 @@ func TestNewLockInvalid(t *testing.T) { } func TestNewLockValid(t *testing.T) { - path := tmpFile(t, ` + path := test.TmpFile(t, ` [[Resource]] Urls = ['http://localhost:123456/test.html'] Integrity = 'sha256-asdasdasd' @@ -34,7 +35,7 @@ func TestNewLockValid(t *testing.T) { } func TestLockManipulations(t *testing.T) { - path := tmpFile(t, ` + path := test.TmpFile(t, ` [[Resource]] Urls = ['http://localhost:123456/test.html'] Integrity = 'sha256-asdasdasd' @@ -47,7 +48,7 @@ func TestLockManipulations(t *testing.T) { t.Fatal(err) } } - port, server := httpHandler(handler) + port, server := test.HttpHandler(handler) defer server.Close() resource := fmt.Sprintf("http://localhost:%d/test2.html", port) err = lock.AddResource([]string{resource}, "sha512", []string{}, "") @@ -61,7 +62,7 @@ func TestLockManipulations(t *testing.T) { func TestDuplicateResource(t *testing.T) { url := "http://localhost:123456/test.html" - path := tmpFile(t, fmt.Sprintf(` + path := test.TmpFile(t, fmt.Sprintf(` [[Resource]] Urls = ['%s'] Integrity = 'sha256-asdasdasd'`, url)) @@ -103,9 +104,9 @@ func TestDownload(t *testing.T) { t.Fatal(err) } } - port, server := httpHandler(handler) + port, server := test.HttpHandler(handler) defer server.Close() - path := tmpFile(t, fmt.Sprintf(` + path := test.TmpFile(t, fmt.Sprintf(` [[Resource]] Urls = ['http://localhost:%d/test.html'] Integrity = 'sha256-vvV+x/U6bUC+tkCngKY5yDvCmsipgW8fxsXG3Nk8RyE='`, port)) @@ -113,7 +114,7 @@ func TestDownload(t *testing.T) { strPerm := "-r--rw-rwx" lock, err := NewLock(path, false) assert.Nil(t, err) - dir := tmpDir(t) + dir := test.TmpDir(t) err = lock.Download(dir, []string{}, []string{}, perm) if err != nil { t.Fatal(err) diff --git a/internal/resource.go b/internal/resource.go index aba6407..1b89c28 100644 --- a/internal/resource.go +++ b/internal/resource.go @@ -88,11 +88,13 @@ func (l *Resource) Download(dir string, mode os.FileMode, ctx context.Context) e if err != nil { return err } + var downloadError error = nil for _, u := range l.Urls { // Download file in the target directory so that the call to // os.Rename is atomic. lpath, err := GetUrlToDir(u, dir, ctx) if err != nil { + downloadError = err break } err = checkIntegrityFromFile(lpath, algo, l.Integrity, u) @@ -117,6 +119,13 @@ func (l *Resource) Download(dir string, mode os.FileMode, ctx context.Context) e ok = true } if !ok { + if err == nil { + if downloadError != nil { + return downloadError + } else { + panic("no error but no file downloaded") + } + } return err } return nil diff --git a/internal/resource_test.go b/internal/resource_test.go index e5b3de4..80a11e9 100644 --- a/internal/resource_test.go +++ b/internal/resource_test.go @@ -8,6 +8,7 @@ import ( "net/http" "testing" + "github.com/cisco-open/grabit/test" "github.com/stretchr/testify/assert" ) @@ -18,7 +19,7 @@ func TestNewResourceFromUrl(t *testing.T) { t.Fatal(err) } } - port, server := httpHandler(handler) + port, server := test.HttpHandler(handler) defer server.Close() algo := "sha256" tests := []struct { diff --git a/internal/sri_test.go b/internal/sri_test.go index 2f4cc38..bbeda73 100644 --- a/internal/sri_test.go +++ b/internal/sri_test.go @@ -6,6 +6,7 @@ package internal import ( "testing" + "github.com/cisco-open/grabit/test" "github.com/stretchr/testify/assert" ) @@ -45,7 +46,7 @@ func TestGetAlgoFromIntegrity(t *testing.T) { } func TestGetIntegrityFromFile(t *testing.T) { - path := tmpFile(t, "abcdef") + path := test.TmpFile(t, "abcdef") sri, err := getIntegrityFromFile(path, "sha256") assert.Nil(t, err) assert.Equal(t, "sha256-vvV+x/U6bUC+tkCngKY5yDvCmsipgW8fxsXG3Nk8RyE=", sri) @@ -58,7 +59,7 @@ func TestCheckIntegrityFromFile(t *testing.T) { } func TestCheckIntegrityFromFileInvalid(t *testing.T) { - path := tmpFile(t, "abcdef") + path := test.TmpFile(t, "abcdef") err := checkIntegrityFromFile(path, "sha256", "invalid", "") assert.NotNil(t, err) assert.Contains(t, err.Error(), "integrity mismatch") diff --git a/main.go b/main.go index 01f2ea4..1e4b6a9 100644 --- a/main.go +++ b/main.go @@ -21,7 +21,8 @@ func main() { signal.Notify(stopChan, os.Interrupt) go listenForInterrupt(stopChan) - cmd.Execute() + rootCmd := cmd.NewRootCmd() + cmd.Execute(rootCmd) } func listenForInterrupt(stopScan chan os.Signal) { diff --git a/internal/utils_test.go b/test/utils.go similarity index 83% rename from internal/utils_test.go rename to test/utils.go index 6078dc8..4175d75 100644 --- a/internal/utils_test.go +++ b/test/utils.go @@ -1,7 +1,7 @@ // Copyright (c) 2023 Cisco Systems, Inc. and its affiliates // All rights reserved. -package internal +package test import ( "fmt" @@ -13,7 +13,7 @@ import ( "testing" ) -func tmpFile(t *testing.T, content string) string { +func TmpFile(t *testing.T, content string) string { f, err := os.CreateTemp(t.TempDir(), "test") if err != nil { t.Fatal(err) @@ -27,7 +27,7 @@ func tmpFile(t *testing.T, content string) string { return name } -func tmpDir(t *testing.T) string { +func TmpDir(t *testing.T) string { dir, err := os.MkdirTemp(t.TempDir(), "test") if err != nil { log.Fatal(err) @@ -36,7 +36,7 @@ func tmpDir(t *testing.T) string { return dir } -func httpHandler(handler http.HandlerFunc) (int, *httptest.Server) { +func HttpHandler(handler http.HandlerFunc) (int, *httptest.Server) { testPort := 12345 // TODO: dynamically find free port l, err := net.Listen("tcp", fmt.Sprintf("127.0.0.1:%d", testPort)) if err != nil {