From ed34fad2e589adb992c644fbe05b4cfabc421d1e Mon Sep 17 00:00:00 2001 From: Tom Wieczorek Date: Sat, 11 Mar 2023 17:49:00 +0100 Subject: [PATCH] Better testify usage (#54) * Use assert.NoError instead of assert.Nil This gives nicer error messages. * Swap arguments to assert.Equal The expectation comes first. Otherwise, error messages will be misleading. An example: === RUN TestDeleteWithIncorrectRepoForDeleteCaches Error: authentication token not found for host github.com delete_test.go:56: Error Trace: /build/source/cmd/delete_test.go:56 Error: Should be true Test: TestDeleteWithIncorrectRepoForDeleteCaches Messages: 1 unmatched mocks: https://api.github.com/repos/testOrg/testRepo/actions/caches?key=cacheName delete_test.go:59: Error Trace: /build/source/cmd/delete_test.go:59 Error: Not equal: expected: "authentication token not found for host github.com" actual : "The given repo does not exist." Diff: --- Expected +++ Actual @@ -1 +1 @@ -authentication token not found for host github.com +The given repo does not exist. Test: TestDeleteWithIncorrectRepoForDeleteCaches --- FAIL: TestDeleteWithIncorrectRepoForDeleteCaches (0.00s) * Use assert.Error instead of assert.NotNil This gives nicer error messages. * Use assert.ErrorAs and assert.ErrorContains This simplifies the assertions and potentially gives better error messages. * Use require instead of assert and use assert.NotNil as guard This is to prevent panics in tests, when things get accessed which shouldn't be nil. --- cmd/delete_test.go | 39 +++++++----------- cmd/list_test.go | 54 ++++++++---------------- internal/utils_test.go | 31 +++++++------- service/actions_cache_test.go | 77 +++++++++++++++++++---------------- 4 files changed, 87 insertions(+), 114 deletions(-) diff --git a/cmd/delete_test.go b/cmd/delete_test.go index 95169c3..754cd3b 100644 --- a/cmd/delete_test.go +++ b/cmd/delete_test.go @@ -1,9 +1,6 @@ package cmd import ( - "errors" - "fmt" - "reflect" "testing" "github.com/actions/gh-actions-cache/internal" @@ -19,8 +16,7 @@ func TestDeleteWithIncorrectArguments(t *testing.T) { cmd.SetArgs([]string{}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, err, fmt.Errorf("accepts 1 arg(s), received 0")) + assert.ErrorContains(t, err, "accepts 1 arg(s), received 0") assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -31,8 +27,7 @@ func TestDeleteWithIncorrectRepo(t *testing.T) { cmd.SetArgs([]string{"--repo", "testOrg/testRepo/123/123", "cacheName"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, err, fmt.Errorf("expected the \"[HOST/]OWNER/REPO\" format, got \"testOrg/testRepo/123/123\"")) + assert.ErrorContains(t, err, "expected the \"[HOST/]OWNER/REPO\" format, got \"testOrg/testRepo/123/123\"") assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -52,11 +47,11 @@ func TestDeleteWithIncorrectRepoForDeleteCaches(t *testing.T) { cmd.SetArgs([]string{"--repo", "testOrg/testRepo", "cacheName"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) var customError types.HandledError - errors.As(err, &customError) - assert.Equal(t, customError.Message, "The given repo does not exist.") + if assert.ErrorAs(t, err, &customError) { + assert.Equal(t, "The given repo does not exist.", customError.Message) + } + assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } func TestDeleteSuccessWithConfirmFlagProvided(t *testing.T) { @@ -85,7 +80,7 @@ func TestDeleteSuccessWithConfirmFlagProvided(t *testing.T) { cmd.SetArgs([]string{"--repo", "testOrg/testRepo", "2022-06-29T13:33:49", "--confirm"}) err := cmd.Execute() - assert.Nil(t, err) + assert.NoError(t, err) assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -116,7 +111,7 @@ func TestDeleteFailureWhileTakingUserInput(t *testing.T) { cmd.SetArgs([]string{"--repo", "testOrg/testRepo", "2022-06-29T13:33:49"}) err := cmd.Execute() - assert.NotNil(t, err) + assert.Error(t, err) assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -135,13 +130,10 @@ func TestDeleteWithUnauthorizedRequestForDeleteCaches(t *testing.T) { cmd.SetArgs([]string{"--repo", "testOrg/testRepo", "cacheKey", "--confirm"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, reflect.TypeOf(err), reflect.TypeOf(types.HandledError{})) - var customError types.HandledError - errors.As(err, &customError) - assert.Equal(t, customError.Message, "Must have admin rights to Repository.") - + if assert.ErrorAs(t, err, &customError) { + assert.Equal(t, "Must have admin rights to Repository.", customError.Message) + } assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -160,12 +152,9 @@ func TestDeleteWithInternalServerErrorForDeleteCaches(t *testing.T) { cmd.SetArgs([]string{"--repo", "testOrg/testRepo", "cacheKey", "--confirm"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, reflect.TypeOf(err), reflect.TypeOf(types.HandledError{})) - var customError types.HandledError - errors.As(err, &customError) - assert.Equal(t, customError.Message, "We could not process your request due to internal error.") - + if assert.ErrorAs(t, err, &customError) { + assert.Equal(t, "We could not process your request due to internal error.", customError.Message) + } assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } diff --git a/cmd/list_test.go b/cmd/list_test.go index f8429dc..5fd2227 100644 --- a/cmd/list_test.go +++ b/cmd/list_test.go @@ -1,10 +1,6 @@ package cmd import ( - "errors" - "fmt" - - "reflect" "testing" "github.com/actions/gh-actions-cache/internal" @@ -21,8 +17,7 @@ func TestListWithIncorrectArguments(t *testing.T) { cmd.SetArgs([]string{"keyValue"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, err, fmt.Errorf("Invalid argument(s). Expected 0 received 1")) + assert.ErrorContains(t, err, "Invalid argument(s). Expected 0 received 1") assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -33,8 +28,7 @@ func TestListWithIncorrectRepo(t *testing.T) { cmd.SetArgs([]string{"--repo", "testOrg/testRepo/123/123"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, err, fmt.Errorf("expected the \"[HOST/]OWNER/REPO\" format, got \"testOrg/testRepo/123/123\"")) + assert.ErrorContains(t, err, "expected the \"[HOST/]OWNER/REPO\" format, got \"testOrg/testRepo/123/123\"") assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -45,8 +39,7 @@ func TestListWithNegativeLimit(t *testing.T) { cmd.SetArgs([]string{"--limit", "-1", "--repo", "testOrg/testRepo"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, err, fmt.Errorf("-1 is not a valid integer value for limit flag. Allowed values: 1-100")) + assert.ErrorContains(t, err, "-1 is not a valid integer value for limit flag. Allowed values: 1-100") assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -57,8 +50,7 @@ func TestListWithIncorrectLimit(t *testing.T) { cmd.SetArgs([]string{"--limit", "101", "--repo", "testOrg/testRepo"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, err, fmt.Errorf("101 is not a valid integer value for limit flag. Allowed values: 1-100")) + assert.ErrorContains(t, err, "101 is not a valid integer value for limit flag. Allowed values: 1-100") assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -69,8 +61,7 @@ func TestListLimitShorthandUsingIncorrectLimit(t *testing.T) { cmd.SetArgs([]string{"-L", "102", "--repo", "testOrg/testRepo"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, err, fmt.Errorf("102 is not a valid integer value for limit flag. Allowed values: 1-100")) + assert.ErrorContains(t, err, "102 is not a valid integer value for limit flag. Allowed values: 1-100") assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -81,8 +72,7 @@ func TestListWithIncorrectOrder(t *testing.T) { cmd.SetArgs([]string{"--order", "incorrectOrderValue", "--repo", "testOrg/testRepo"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, err, fmt.Errorf("incorrectOrderValue is not a valid value for order flag. Allowed values: asc/desc")) + assert.ErrorContains(t, err, "incorrectOrderValue is not a valid value for order flag. Allowed values: asc/desc") assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -93,8 +83,7 @@ func TestListWithIncorrectSort(t *testing.T) { cmd.SetArgs([]string{"--sort", "incorrectSortValue", "--repo", "testOrg/testRepo"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, err, fmt.Errorf("incorrectSortValue is not a valid value for sort flag. Allowed values: last-used/size/created-at")) + assert.ErrorContains(t, err, "incorrectSortValue is not a valid value for sort flag. Allowed values: last-used/size/created-at") assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -121,13 +110,10 @@ func TestListWithIncorrectRepoForListCaches(t *testing.T) { cmd.SetArgs([]string{"--repo", "testOrg/testRepo"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, reflect.TypeOf(err), reflect.TypeOf(types.HandledError{})) - var customError types.HandledError - errors.As(err, &customError) - assert.Equal(t, customError.Message, "The given repo does not exist.") - + if assert.ErrorAs(t, err, &customError) { + assert.Equal(t, "The given repo does not exist.", customError.Message) + } assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -154,13 +140,10 @@ func TestListWithUnauthorizedRequestForListCaches(t *testing.T) { cmd.SetArgs([]string{"--repo", "testOrg/testRepo"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, reflect.TypeOf(err), reflect.TypeOf(types.HandledError{})) - var customError types.HandledError - errors.As(err, &customError) - assert.Equal(t, customError.Message, "Must have admin rights to Repository.") - + if assert.ErrorAs(t, err, &customError) { + assert.Equal(t, "Must have admin rights to Repository.", customError.Message) + } assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -187,13 +170,10 @@ func TestListWithInternalServerErrorForListCaches(t *testing.T) { cmd.SetArgs([]string{"--repo", "testOrg/testRepo"}) err := cmd.Execute() - assert.NotNil(t, err) - assert.Equal(t, reflect.TypeOf(err), reflect.TypeOf(types.HandledError{})) - var customError types.HandledError - errors.As(err, &customError) - assert.Equal(t, customError.Message, "We could not process your request due to internal error.") - + if assert.ErrorAs(t, err, &customError) { + assert.Equal(t, "We could not process your request due to internal error.", customError.Message) + } assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -229,6 +209,6 @@ func TestListSuccess(t *testing.T) { cmd.SetArgs([]string{"--repo", "testOrg/testRepo"}) err := cmd.Execute() - assert.Nil(t, err) + assert.NoError(t, err) assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } diff --git a/internal/utils_test.go b/internal/utils_test.go index 1bf6e68..e2d64d7 100644 --- a/internal/utils_test.go +++ b/internal/utils_test.go @@ -11,45 +11,44 @@ func TestGetRepo_IncorrectRepoString(t *testing.T) { r := "testOrg/testRepo/123/123" repo, err := GetRepo(r) - assert.Error(t, err) + assert.ErrorContains(t, err, fmt.Sprintf("expected the \"[HOST/]OWNER/REPO\" format, got \"%s\"", r)) assert.Nil(t, repo) - assert.Equal(t, err.Error(), fmt.Sprintf("expected the \"[HOST/]OWNER/REPO\" format, got \"%s\"", r)) } func TestGetRepo_CorrectRepoString(t *testing.T) { r := "testOrg/testRepo" repo, err := GetRepo(r) - assert.NotNil(t, repo) - assert.Nil(t, err) - assert.Equal(t, repo.Host(), "github.com") - assert.Equal(t, repo.Owner(), "testOrg") - assert.Equal(t, repo.Name(), "testRepo") + assert.NoError(t, err) + if assert.NotNil(t, repo) { + assert.Equal(t, "github.com", repo.Host()) + assert.Equal(t, "testOrg", repo.Owner()) + assert.Equal(t, "testRepo", repo.Name()) + } } func TestGetRepo_CorrectRepoStringWithCustomHost(t *testing.T) { r := "api.testEnterprise.com/testOrg/testRepo" repo, err := GetRepo(r) - assert.NotNil(t, repo) - assert.Nil(t, err) - assert.Equal(t, repo.Host(), "api.testEnterprise.com") - assert.Equal(t, repo.Owner(), "testOrg") - assert.Equal(t, repo.Name(), "testRepo") + assert.NoError(t, err) + if assert.NotNil(t, repo) { + assert.Equal(t, "api.testEnterprise.com", repo.Host()) + assert.Equal(t, "testOrg", repo.Owner()) + assert.Equal(t, "testRepo", repo.Name()) + } } func TestFormatCacheSize_MB(t *testing.T) { cacheSizeInBytes := 1024 * 1024 * 1.5 cacheSizeDetailString := FormatCacheSize(cacheSizeInBytes) - assert.NotNil(t, cacheSizeDetailString) - assert.Equal(t, cacheSizeDetailString, "1.50 MB") + assert.Equal(t, "1.50 MB", cacheSizeDetailString) } func TestFormatCacheSize_GB(t *testing.T) { cacheSizeInBytes := 1024 * 1024 * 1024 * 1.5 cacheSizeDetailString := FormatCacheSize(cacheSizeInBytes) - assert.NotNil(t, cacheSizeDetailString) - assert.Equal(t, cacheSizeDetailString, "1.50 GB") + assert.Equal(t, "1.50 GB", cacheSizeDetailString) } diff --git a/service/actions_cache_test.go b/service/actions_cache_test.go index c89ac8f..a430db1 100644 --- a/service/actions_cache_test.go +++ b/service/actions_cache_test.go @@ -1,7 +1,6 @@ package service import ( - "errors" "net/url" "testing" @@ -9,6 +8,7 @@ import ( "github.com/actions/gh-actions-cache/types" "github.com/cli/go-gh/pkg/api" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "gopkg.in/h2non/gock.v1" ) @@ -27,14 +27,15 @@ func TestGetCacheUsage_CorrectRepo(t *testing.T) { }`) repo, err := internal.GetRepo("testOrg/testRepo") - assert.Nil(t, err) + require.NoError(t, err) artifactCache, err := NewArtifactCache(repo, "list", VERSION) - assert.Nil(t, err) + require.NoError(t, err) + require.NotNil(t, artifactCache) totalCacheSize, err := artifactCache.GetCacheUsage() - assert.Equal(t, totalCacheSize, float64(291205)) - assert.Nil(t, err) + assert.NoError(t, err) + assert.Equal(t, float64(291205), totalCacheSize) assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -50,18 +51,18 @@ func TestGetCacheUsage_IncorrectRepo(t *testing.T) { }`) repo, err := internal.GetRepo("testOrg/wrongRepo") - assert.Nil(t, err) + require.NoError(t, err) artifactCache, err := NewArtifactCache(repo, "list", VERSION) - assert.Nil(t, err) + require.NoError(t, err) + require.NotNil(t, artifactCache) totalCacheSize, err := artifactCache.GetCacheUsage() var httpError api.HTTPError - errors.As(err, &httpError) - - assert.NotNil(t, err) - assert.Equal(t, httpError.StatusCode, 404) - assert.Equal(t, httpError.Message, "Not Found") - assert.Equal(t, totalCacheSize, float64(-1)) + if assert.ErrorAs(t, err, &httpError) { + assert.Equal(t, 404, httpError.StatusCode) + assert.Equal(t, "Not Found", httpError.Message) + } + assert.Equal(t, float64(-1), totalCacheSize) assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -86,21 +87,23 @@ func TestListCaches_Success(t *testing.T) { }`) repo, err := internal.GetRepo("testOrg/testRepo") - assert.Nil(t, err) + require.NoError(t, err) f := types.ListOptions{BaseOptions: types.BaseOptions{Repo: "testOrg/testRepo"}, Limit: 30} queryParams := url.Values{} f.GenerateQueryParams(queryParams) artifactCache, err := NewArtifactCache(repo, "list", VERSION) - assert.Nil(t, err) + require.NoError(t, err) + require.NotNil(t, artifactCache) listCacheResponse, err := artifactCache.ListCaches(queryParams) - assert.Nil(t, err) - assert.NotNil(t, listCacheResponse) - assert.Equal(t, listCacheResponse.TotalCount, 1) - assert.Equal(t, len(listCacheResponse.ActionsCaches), 1) - assert.Equal(t, listCacheResponse.ActionsCaches[0].Id, 29) + assert.NoError(t, err) + if assert.NotNil(t, listCacheResponse) { + assert.Equal(t, 1, listCacheResponse.TotalCount) + assert.Equal(t, 1, len(listCacheResponse.ActionsCaches)) + assert.Equal(t, 29, listCacheResponse.ActionsCaches[0].Id) + } assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -116,22 +119,22 @@ func TestListCaches_Failure(t *testing.T) { }`) repo, err := internal.GetRepo("testOrg/testRepo") - assert.Nil(t, err) + require.NoError(t, err) f := types.ListOptions{BaseOptions: types.BaseOptions{Repo: "testOrg/testRepo"}, Limit: 30} queryParams := url.Values{} f.GenerateQueryParams(queryParams) artifactCache, err := NewArtifactCache(repo, "list", VERSION) - assert.Nil(t, err) + require.NoError(t, err) + require.NotNil(t, artifactCache) listCacheResponse, err := artifactCache.ListCaches(queryParams) var httpError api.HTTPError - errors.As(err, &httpError) - - assert.NotNil(t, err) - assert.Equal(t, httpError.StatusCode, 404) - assert.Equal(t, httpError.Message, "Not Found") - assert.Equal(t, listCacheResponse, types.ListApiResponse{}) + if assert.ErrorAs(t, err, &httpError) { + assert.Equal(t, 404, httpError.StatusCode) + assert.Equal(t, "Not Found", httpError.Message) + } + assert.Equal(t, types.ListApiResponse{}, listCacheResponse) assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -156,18 +159,19 @@ func TestDeleteCaches_Success(t *testing.T) { }`) repo, err := internal.GetRepo("testOrg/testRepo") - assert.Nil(t, err) + require.NoError(t, err) f := types.DeleteOptions{BaseOptions: types.BaseOptions{Repo: "testOrg/testRepo"}} queryParams := url.Values{} f.GenerateBaseQueryParams(queryParams) artifactCache, err := NewArtifactCache(repo, "delete", VERSION) - assert.Nil(t, err) + require.NoError(t, err) + require.NotNil(t, artifactCache) deletedCache, err := artifactCache.DeleteCaches(queryParams) - assert.Nil(t, err) - assert.Equal(t, deletedCache, 1) + assert.NoError(t, err) + assert.Equal(t, 1, deletedCache) assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) } @@ -183,17 +187,18 @@ func TestDeleteCaches_Failure(t *testing.T) { }`) repo, err := internal.GetRepo("testOrg/testRepo") - assert.Nil(t, err) + require.NoError(t, err) f := types.DeleteOptions{BaseOptions: types.BaseOptions{Repo: "testOrg/testRepo"}} queryParams := url.Values{} f.GenerateBaseQueryParams(queryParams) artifactCache, err := NewArtifactCache(repo, "delete", VERSION) - assert.Nil(t, err) + require.NoError(t, err) + require.NotNil(t, artifactCache) deletedCache, err := artifactCache.DeleteCaches(queryParams) - assert.NotNil(t, err) - assert.Equal(t, deletedCache, 0) + assert.Error(t, err) + assert.Equal(t, 0, deletedCache) assert.True(t, gock.IsDone(), internal.PrintPendingMocks(gock.Pending())) }