fix: enhance error handling and improve test assertions (#32352)

Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>

.golangci.yml

@@ -74,9 +74,9 @@ linters:
 
     errorlint:
       # Check for plain type assertions and type switches.
-      asserts: false
+      asserts: true
       # Check for plain error comparisons.
-      comparison: false
+      comparison: true
 
     exhaustive:
       default-signifies-exhaustive: true

internal/chart/v3/lint/rules/deprecations_test.go

@@ -29,14 +29,12 @@ func TestValidateNoDeprecations(t *testing.T) {
 		Kind:       "Deployment",
 	}
 	err := validateNoDeprecations(deprecated, nil)
-	var depErr deprecatedAPIError
 	require.Error(t, err, "Expected deprecated extension to be flagged")
+	var depErr deprecatedAPIError
 	require.ErrorAs(t, err, &depErr, "Expected error to be of type deprecatedAPIError")
 	require.NotEmpty(t, depErr.Message, "Expected error message to be non-blank: %v", err)
-
-	err = validateNoDeprecations(&k8sYamlStruct{
+	assert.NoError(t, validateNoDeprecations(&k8sYamlStruct{
 		APIVersion: "v1",
 		Kind:       "Pod",
-	}, nil)
-	assert.NoError(t, err, "Expected a v1 Pod to not be deprecated")
+	}, nil), "Expected a v1 Pod to not be deprecated")
 }

internal/plugin/runtime_subprocess.go

@@ -18,6 +18,7 @@ package plugin
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"log/slog"
@@ -152,7 +153,8 @@ func (r *SubprocessPluginRuntime) InvokeHook(event string) error {
 
 	slog.Debug("executing plugin hook command", slog.String("pluginName", r.metadata.Name), slog.String("command", cmd.String()))
 	if err := cmd.Run(); err != nil {
-		if eerr, ok := err.(*exec.ExitError); ok {
+		var eerr *exec.ExitError
+		if errors.As(err, &eerr) {
 			os.Stderr.Write(eerr.Stderr)
 			return fmt.Errorf("plugin %s hook for %q exited with error", event, r.metadata.Name)
 		}
@@ -166,7 +168,8 @@ func (r *SubprocessPluginRuntime) InvokeHook(event string) error {
 // then replace the other three with a call to this func
 func executeCmd(prog *exec.Cmd, pluginName string) error {
 	if err := prog.Run(); err != nil {
-		if eerr, ok := err.(*exec.ExitError); ok {
+		var eerr *exec.ExitError
+		if errors.As(err, &eerr) {
 			slog.Debug(
 				"plugin execution failed",
 				slog.String("pluginName", pluginName),

internal/sympath/walk.go

@@ -21,6 +21,7 @@ limitations under the License.
 package sympath
 
 import (
+	"errors"
 	"fmt"
 	"log/slog"
 	"os"
@@ -40,7 +41,7 @@ func Walk(root string, walkFn filepath.WalkFunc) error {
 	} else {
 		err = symwalk(root, info, walkFn)
 	}
-	if err == filepath.SkipDir {
+	if errors.Is(err, filepath.SkipDir) {
 		return nil
 	}
 	return err
@@ -75,7 +76,7 @@ func symwalk(path string, info os.FileInfo, walkFn filepath.WalkFunc) error {
 		if info, err = os.Lstat(resolved); err != nil {
 			return err
 		}
-		if err := symwalk(path, info, walkFn); err != nil && err != filepath.SkipDir {
+		if err := symwalk(path, info, walkFn); err != nil && !errors.Is(err, filepath.SkipDir) {
 			return err
 		}
 		return nil
@@ -98,13 +99,13 @@ func symwalk(path string, info os.FileInfo, walkFn filepath.WalkFunc) error {
 		filename := filepath.Join(path, name)
 		fileInfo, err := os.Lstat(filename)
 		if err != nil {
-			if err := walkFn(filename, fileInfo, err); err != nil && err != filepath.SkipDir {
+			if err := walkFn(filename, fileInfo, err); err != nil && !errors.Is(err, filepath.SkipDir) {
 				return err
 			}
 		} else {
 			err = symwalk(filename, fileInfo, walkFn)
 			if err != nil {
-				if (!fileInfo.IsDir() && !IsSymlink(fileInfo)) || err != filepath.SkipDir {
+				if (!fileInfo.IsDir() && !IsSymlink(fileInfo)) || !errors.Is(err, filepath.SkipDir) {
 					return err
 				}
 			}

pkg/chart/v2/lint/rules/deprecations_test.go

@@ -16,7 +16,11 @@ limitations under the License.
 
 package rules // import "helm.sh/helm/v4/pkg/chart/v2/lint/rules"
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
 
 func TestValidateNoDeprecations(t *testing.T) {
 	deprecated := &k8sYamlStruct{
@@ -27,10 +31,9 @@ func TestValidateNoDeprecations(t *testing.T) {
 	if err == nil {
 		t.Fatal("Expected deprecated extension to be flagged")
 	}
-	depErr := err.(deprecatedAPIError)
-	if depErr.Message == "" {
-		t.Fatalf("Expected error message to be non-blank: %v", err)
-	}
+	var depErr deprecatedAPIError
+	require.ErrorAs(t, err, &depErr)
+	require.NotEmptyf(t, depErr.Message, "Expected error message to be non-blank")
 
 	if err := validateNoDeprecations(&k8sYamlStruct{
 		APIVersion: "v1",

pkg/chart/v2/lint/rules/template_test.go

@@ -24,6 +24,9 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	"helm.sh/helm/v4/pkg/chart/common"
 	chart "helm.sh/helm/v4/pkg/chart/v2"
 	"helm.sh/helm/v4/pkg/chart/v2/lint/support"
@@ -235,10 +238,9 @@ func TestDeprecatedAPIFails(t *testing.T) {
 		t.Fatalf("Expected 1 lint error, got %d", l)
 	}
 
-	err := linter.Messages[0].Err.(deprecatedAPIError)
-	if err.Deprecated != "apps/v1beta1 Deployment" {
-		t.Errorf("Surprised to learn that %q is deprecated", err.Deprecated)
-	}
+	var depErr deprecatedAPIError
+	require.ErrorAs(t, linter.Messages[0].Err, &depErr)
+	assert.Equalf(t, "apps/v1beta1 Deployment", depErr.Deprecated, "Surprised to learn that %q is deprecated", depErr.Deprecated)
 }
 
 const manifest = `apiVersion: v1

pkg/chart/v2/util/dependencies_test.go

@@ -21,6 +21,8 @@ import (
 	"strconv"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"helm.sh/helm/v4/pkg/chart/common"
 	chart "helm.sh/helm/v4/pkg/chart/v2"
 	"helm.sh/helm/v4/pkg/chart/v2/loader"
@@ -251,12 +253,8 @@ func TestProcessDependencyImportValues(t *testing.T) {
 	if err == nil {
 		t.Error("expect nil value not found but found it")
 	}
-	switch xerr := err.(type) {
-	case common.ErrNoValue:
-		// We found what we expected
-	default:
-		t.Errorf("expected an ErrNoValue but got %q instead", xerr)
-	}
+	var xerr common.ErrNoValue
+	require.ErrorAs(t, err, &xerr, "expected an ErrNoValue")
 
 	c = loadChart(t, "testdata/subpop")
 	if err := processDependencyImportValues(c, true); err != nil {

pkg/cmd/plugin_test.go

@@ -144,16 +144,10 @@ func TestLoadCLIPlugins(t *testing.T) {
 			// tests until this is fixed
 			if runtime.GOOS != "windows" {
 				if err := pluginCmd.RunE(pluginCmd, tt.args); err != nil {
-					if tt.code > 0 {
-						cerr, ok := err.(CommandError)
-						if !ok {
-							t.Errorf("Expected %s to return pluginError: got %v(%T)", tt.use, err, err)
-						}
-						if cerr.ExitCode != tt.code {
-							t.Errorf("Expected %s to return %d: got %d", tt.use, tt.code, cerr.ExitCode)
-						}
-					} else {
-						t.Errorf("Error running %s: %+v", tt.use, err)
+					if assert.Positive(t, tt.code, "Error running %s: %+v", tt.use, err) {
+						var cerr CommandError
+						require.ErrorAs(t, err, &cerr, "Expected %s to return pluginError: got %v(%T)", tt.use, err, err)
+						assert.Equalf(t, tt.code, cerr.ExitCode, "Expected %s to return %d: got %d", tt.use, tt.code, cerr.ExitCode)
 					}
 				}
 				assert.Equal(t, tt.expect, out.String(), "expected output for %q", tt.use)
@@ -218,16 +212,10 @@ func TestLoadPluginsWithSpace(t *testing.T) {
 		// tests until this is fixed
 		if runtime.GOOS != "windows" {
 			if err := pp.RunE(pp, tt.args); err != nil {
-				if tt.code > 0 {
-					cerr, ok := err.(CommandError)
-					if !ok {
-						t.Errorf("Expected %s to return pluginError: got %v(%T)", tt.use, err, err)
-					}
-					if cerr.ExitCode != tt.code {
-						t.Errorf("Expected %s to return %d: got %d", tt.use, tt.code, cerr.ExitCode)
-					}
-				} else {
-					t.Errorf("Error running %s: %+v", tt.use, err)
+				if assert.Positive(t, tt.code, "Error running %s: %+v", tt.use, err) {
+					var cerr CommandError
+					require.ErrorAs(t, err, &cerr, "Expected %s to return pluginError: got %v(%T)", tt.use, err, err)
+					assert.Equalf(t, tt.code, cerr.ExitCode, "Expected %s to return %d: got %d", tt.use, tt.code, cerr.ExitCode)
 				}
 			}
 			assert.Equal(t, tt.expect, out.String(), "expected output for %s", tt.use)

pkg/kube/client.go

@@ -249,7 +249,7 @@ func (c *Client) getKubeClient() (kubernetes.Interface, error) {
 // IsReachable tests connectivity to the cluster.
 func (c *Client) IsReachable() error {
 	client, err := c.getKubeClient()
-	if err == genericclioptions.ErrEmptyConfig {
+	if errors.Is(err, genericclioptions.ErrEmptyConfig) {
 		// re-replace kubernetes ErrEmptyConfig error with a friendly error
 		// moar workarounds for Kubernetes API breaking.
 		return errors.New("kubernetes cluster unreachable")
@@ -949,11 +949,12 @@ func (c *Client) Delete(resources ResourceList, policy metav1.DeletionPropagatio
 func isIncompatibleServerError(err error) bool {
 	// 415: Unsupported media type means we're talking to a server which doesn't
 	// support server-side apply.
-	if _, ok := err.(*apierrors.StatusError); !ok {
+	var sErr *apierrors.StatusError
+	if !errors.As(err, &sErr) {
 		// Non-StatusError means the error isn't because the server is incompatible.
 		return false
 	}
-	return err.(*apierrors.StatusError).Status().Code == http.StatusUnsupportedMediaType
+	return sErr.Status().Code == http.StatusUnsupportedMediaType
 }
 
 // isServerSideRetryable checks if an error encountered during server-side apply

pkg/provenance/sign_test.go

@@ -385,11 +385,9 @@ func TestVerify(t *testing.T) {
 	_, err = signer.Verify(archiveData, tamperedSigData, filepath.Base(testChartfile))
 	require.Errorf(t, err, "Expected %s to fail.", testTamperedSigBlock)
 
-	switch err.(type) {
-	case pgperrors.SignatureError:
-		t.Logf("Tampered sig block error: %s (%T)", err, err)
-	default:
-		t.Errorf("Expected invalid signature error, got %q (%T)", err, err)
+	var sErr pgperrors.SignatureError
+	if assert.ErrorAs(t, err, &sErr, "Expected invalid signature error, got %q (%T)", err, err) {
+		t.Logf("Tampered sig block error: %s (%T)", sErr, sErr)
 	}
 }
 

pkg/registry/transport.go

@@ -18,6 +18,7 @@ package registry
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"io"
 	"log/slog"
@@ -126,7 +127,7 @@ func logResponseBody(resp *http.Response) string {
 		Closer: body,
 	}
 	// read the body up to limit+1 to check if the body exceeds the limit
-	if _, err := io.CopyN(buf, body, payloadSizeLimit+1); err != nil && err != io.EOF {
+	if _, err := io.CopyN(buf, body, payloadSizeLimit+1); err != nil && !errors.Is(err, io.EOF) {
 		return fmt.Sprintf("   Error reading response body: %v", err)
 	}
 

pkg/repo/v1/index.go

@@ -421,8 +421,8 @@ func jsonOrYamlUnmarshal(b []byte, i any) error {
 // And repository indexes may be generated by older/non-compliant software, which doesn't
 // conform to all validations.
 func ignoreSkippableChartValidationError(err error) error {
-	verr, ok := err.(chart.ValidationError)
-	if !ok {
+	var verr chart.ValidationError
+	if !errors.As(err, &verr) {
 		return err
 	}
 

pkg/strvals/parser.go

@@ -238,16 +238,16 @@ func (t *parser) key(data map[string]any, nestedNameLevel int) (reterr error) {
 			// End of key. Consume =, Get value.
 			// FIXME: Get value list first
 			vl, e := t.valList()
-			switch e {
-			case nil:
+			switch {
+			case e == nil:
 				set(data, string(k), vl)
 				return nil
-			case io.EOF:
+			case errors.Is(e, io.EOF):
 				set(data, string(k), "")
 				return e
-			case ErrNotList:
+			case errors.Is(e, ErrNotList):
 				rs, e := t.val()
-				if e != nil && e != io.EOF {
+				if e != nil && !errors.Is(e, io.EOF) {
 					return e
 				}
 				v, e := t.reader(rs)
@@ -370,14 +370,14 @@ func (t *parser) listItem(list []any, i, nestedNameLevel int) ([]any, error) {
 			return list, err
 		}
 		vl, e := t.valList()
-		switch e {
-		case nil:
+		switch {
+		case e == nil:
 			return setIndex(list, i, vl)
-		case io.EOF:
+		case errors.Is(e, io.EOF):
 			return setIndex(list, i, "")
-		case ErrNotList:
+		case errors.Is(e, ErrNotList):
 			rs, e := t.val()
-			if e != nil && e != io.EOF {
+			if e != nil && !errors.Is(e, io.EOF) {
 				return list, e
 			}
 			v, e := t.reader(rs)
@@ -476,7 +476,7 @@ func (t *parser) valList() ([]any, error) {
 	for {
 		switch rs, last, err := runesUntil(t.sc, stop); {
 		case err != nil:
-			if err == io.EOF {
+			if errors.Is(err, io.EOF) {
 				err = errors.New("list must terminate with '}'")
 			}
 			return list, err