Merge pull request #8 from secureworks/phughes-fix-action

Fix GitHub workflow and revert Errorf behavior for 1 wrapped error

.github/workflows/cicd.yaml

@@ -0,0 +1,39 @@
+name: cicd
+on:
+  pull_request:
+  push:
+    branches:
+      - main
+
+permissions:
+  contents: read
+  checks: write
+
+jobs:
+  build:
+    name: test
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-go@v5
+        with:
+          go-version: ^1.20
+          cache: false
+      - name: build
+        run: go build -v ./...
+      - name: test
+        run: go test -v ./...
+  lint:
+    name: lint
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: actions/setup-go@v5
+        with:
+          go-version: ^1.20
+          cache: false
+      - name: golangci-lint
+        uses: golangci/golangci-lint-action@v6
+        with:
+          version: v1.58
+          args: --config=.golangci.yaml

errors.go

@@ -233,6 +233,9 @@ func (w *withFrames) Format(s fmt.State, verb rune) {
 // information possible without returning a confusing frame set.
 // Therefore, try not to mix the WithFrame and WithStackTrace patterns
 // in a single error chain.
+//
+// FramesFrom will not traverse a multierror, since there is no sensible
+// way to structure the returned frames.
 func FramesFrom(err error) (ff Frames) {
 	var traceFound bool
 	for err != nil {

errors_test.go

@@ -108,7 +108,7 @@ var (
 	withFrameFuncM  = "^github\\.com/secureworks/errors.withFrameCaller$"
 	withStackFuncM  = "^github\\.com/secureworks/errors.withStackTraceCaller$"
 	withWrapFuncM   = "^github\\.com/secureworks/errors.wrapCaller$"
-	errorTestAnonM  = func(fnName string) string { return fmt.Sprintf(`^%s\.glob\.\.func%s$`, errorsTestPkgM, fnName) }
+	errorTestAnonM  = func(fnName string) string { return fmt.Sprintf(`^%s\..*\.func%s$`, errorsTestPkgM, fnName) }
 	errorTestFileM  = func(line string) string { return fmt.Sprintf("^\t.+%s:%s$", errorsTestFilM, line) }
 
 	framesChainM = []string{
@@ -162,10 +162,6 @@ func nilError() error {
 	return nil
 }
 
-type errorType struct{}
-
-func (e errorType) Error() string { return "i'm an error" }
-
 var (
 	stackFramerIface = reflect.TypeOf((*interface {
 		Frames() Frames
@@ -274,7 +270,7 @@ func TestErrorFrames(t *testing.T) {
 				skip: 0,
 				frameMatchers: []string{
 					"",
-					errorsTestPkgM + `\.TestErrorFrames\.func3\.1\.1$`,
+					errorsTestPkgM + `\.TestErrorFrames.*\.func3\.1\.1$`,
 					errorTestFileM(`\d+`), // Offsets based on the anon func above.
 				},
 			},
@@ -290,15 +286,15 @@ func TestErrorFrames(t *testing.T) {
 				skip: 2,
 				frameMatchers: []string{
 					"",
-					errorsTestPkgM + `\.TestErrorFrames\.func3\.1$`,
+					errorsTestPkgM + `\.TestErrorFrames.*\.func3\.1$`,
 					errorTestFileM(`\d+`), // Offsets based on the anon func above.
 				},
 			},
 			{
 				skip: 3,
 				frameMatchers: []string{
 					"",
-					errorsTestPkgM + `\.TestErrorFrames\.func3\.2$`,
+					errorsTestPkgM + `\.TestErrorFrames.*\.func3\.2$`,
 					errorTestFileM(`\d+`), // Offsets based on the anon func above.
 				},
 			},
@@ -482,6 +478,31 @@ func TestFramesFrom(t *testing.T) {
 			expected,
 		)
 	})
+
+	t.Run("when called on a multierror", func(t *testing.T) {
+		errChain := Errorf("wrap: %w: context: %w", framesChainError(), framesChainError())
+
+		// None on the multierror.
+		ff := FramesFrom(errChain)
+		testutils.AssertEqual(t, 0, len(ff))
+
+		// All on the wrapped errors.
+		for _, err := range ErrorsFrom(errChain) {
+			fff := FramesFrom(err)
+			testutils.AssertLinesMatch(t,
+				fff,
+				"%+v",
+				append(
+					framesChainM,
+					[]string{
+						// From the call to Errorf.
+						"^github.com/secureworks/errors\\.TestFramesFrom.func5$",
+						errorTestFileM(`\d+`),
+					}...,
+				),
+			)
+		}
+	})
 }
 
 func TestErrorFormat(t *testing.T) {
@@ -511,17 +532,18 @@ func TestErrorFormat(t *testing.T) {
 				},
 			},
 			{
-				// Test that subsequent withFrames do not print frames recursively.
+				// Test that subsequent withFrames do not print frames recursively, but
+				// serially!
 				format: "%+v",
 				error:  errChain,
 				expect: []string{
-					"err",
+					"wrap: wrap: err",
 					"^github.com/secureworks/errors.TestErrorFormat$",
-					errorTestFileM(`488`),
+					errorTestFileM(`509`),
 					"^github.com/secureworks/errors.TestErrorFormat$",
-					errorTestFileM(`489`),
+					errorTestFileM(`510`),
 					"^github.com/secureworks/errors.TestErrorFormat$",
-					errorTestFileM(`490`),
+					errorTestFileM(`511`),
 				},
 			},
 		}
@@ -563,7 +585,7 @@ func TestErrorFormat(t *testing.T) {
 				expect: []string{
 					"err",
 					"^github.com/secureworks/errors.TestErrorFormat$",
-					errorTestFileM(`491`),
+					errorTestFileM(`512`),
 					`^testing\.tRunner$`,
 					`^.+/testing/testing.go:\d+$`,
 				},

examples_test.go

@@ -540,7 +540,7 @@ func ExampleMultiError_ErrorOrNil() {
 }
 
 func ExampleMultiError_as() {
-	err1 := fmt.Errorf("context: %w",
+	err1 := errors.Errorf("context: %w",
 		&unknownErrorType{error: errors.New("err"), SecretValue: "secret A"})
 
 	fmt.Println()
@@ -554,7 +554,7 @@ func ExampleMultiError_as() {
 
 	// MultiError implements As by iterating over each error in order,
 	// unwrapping the contained values.
-	err2 := fmt.Errorf("outer context: %w", errors.NewMultiError(
+	err2 := errors.Errorf("outer context: %w", errors.NewMultiError(
 		errors.New("err"),
 		err1,
 		// Last in order, so not reached.
@@ -588,15 +588,15 @@ func ExampleMultiError_as() {
 
 func ExampleMultiError_is() {
 	errSentinel := errors.New("sentinel err")
-	errA := fmt.Errorf("ctx A: %w", errSentinel)
-	errB := fmt.Errorf("ctx B: %w", errors.New("err"))
-	errC := fmt.Errorf("ctx C: %w", errSentinel)
+	errA := errors.Errorf("ctx A: %w", errSentinel)
+	errB := errors.Errorf("ctx B: %w", errors.New("err"))
+	errC := errors.Errorf("ctx C: %w", errSentinel)
 
 	fmt.Println()
 
 	// MultiError implements Is by iterating over each error in order,
 	// unwrapping the contained values.
-	err := fmt.Errorf("outer context: %w", errors.NewMultiError(
+	err := errors.Errorf("outer context: %w", errors.NewMultiError(
 		errA,
 		errB,
 		errC,
@@ -692,8 +692,8 @@ func ExampleErrorsFrom() {
 
 func ExampleErrorsFrom_singleError() {
 	err := errors.New("err")
-	err = fmt.Errorf("inner context: %w", err)
-	err = fmt.Errorf("outer context: %w", err)
+	err = errors.Errorf("inner context: %w", err)
+	err = errors.Errorf("outer context: %w", err)
 
 	errs := errors.ErrorsFrom(err)
 	for _, err := range errs { // errs contains the given error.

formatter.go

@@ -32,21 +32,41 @@ func Errorf(format string, values ...interface{}) error {
 		return errors.New(`%!e(errors.Errorf=failed: ` + err.Error() + `)`)
 	}
 
-	// Interpose and wrap errors with framer if the associated verb is `%w`.
+	var numWrapped int
+	for _, v := range verbs {
+		if v.letter != 'w' {
+			continue
+		}
+		numWrapped++
+	}
+
+	// Allow no %w verbs, and when 0 or 1, then we wrap the created error
+	// with a frame. This allows the received error to handle %+v formatting
+	// correctly.
+	if numWrapped <= 1 {
+		return &withFrames{
+			error:  fmt.Errorf(format, values...),
+			frames: frames{getFrame(3)},
+		}
+	}
+
+	// Interpose and wrap errors with framer if the associated verb is `%w`,
+	// when there is more than one value, creating a multierror where each
+	// error has a reference to the frame.
 	for _, v := range verbs {
 		if v.letter != 'w' {
 			continue
 		}
 		if wrappedErr, ok := values[v.idx].(error); ok {
 			if wrappedErr != nil {
+				numWrapped++
 				values[v.idx] = &withFrames{
 					error:  wrappedErr,
 					frames: frames{getFrame(3)},
 				}
 			}
 		}
 	}
-
 	return fmt.Errorf(format, values...)
 }
 

formatter_test.go

@@ -68,7 +68,7 @@ func TestErrorf_multiError(t *testing.T) {
 
 func Benchmark_parseVerb(b *testing.B) {
 	for i := 0; i < b.N; i++ {
-		parseVerb("%[3]*.[2]*[1]f")
+		parseVerb("%[3]*.[2]*[1]f") //nolint:errcheck
 	}
 }
 

frames_test.go

@@ -277,7 +277,7 @@ func TestFramesFormat(t *testing.T) {
 			"default %n",
 			rtimeStack,
 			"%n",
-			`^\[stackCaller init doInit1 doInit main\]$`,
+			`^\[stackCaller init doInit.? doInit main\]$`,
 		},
 
 		{"empty %v", nil, "%v", `\[\]`},
@@ -302,7 +302,7 @@ github\.com/secureworks/errors.stackCaller
 	.+/frames_test.go:29
 github\.com/secureworks/errors.init
 	.+/frames_test.go:46
-runtime\.doInit1
+runtime\.doInit.?
 	.+/runtime/proc.go:\d+
 runtime\.doInit
 	.+/runtime/proc.go:\d+