Repository: go-bdd/gobdd (go)

Issue:
Step failure output mangled
**Describe the bug**

If a step fails because the step function returned an error, the output is mangled with `fmt.SPrintf()` error output.

For example, say the step is `And 2 items posted`.  If the function returns `fmt.Errorf("expected %d items, received %d", n, actual)`, then the output would be

> gobdd.go:377: And %!(EXTRA string=2 items posted, *errors.errorString=expected 2 items posted, received 0) 

**Expected behavior**

Prints out the step and the error clearly formatted. Something like:

> gobdd.go:377: And 2 items posted: expected 2 items posted, received 0


Pull request:
Fix step failure message
Fixes #66

When a step failed, the step's Kind was being passed in as the format param. This wasn't a valid format and so an Printf error was being printed out resulting in a garbled error. Added a proper format param in front of the Kind. Also extracted the function that runs each step so it could be unit tested properly

Patch:
diff --git a/gobdd.go b/gobdd.go
index a7bc134..aef66d3 100644
--- a/gobdd.go
+++ b/gobdd.go
@@ -96,6 +96,7 @@ type TestingT interface {
 	Log(...interface{})
 	Fatal(...interface{})
 	Fatalf(string, ...interface{})
+	Errorf(string, ...interface{})
 	Parallel()
 	Fail()
 	Run(name string, f func(t *testing.T)) bool
@@ -352,38 +353,42 @@ func (s *Suite) runStep(ctx context.Context, t *testing.T, step *messages.Gherki
 
 	params := def.expr.FindSubmatch([]byte(step.Text))[1:]
 	t.Run(step.Text, func(t *testing.T) {
-		defer func() {
-			if r := recover(); r != nil {
-				t.Errorf("%+v", r)
-			}
-		}()
+		ctx = def.run(ctx, t, step, params)
+	})
 
-		d := reflect.ValueOf(def.f)
-		in := []reflect.Value{reflect.ValueOf(ctx)}
-		if len(params)+1 != d.Type().NumIn() {
-			t.Errorf("the step function %s accepts %d arguments but %d received", d.String(), d.Type().NumIn(), len(params)+1)
-			return
-		}
-		for i, v := range params {
-			inType := d.Type().In(i + 1)
-			paramType := s.paramType(v, inType)
-			in = append(in, paramType)
-		}
-		retValues := d.Call(in)
-		v := retValues[1]
+	return ctx
+}
 
-		if !v.IsNil() {
-			err = v.Interface().(error)
-			t.Errorf(step.Keyword, step.Text, err)
+func (def *stepDef) run(ctx context.Context, t TestingT, step *messages.GherkinDocument_Feature_Step, params [][]byte) context.Context {
+	defer func() {
+		if r := recover(); r != nil {
+			t.Errorf("%+v", r)
 		}
+	}()
 
-		ctx = retValues[0].Interface().(context.Context)
-	})
+	d := reflect.ValueOf(def.f)
+	in := []reflect.Value{reflect.ValueOf(ctx)}
+	if len(params)+1 != d.Type().NumIn() {
+		t.Errorf("the step function %s accepts %d arguments but %d received", d.String(), d.Type().NumIn(), len(params)+1)
+		return ctx
+	}
+	for i, v := range params {
+		inType := d.Type().In(i + 1)
+		paramType := paramType(v, inType)
+		in = append(in, paramType)
+	}
+	retValues := d.Call(in)
+	v := retValues[1]
 
-	return ctx
+	if !v.IsNil() {
+		err := v.Interface().(error)
+		t.Errorf("%s %s: %v", step.Keyword, step.Text, err)
+	}
+
+	return retValues[0].Interface().(context.Context)
 }
 
-func (s *Suite) paramType(param []byte, inType reflect.Type) reflect.Value {
+func paramType(param []byte, inType reflect.Type) reflect.Value {
 	paramType := reflect.ValueOf(param)
 	if inType.Kind() == reflect.String {
 		paramType = reflect.ValueOf(string(paramType.Interface().([]uint8)))


Test patch:
diff --git a/gobdd_test.go b/gobdd_test.go
index aa4aadb..a6fc30d 100644
--- a/gobdd_test.go
+++ b/gobdd_test.go
@@ -2,8 +2,10 @@ package gobdd
 
 import (
 	"errors"
+	"fmt"
 	"testing"
 
+	"github.com/cucumber/messages-go/v9"
 	"github.com/go-bdd/assert"
 	"github.com/go-bdd/gobdd/context"
 )
@@ -83,6 +85,32 @@ func TestInvalidFunctionSignature(t *testing.T) {
 	}
 }
 
+func TestFailureOutput(t *testing.T) {
+	testCases := []struct {
+		name           string
+		f              interface{}
+		expectedErrors []string
+	}{
+		{name: "passes", f: pass, expectedErrors: nil},
+		{name: "returns error", f: failure, expectedErrors: []string{"Test text: the step failed"}},
+		{name: "step panics", f: panics, expectedErrors: []string{"the step panicked"}},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			def := stepDef{f: testCase.f}
+			step := &messages.GherkinDocument_Feature_Step{Keyword: "Test", Text: "text"}
+
+			tester := &mockTester{}
+			def.run(context.New(), tester, step, nil)
+			err := assert.Equals(testCase.expectedErrors, tester.errors)
+			if err != nil {
+				t.Fatal(err)
+			}
+		})
+	}
+}
+
 func addf(ctx context.Context, var1, var2 float32) (context.Context, error) {
 	res := var1 + var2
 	ctx.Set("sumRes", res)
@@ -119,30 +147,43 @@ func fail(ctx context.Context) (context.Context, error) {
 	return ctx, errors.New("the step should never be executed")
 }
 
+func failure(ctx context.Context) (context.Context, error) {
+	return ctx, errors.New("the step failed")
+}
+
+func panics(_ context.Context) (context.Context, error) {
+	panic(errors.New("the step panicked"))
+}
+
 func pass(ctx context.Context) (context.Context, error) {
 	return ctx, nil
 }
 
 type mockTester struct {
 	fatalCalled int
+	errors      []string
 }
 
-func (m mockTester) Log(...interface{}) {
+func (m *mockTester) Log(...interface{}) {
 }
 
 func (m *mockTester) Fatal(...interface{}) {
 	m.fatalCalled++
 }
 
-func (m mockTester) Fatalf(string, ...interface{}) {
+func (m *mockTester) Fatalf(string, ...interface{}) {
+}
+
+func (m *mockTester) Errorf(format string, a ...interface{}) {
+	m.errors = append(m.errors, fmt.Sprintf(format, a...))
 }
 
-func (m mockTester) Parallel() {
+func (m *mockTester) Parallel() {
 }
 
-func (m mockTester) Fail() {
+func (m *mockTester) Fail() {
 }
 
-func (m mockTester) Run(name string, f func(t *testing.T)) bool {
+func (m *mockTester) Run(_ string, _ func(t *testing.T)) bool {
 	return true
 }