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

@@ -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()))
 }

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()))
 }

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)
 }