From a2b1a60d51223254bd9c2f08cf3f411eb9d9fc58 Mon Sep 17 00:00:00 2001 From: Matthieu MOREL Date: Sat, 18 Jul 2026 13:28:03 +0200 Subject: [PATCH] fix: enhance error handling and improve test assertions (#32352) Signed-off-by: Matthieu MOREL --- .golangci.yml | 4 +-- .../chart/v3/lint/rules/deprecations_test.go | 8 ++---- internal/plugin/runtime_subprocess.go | 7 +++-- internal/sympath/walk.go | 9 +++--- pkg/chart/v2/lint/rules/deprecations_test.go | 13 +++++---- pkg/chart/v2/lint/rules/template_test.go | 10 ++++--- pkg/chart/v2/util/dependencies_test.go | 10 +++---- pkg/cmd/plugin_test.go | 28 ++++++------------- pkg/kube/client.go | 7 +++-- pkg/provenance/sign_test.go | 8 ++---- pkg/registry/transport.go | 3 +- pkg/repo/v1/index.go | 4 +-- pkg/strvals/parser.go | 22 +++++++-------- 13 files changed, 63 insertions(+), 70 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 6727d3001..5b6d7cf8e 100644 --- a/.golangci.yml +++ b/.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 diff --git a/internal/chart/v3/lint/rules/deprecations_test.go b/internal/chart/v3/lint/rules/deprecations_test.go index a255abf50..55c454118 100644 --- a/internal/chart/v3/lint/rules/deprecations_test.go +++ b/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") } diff --git a/internal/plugin/runtime_subprocess.go b/internal/plugin/runtime_subprocess.go index b921587c1..2dbe220d9 100644 --- a/internal/plugin/runtime_subprocess.go +++ b/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), diff --git a/internal/sympath/walk.go b/internal/sympath/walk.go index 812bb68ce..b54b97ad4 100644 --- a/internal/sympath/walk.go +++ b/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 } } diff --git a/pkg/chart/v2/lint/rules/deprecations_test.go b/pkg/chart/v2/lint/rules/deprecations_test.go index 86643f1c9..fdeb47e91 100644 --- a/pkg/chart/v2/lint/rules/deprecations_test.go +++ b/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", diff --git a/pkg/chart/v2/lint/rules/template_test.go b/pkg/chart/v2/lint/rules/template_test.go index f0eb008b3..12eec7531 100644 --- a/pkg/chart/v2/lint/rules/template_test.go +++ b/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 diff --git a/pkg/chart/v2/util/dependencies_test.go b/pkg/chart/v2/util/dependencies_test.go index 90a8806ec..40a17ac3a 100644 --- a/pkg/chart/v2/util/dependencies_test.go +++ b/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 { diff --git a/pkg/cmd/plugin_test.go b/pkg/cmd/plugin_test.go index 476997110..5364423a0 100644 --- a/pkg/cmd/plugin_test.go +++ b/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) diff --git a/pkg/kube/client.go b/pkg/kube/client.go index e363fad61..1dfa88800 100644 --- a/pkg/kube/client.go +++ b/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 diff --git a/pkg/provenance/sign_test.go b/pkg/provenance/sign_test.go index 3f8e654f9..79b33a970 100644 --- a/pkg/provenance/sign_test.go +++ b/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) } } diff --git a/pkg/registry/transport.go b/pkg/registry/transport.go index beec43cdd..fa0103ac0 100644 --- a/pkg/registry/transport.go +++ b/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) } diff --git a/pkg/repo/v1/index.go b/pkg/repo/v1/index.go index 839a4cca9..1c1497c5e 100644 --- a/pkg/repo/v1/index.go +++ b/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 } diff --git a/pkg/strvals/parser.go b/pkg/strvals/parser.go index d74e46986..be3b1e71b 100644 --- a/pkg/strvals/parser.go +++ b/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