Fix some type checks on the signatures of nested step handlers (#647)

* at some point someone changed the return type for nested steps from []string to godog.Steps but they forgot to adjust the type checks. The existing type checks were lax and unable to distinguish  []string from godog.Steps but in a couple of places in the code the value is coerced to godog.Steps and so if someone returned []string then the code would blow up. Additionally there were some tests aroudn these types but they also had not been updated but the test was passing for the wrong reason - the particular test expected an error but the cause of the error wasn't the one the code expected.

* CHANGELOG.md

* use chatgpt to regen the top of the code based on the new tests

* use chatgpt to regen the top of the code based on the new tests

* corrected the error messages of the param checks to indicate that the problem is the function signature and not the args being passed to the function, also added numerous extra assertions on the precise error messages returned. Now that the precise error is being verified in the test I have improved certain error messages to that more accurate detail is included in the errors

* added further constraints to the step arg mapping tests

* removed redundant test

* include a step error result in the reported error even when the ctx is nil

CHANGELOG.md

@@ -8,6 +8,7 @@ This document is formatted according to the principles of [Keep A CHANGELOG](htt
 
 ## Unreleased
 
+- Improved the type checking of step return types and improved the error messages - ([647](https://github.com/cucumber/godog/pull/647) - [johnlon](https://github.com/johnlon))
 - Ambiguous step definitions will now be detected when strict mode is activated - ([636](https://github.com/cucumber/godog/pull/636) - [johnlon](https://github.com/johnlon))
 - Provide support for attachments / embeddings including a new example in the examples dir - ([623](https://github.com/cucumber/godog/pull/623) - [johnlon](https://github.com/johnlon))
 

internal/formatters/fmt_output_test.go

@@ -179,9 +179,6 @@ func fmtOutputTest(fmtName, testName, featureFilePath string) func(*testing.T) {
 		expected := normalise(string(expectedOutput))
 		actual := normalise(buf.String())
 		assert.Equalf(t, expected, actual, "path: %s", expectOutputPath)
-		if expected != actual {
-			println("diff")
-		}
 	}
 }
 

internal/models/stepdef.go

@@ -16,9 +16,9 @@ var typeOfBytes = reflect.TypeOf([]byte(nil))
 
 // matchable errors
 var (
-	ErrUnmatchedStepArgumentNumber = errors.New("func received more arguments than expected")
+	ErrUnmatchedStepArgumentNumber = errors.New("func expected more arguments than given")
 	ErrCannotConvert               = errors.New("cannot convert argument")
-	ErrUnsupportedArgumentType     = errors.New("unsupported argument type")
+	ErrUnsupportedParameterType    = errors.New("func has unsupported parameter type")
 )
 
 // StepDefinition ...
@@ -36,6 +36,9 @@ type StepDefinition struct {
 var typeOfContext = reflect.TypeOf((*context.Context)(nil)).Elem()
 
 // Run a step with the matched arguments using reflect
+// Returns one of ...
+// (context, error)
+// (context, godog.Steps)
 func (sd *StepDefinition) Run(ctx context.Context) (context.Context, interface{}) {
 	var values []reflect.Value
 
@@ -161,7 +164,8 @@ func (sd *StepDefinition) Run(ctx context.Context) (context.Context, interface{}
 
 				return ctx, fmt.Errorf(`%w %d: "%v" of type "%T" to *messages.PickleTable`, ErrCannotConvert, i, arg, arg)
 			default:
-				return ctx, fmt.Errorf("%w: the argument %d type %T is not supported %s", ErrUnsupportedArgumentType, i, arg, param.Elem().String())
+				// the error here is that the declared function has an unsupported param type - really this ought to be trapped at registration ti,e
+				return ctx, fmt.Errorf("%w: the data type of parameter %d type *%s is not supported", ErrUnsupportedParameterType, i, param.Elem().String())
 			}
 		case reflect.Slice:
 			switch param {
@@ -172,10 +176,13 @@ func (sd *StepDefinition) Run(ctx context.Context) (context.Context, interface{}
 				}
 				values = append(values, reflect.ValueOf([]byte(s)))
 			default:
-				return ctx, fmt.Errorf("%w: the slice argument %d type %s is not supported", ErrUnsupportedArgumentType, i, param.Kind())
+				// the problem is the function decl is not using a support slice type as the param
+				return ctx, fmt.Errorf("%w: the slice parameter %d type []%s is not supported", ErrUnsupportedParameterType, i, param.Elem().Kind())
 			}
+		case reflect.Struct:
+			return ctx, fmt.Errorf("%w: the struct parameter %d type %s is not supported", ErrUnsupportedParameterType, i, param.String())
 		default:
-			return ctx, fmt.Errorf("%w: the argument %d type %s is not supported", ErrUnsupportedArgumentType, i, param.Kind())
+			return ctx, fmt.Errorf("%w: the parameter %d type %s is not supported", ErrUnsupportedParameterType, i, param.Kind())
 		}
 	}
 
@@ -184,17 +191,43 @@ func (sd *StepDefinition) Run(ctx context.Context) (context.Context, interface{}
 		return ctx, nil
 	}
 
-	if len(res) == 1 {
-		r := res[0].Interface()
+	// Note that the step fn return types were validated at Initialise in test_context.go stepWithKeyword()
 
-		if ctx, ok := r.(context.Context); ok {
+	// single return value may be one of ...
+	// error
+	// context.Context
+	// godog.Steps
+	result0 := res[0].Interface()
+	if len(res) == 1 {
+
+		// if the single return value is a context then just return it
+		if ctx, ok := result0.(context.Context); ok {
 			return ctx, nil
 		}
 
-		return ctx, res[0].Interface()
+		// return type is presumably one of nil, "error" or "Steps" so place it into second return position
+		return ctx, result0
 	}
 
-	return res[0].Interface().(context.Context), res[1].Interface()
+	// multi-value value return must be
+	//  (context, error) and the context value must not be nil
+	if ctx, ok := result0.(context.Context); ok {
+		return ctx, res[1].Interface()
+	}
+
+	result1 := res[1].Interface()
+	errMsg := ""
+	if result1 != nil {
+		errMsg = fmt.Sprintf(", step def also returned an error: %v", result1)
+	}
+
+	text := sd.StepDefinition.Expr.String()
+
+	if result0 == nil {
+		panic(fmt.Sprintf("step definition '%v' with return type (context.Context, error) must not return <nil> for the context.Context value%s", text, errMsg))
+	}
+
+	panic(fmt.Errorf("step definition '%v' has return type (context.Context, error), but found %v rather than a context.Context value%s", text, result0, errMsg))
 }
 
 func (sd *StepDefinition) shouldBeString(idx int) (string, error) {

internal/models/stepdef_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
+	"regexp"
 	"strings"
 	"testing"
 
@@ -18,10 +19,91 @@ import (
 
 type ctxKey string
 
-func TestShouldSupportContext(t *testing.T) {
+func TestShouldSupportVoidHandlerReturn(t *testing.T) {
+	wasCalled := false
+	initialCtx := context.WithValue(context.Background(), ctxKey("original"), 123)
+
+	fn := func(ctx context.Context) {
+		wasCalled = true
+		assert.Equal(t, 123, ctx.Value(ctxKey("original")))
+	}
+
+	def := &models.StepDefinition{
+		StepDefinition: formatters.StepDefinition{
+			Handler: fn,
+		},
+		HandlerValue: reflect.ValueOf(fn),
+	}
+
+	def.Args = []interface{}{}
+
+	ctx, err := def.Run(initialCtx)
+	assert.True(t, wasCalled)
+	// ctx is passed thru
+	assert.Equal(t, initialCtx, ctx)
+	assert.Nil(t, err)
+
+}
+
+func TestShouldSupportNilContextReturn(t *testing.T) {
+	initialCtx := context.WithValue(context.Background(), ctxKey("original"), 123)
+
+	wasCalled := false
+	fn := func(ctx context.Context) context.Context {
+		wasCalled = true
+		assert.Equal(t, 123, ctx.Value(ctxKey("original")))
+
+		// nil context is permitted if is single return value
+		return nil
+	}
+
+	def := &models.StepDefinition{
+		StepDefinition: formatters.StepDefinition{
+			Handler: fn,
+		},
+		HandlerValue: reflect.ValueOf(fn),
+	}
+
+	def.Args = []interface{}{}
+	ctx, err := def.Run(initialCtx)
+	assert.True(t, wasCalled)
+	// original context is substituted for a nil return value
+	//  << JL : IS THIS A BUG? TWO ARG API DOESN'T ALLOW THIS
+	assert.Equal(t, initialCtx, ctx)
+	assert.Nil(t, err)
+}
+
+func TestShouldSupportNilErrorReturn(t *testing.T) {
+	initialCtx := context.WithValue(context.Background(), ctxKey("original"), 123)
+
+	wasCalled := false
+	fn := func(ctx context.Context) error {
+		wasCalled = true
+		assert.Equal(t, 123, ctx.Value(ctxKey("original")))
+
+		// nil error is permitted
+		return nil
+	}
+
+	def := &models.StepDefinition{
+		StepDefinition: formatters.StepDefinition{
+			Handler: fn,
+		},
+		HandlerValue: reflect.ValueOf(fn),
+	}
+
+	def.Args = []interface{}{}
+	ctx, err := def.Run(initialCtx)
+	assert.True(t, wasCalled)
+	// original context is passed thru if method doesn't return context.
+	assert.Equal(t, initialCtx, ctx)
+	assert.Nil(t, err)
+}
+
+func TestShouldSupportContextReturn(t *testing.T) {
 	ctx := context.WithValue(context.Background(), ctxKey("original"), 123)
 
-	fn := func(ctx context.Context, a int64, b int32, c int16, d int8) context.Context {
+	fn := func(ctx context.Context) context.Context {
 		assert.Equal(t, 123, ctx.Value(ctxKey("original")))
 
 		return context.WithValue(ctx, ctxKey("updated"), 321)
@@ -34,18 +116,69 @@ func TestShouldSupportContext(t *testing.T) {
 		HandlerValue: reflect.ValueOf(fn),
 	}
 
-	def.Args = []interface{}{"1", "1", "1", "1"}
+	def.Args = []interface{}{}
 	ctx, err := def.Run(ctx)
 	assert.Nil(t, err)
+	// converys the context
 	assert.Equal(t, 123, ctx.Value(ctxKey("original")))
 	assert.Equal(t, 321, ctx.Value(ctxKey("updated")))
 }
 
-func TestShouldSupportContextAndError(t *testing.T) {
+func TestShouldSupportErrorReturn(t *testing.T) {
+	ctx := context.WithValue(context.Background(), ctxKey("original"), 123)
+	expectedErr := fmt.Errorf("expected error")
+
+	fn := func(ctx context.Context) error {
+		assert.Equal(t, 123, ctx.Value(ctxKey("original")))
+
+		return expectedErr
+	}
+
+	def := &models.StepDefinition{
+		StepDefinition: formatters.StepDefinition{
+			Handler: fn,
+		},
+		HandlerValue: reflect.ValueOf(fn),
+	}
+
+	def.Args = []interface{}{}
+	ctx, err := def.Run(ctx)
+	// conveys the returned error
+	assert.Equal(t, expectedErr, err)
+	assert.Equal(t, 123, ctx.Value(ctxKey("original")))
+}
+
+func TestShouldSupportContextAndErrorReturn(t *testing.T) {
+
+	ctx := context.WithValue(context.Background(), ctxKey("original"), 123)
+	expectedErr := fmt.Errorf("expected error")
+
+	fn := func(ctx context.Context) (context.Context, error) {
+		assert.Equal(t, 123, ctx.Value(ctxKey("original")))
+
+		return context.WithValue(ctx, ctxKey("updated"), 321), expectedErr
+	}
+
+	def := &models.StepDefinition{
+		StepDefinition: formatters.StepDefinition{
+			Handler: fn,
+		},
+		HandlerValue: reflect.ValueOf(fn),
+	}
+
+	def.Args = []interface{}{}
+	ctx, err := def.Run(ctx)
+	// conveys error and context
+	assert.Equal(t, expectedErr, err)
+	assert.Equal(t, 123, ctx.Value(ctxKey("original")))
+	assert.Equal(t, 321, ctx.Value(ctxKey("updated")))
+}
+
+func TestShouldSupportContextAndNilErrorReturn(t *testing.T) {
 
 	ctx := context.WithValue(context.Background(), ctxKey("original"), 123)
 
-	fn := func(ctx context.Context, a int64, b int32, c int16, d int8) (context.Context, error) {
+	fn := func(ctx context.Context) (context.Context, error) {
 		assert.Equal(t, 123, ctx.Value(ctxKey("original")))
 
 		return context.WithValue(ctx, ctxKey("updated"), 321), nil
@@ -58,146 +191,54 @@ func TestShouldSupportContextAndError(t *testing.T) {
 		HandlerValue: reflect.ValueOf(fn),
 	}
 
-	def.Args = []interface{}{"1", "1", "1", "1"}
+	def.Args = []interface{}{}
 	ctx, err := def.Run(ctx)
+	// conveys nil error and context
 	assert.Nil(t, err)
 	assert.Equal(t, 123, ctx.Value(ctxKey("original")))
 	assert.Equal(t, 321, ctx.Value(ctxKey("updated")))
 }
 
-func TestShouldSupportEmptyHandlerReturn(t *testing.T) {
-	fn := func(a int64, b int32, c int16, d int8) {}
+func TestShouldRejectNilContextWhenMultiValueReturn(t *testing.T) {
+
+	ctx := context.WithValue(context.Background(), ctxKey("original"), 123)
+
+	fn := func(ctx context.Context) (context.Context, error) {
+		assert.Equal(t, 123, ctx.Value(ctxKey("original")))
+
+		// nil context is illegal.
+		return nil, fmt.Errorf("expected error")
+	}
 
 	def := &models.StepDefinition{
 		StepDefinition: formatters.StepDefinition{
 			Handler: fn,
+			Expr:    regexp.MustCompile("some regex string"),
 		},
 		HandlerValue: reflect.ValueOf(fn),
 	}
 
-	def.Args = []interface{}{"1", "1", "1", "1"}
-	if _, err := def.Run(context.Background()); err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
+	def.Args = []interface{}{}
 
-	def.Args = []interface{}{"1", "1", "1", strings.Repeat("1", 9)}
-	if _, err := def.Run(context.Background()); err == nil {
-		t.Fatalf("expected convertion fail for int8, but got none")
-	}
+	defer func() {
+		if e := recover(); e != nil {
+			pe := e.(string)
+			assert.Equal(t, "step definition 'some regex string' with return type (context.Context, error) must not return <nil> for the context.Context value, step def also returned an error: expected error", pe)
+		}
+	}()
+
+	def.Run(ctx)
+
+	assert.Fail(t, "should not get here")
 }
 
-func TestShouldSupportIntTypes(t *testing.T) {
-	fn := func(a int64, b int32, c int16, d int8) error { return nil }
+func TestArgumentCountChecks(t *testing.T) {
 
-	def := &models.StepDefinition{
-		StepDefinition: formatters.StepDefinition{
-			Handler: fn,
-		},
-		HandlerValue: reflect.ValueOf(fn),
+	wasCalled := false
+	fn := func(a int, b int) {
+		wasCalled = true
 	}
 
-	def.Args = []interface{}{"1", "1", "1", "1"}
-	if _, err := def.Run(context.Background()); err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-
-	def.Args = []interface{}{"1", "1", "1", strings.Repeat("1", 9)}
-	if _, err := def.Run(context.Background()); err == nil {
-		t.Fatalf("expected convertion fail for int8, but got none")
-	}
-}
-
-func TestShouldSupportFloatTypes(t *testing.T) {
-	fn := func(a float64, b float32) error { return nil }
-
-	def := &models.StepDefinition{
-		StepDefinition: formatters.StepDefinition{
-			Handler: fn,
-		},
-		HandlerValue: reflect.ValueOf(fn),
-	}
-
-	def.Args = []interface{}{"1.1", "1.09"}
-	if _, err := def.Run(context.Background()); err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-
-	def.Args = []interface{}{"1.08", strings.Repeat("1", 65) + ".67"}
-	if _, err := def.Run(context.Background()); err == nil {
-		t.Fatalf("expected convertion fail for float32, but got none")
-	}
-}
-
-func TestShouldNotSupportOtherPointerTypesThanGherkin(t *testing.T) {
-	fn1 := func(a *int) error { return nil }
-	fn2 := func(a *messages.PickleDocString) error { return nil }
-	fn3 := func(a *messages.PickleTable) error { return nil }
-
-	def1 := &models.StepDefinition{
-		StepDefinition: formatters.StepDefinition{
-			Handler: fn1,
-		},
-		HandlerValue: reflect.ValueOf(fn1),
-		Args:         []interface{}{(*int)(nil)},
-	}
-	def2 := &models.StepDefinition{
-		StepDefinition: formatters.StepDefinition{
-			Handler: fn2,
-		},
-		HandlerValue: reflect.ValueOf(fn2),
-		Args:         []interface{}{&messages.PickleDocString{}},
-	}
-	def3 := &models.StepDefinition{
-		StepDefinition: formatters.StepDefinition{
-			Handler: fn3,
-		},
-		HandlerValue: reflect.ValueOf(fn3),
-		Args:         []interface{}{(*messages.PickleTable)(nil)},
-	}
-
-	if _, err := def1.Run(context.Background()); err == nil {
-		t.Fatalf("expected conversion error, but got none")
-	}
-
-	if _, err := def2.Run(context.Background()); err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-
-	if _, err := def3.Run(context.Background()); err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-}
-
-func TestShouldSupportOnlyByteSlice(t *testing.T) {
-	fn1 := func(a []byte) error { return nil }
-	fn2 := func(a []string) error { return nil }
-
-	def1 := &models.StepDefinition{
-		StepDefinition: formatters.StepDefinition{
-			Handler: fn1,
-		},
-		HandlerValue: reflect.ValueOf(fn1),
-		Args:         []interface{}{"str"},
-	}
-	def2 := &models.StepDefinition{
-		StepDefinition: formatters.StepDefinition{
-			Handler: fn2,
-		},
-		HandlerValue: reflect.ValueOf(fn2),
-		Args:         []interface{}{[]string{}},
-	}
-
-	if _, err := def1.Run(context.Background()); err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-
-	if _, err := def2.Run(context.Background()); err == nil {
-		t.Fatalf("expected conversion error, but got none")
-	}
-}
-
-func TestUnexpectedArguments(t *testing.T) {
-	fn := func(a, b int) error { return nil }
 	def := &models.StepDefinition{
 		StepDefinition: formatters.StepDefinition{
 			Handler: fn,
@@ -206,24 +247,168 @@ func TestUnexpectedArguments(t *testing.T) {
 	}
 
 	def.Args = []interface{}{"1"}
+	_, err := def.Run(context.Background())
+	assert.False(t, wasCalled)
+	assert.Equal(t, `func expected more arguments than given: expected 2 arguments, matched 1 from step`, err.(error).Error())
+	assert.True(t, errors.Is(err.(error), models.ErrUnmatchedStepArgumentNumber))
 
-	_, res := def.Run(context.Background())
-	if res == nil {
-		t.Fatalf("expected an error due to wrong number of arguments, but got none")
-	}
-
-	err, ok := res.(error)
-	if !ok {
-		t.Fatalf("expected an error due to wrong number of arguments, but got %T instead", res)
-	}
-
-	if !errors.Is(err, models.ErrUnmatchedStepArgumentNumber) {
-		t.Fatalf("expected an error due to wrong number of arguments, but got %v instead", err)
-	}
+	// FIXME - extra args are ignored - but should be reported at runtime
+	def.Args = []interface{}{"1", "2", "IGNORED-EXTRA-ARG"}
+	_, err = def.Run(context.Background())
+	assert.True(t, wasCalled)
+	assert.Nil(t, err)
 }
 
-func TestStepDefinition_Run_StepShouldBeString(t *testing.T) {
-	test := func(t *testing.T, fn interface{}) {
+func TestShouldSupportIntTypes(t *testing.T) {
+	var aActual int64
+	var bActual int32
+	var cActual int16
+	var dActual int8
+
+	fn := func(a int64, b int32, c int16, d int8) {
+		aActual = a
+		bActual = b
+		cActual = c
+		dActual = d
+	}
+
+	def := &models.StepDefinition{
+		StepDefinition: formatters.StepDefinition{
+			Handler: fn,
+		},
+		HandlerValue: reflect.ValueOf(fn),
+	}
+
+	def.Args = []interface{}{"1", "2", "3", "4"}
+	_, err := def.Run(context.Background())
+	assert.Nil(t, err)
+	assert.Equal(t, int64(1), aActual)
+	assert.Equal(t, int32(2), bActual)
+	assert.Equal(t, int16(3), cActual)
+	assert.Equal(t, int8(4), dActual)
+
+	// 128 doesn't fit in signed 8bit int
+	def.Args = []interface{}{"1", "2", "3", "128"}
+	_, err = def.Run(context.Background())
+	assert.Equal(t, `cannot convert argument 3: "128" to int8: strconv.ParseInt: parsing "128": value out of range`, err.(error).Error())
+
+	def.Args = []interface{}{"1", "2", "99999", "4"}
+	_, err = def.Run(context.Background())
+	assert.Equal(t, `cannot convert argument 2: "99999" to int16: strconv.ParseInt: parsing "99999": value out of range`, err.(error).Error())
+
+	def.Args = []interface{}{"1", strings.Repeat("2", 32), "3", "4"}
+	_, err = def.Run(context.Background())
+	assert.Equal(t, `cannot convert argument 1: "22222222222222222222222222222222" to int32: strconv.ParseInt: parsing "22222222222222222222222222222222": value out of range`, err.(error).Error())
+
+	def.Args = []interface{}{strings.Repeat("1", 32), "2", "3", "4"}
+	_, err = def.Run(context.Background())
+	assert.Equal(t, `cannot convert argument 0: "11111111111111111111111111111111" to int64: strconv.ParseInt: parsing "11111111111111111111111111111111": value out of range`, err.(error).Error())
+}
+
+func TestShouldSupportFloatTypes(t *testing.T) {
+	var aActual float64
+	var bActual float32
+	fn := func(a float64, b float32) {
+		aActual = a
+		bActual = b
+	}
+
+	def := &models.StepDefinition{
+		StepDefinition: formatters.StepDefinition{
+			Handler: fn,
+		},
+		HandlerValue: reflect.ValueOf(fn),
+	}
+
+	def.Args = []interface{}{"1.1", "2.2"}
+	_, err := def.Run(context.Background())
+	assert.Nil(t, err)
+	assert.Equal(t, float64(1.1), aActual)
+	assert.Equal(t, float32(2.2), bActual)
+
+	def.Args = []interface{}{"1.1", strings.Repeat("2", 65) + ".22"}
+	_, err = def.Run(context.Background())
+	assert.Equal(t, `cannot convert argument 1: "22222222222222222222222222222222222222222222222222222222222222222.22" to float32: strconv.ParseFloat: parsing "22222222222222222222222222222222222222222222222222222222222222222.22": value out of range`, err.(error).Error())
+}
+
+func TestShouldSupportGherkinDocstring(t *testing.T) {
+	var actualDocString *messages.PickleDocString
+	fnDocstring := func(a *messages.PickleDocString) {
+		actualDocString = a
+	}
+
+	expectedDocString := &messages.PickleDocString{Content: "hello"}
+	defDocstring := &models.StepDefinition{
+		StepDefinition: formatters.StepDefinition{
+			Handler: fnDocstring,
+		},
+		HandlerValue: reflect.ValueOf(fnDocstring),
+		Args:         []interface{}{expectedDocString},
+	}
+
+	_, err := defDocstring.Run(context.Background())
+	assert.Nil(t, err)
+	assert.Equal(t, expectedDocString, actualDocString)
+}
+
+func TestShouldSupportGherkinTable(t *testing.T) {
+
+	var actualTable *messages.PickleTable
+	fnTable := func(a *messages.PickleTable) {
+		actualTable = a
+	}
+
+	expectedTable := &messages.PickleTable{}
+	defTable := &models.StepDefinition{
+		StepDefinition: formatters.StepDefinition{
+			Handler: fnTable,
+		},
+		HandlerValue: reflect.ValueOf(fnTable),
+		Args:         []interface{}{expectedTable},
+	}
+
+	_, err := defTable.Run(context.Background())
+	assert.Nil(t, err)
+	assert.Equal(t, expectedTable, actualTable)
+}
+
+func TestShouldSupportOnlyByteSlice(t *testing.T) {
+	var aActual []byte
+	fn1 := func(a []byte) {
+		aActual = a
+	}
+	fn2 := func(a []string) {
+		assert.Fail(t, "fn2 should not be called")
+	}
+
+	def1 := &models.StepDefinition{
+		StepDefinition: formatters.StepDefinition{
+			Handler: fn1,
+		},
+		HandlerValue: reflect.ValueOf(fn1),
+		Args:         []interface{}{"str"},
+	}
+
+	def2 := &models.StepDefinition{
+		StepDefinition: formatters.StepDefinition{
+			Handler: fn2,
+		},
+		HandlerValue: reflect.ValueOf(fn2),
+		Args:         []interface{}{[]string{}},
+	}
+
+	_, err := def1.Run(context.Background())
+	assert.Nil(t, err)
+	assert.Equal(t, []byte{'s', 't', 'r'}, aActual)
+
+	_, err = def2.Run(context.Background())
+	assert.Equal(t, `func has unsupported parameter type: the slice parameter 0 type []string is not supported`, err.(error).Error())
+	assert.True(t, errors.Is(err.(error), models.ErrUnsupportedParameterType))
+}
+
+// this test is superficial compared to the ones above where the actual error messages the user woudl see are verified
+func TestStepDefinition_Run_StepArgsShouldBeString(t *testing.T) {
+	test := func(t *testing.T, fn interface{}, expectedError string) {
 		def := &models.StepDefinition{
 			StepDefinition: formatters.StepDefinition{
 				Handler: fn,
@@ -231,6 +416,7 @@ func TestStepDefinition_Run_StepShouldBeString(t *testing.T) {
 			HandlerValue: reflect.ValueOf(fn),
 		}
 
+		// some value that is not a string
 		def.Args = []interface{}{12}
 
 		_, res := def.Run(context.Background())
@@ -246,26 +432,30 @@ func TestStepDefinition_Run_StepShouldBeString(t *testing.T) {
 		if !errors.Is(err, models.ErrCannotConvert) {
 			t.Fatalf("expected a string convertion error, but got '%v' instead", err)
 		}
+
+		assert.Equal(t, expectedError, err.Error())
 	}
 
 	// Ensure step type error if step argument is not a string
 	// for all supported types.
-	test(t, func(a int) error { return nil })
-	test(t, func(a int64) error { return nil })
-	test(t, func(a int32) error { return nil })
-	test(t, func(a int16) error { return nil })
-	test(t, func(a int8) error { return nil })
-	test(t, func(a string) error { return nil })
-	test(t, func(a float64) error { return nil })
-	test(t, func(a float32) error { return nil })
-	test(t, func(a *godog.Table) error { return nil })
-	test(t, func(a *godog.DocString) error { return nil })
-	test(t, func(a []byte) error { return nil })
+	const toStringError = `cannot convert argument 0: "12" of type "int" to string`
+	shouldNotBeCalled := func() { assert.Fail(t, "shound not be called") }
+	test(t, func(a int) { shouldNotBeCalled() }, toStringError)
+	test(t, func(a int64) { shouldNotBeCalled() }, toStringError)
+	test(t, func(a int32) { shouldNotBeCalled() }, toStringError)
+	test(t, func(a int16) { shouldNotBeCalled() }, toStringError)
+	test(t, func(a int8) { shouldNotBeCalled() }, toStringError)
+	test(t, func(a string) { shouldNotBeCalled() }, toStringError)
+	test(t, func(a float64) { shouldNotBeCalled() }, toStringError)
+	test(t, func(a float32) { shouldNotBeCalled() }, toStringError)
+	test(t, func(a *godog.Table) { shouldNotBeCalled() }, `cannot convert argument 0: "12" of type "int" to *messages.PickleTable`)
+	test(t, func(a *godog.DocString) { shouldNotBeCalled() }, `cannot convert argument 0: "12" of type "int" to *messages.PickleDocString`)
+	test(t, func(a []byte) { shouldNotBeCalled() }, toStringError)
 
 }
 
 func TestStepDefinition_Run_InvalidHandlerParamConversion(t *testing.T) {
-	test := func(t *testing.T, fn interface{}) {
+	test := func(t *testing.T, fn interface{}, expectedError string) {
 		def := &models.StepDefinition{
 			StepDefinition: formatters.StepDefinition{
 				Handler: fn,
@@ -285,44 +475,49 @@ func TestStepDefinition_Run_InvalidHandlerParamConversion(t *testing.T) {
 			t.Fatalf("expected an unsupported argument type error, but got %T instead", res)
 		}
 
-		if !errors.Is(err, models.ErrUnsupportedArgumentType) {
+		if !errors.Is(err, models.ErrUnsupportedParameterType) {
+			// FIXME JL - check logic as the error message was wrong
 			t.Fatalf("expected an unsupported argument type error, but got '%v' instead", err)
 		}
+
+		assert.Equal(t, expectedError, err.Error())
 	}
 
+	shouldNotBeCalled := func() { assert.Fail(t, "shound not be called") }
+
 	// Lists some unsupported argument types for step handler.
 
 	// Pointers should work only for godog.Table/godog.DocString
-	test(t, func(a *int) error { return nil })
-	test(t, func(a *int64) error { return nil })
-	test(t, func(a *int32) error { return nil })
-	test(t, func(a *int16) error { return nil })
-	test(t, func(a *int8) error { return nil })
-	test(t, func(a *string) error { return nil })
-	test(t, func(a *float64) error { return nil })
-	test(t, func(a *float32) error { return nil })
+	test(t, func(a *int) { shouldNotBeCalled() }, "func has unsupported parameter type: the data type of parameter 0 type *int is not supported")
+	test(t, func(a *int64) { shouldNotBeCalled() }, "func has unsupported parameter type: the data type of parameter 0 type *int64 is not supported")
+	test(t, func(a *int32) { shouldNotBeCalled() }, "func has unsupported parameter type: the data type of parameter 0 type *int32 is not supported")
+	test(t, func(a *int16) { shouldNotBeCalled() }, "func has unsupported parameter type: the data type of parameter 0 type *int16 is not supported")
+	test(t, func(a *int8) { shouldNotBeCalled() }, "func has unsupported parameter type: the data type of parameter 0 type *int8 is not supported")
+	test(t, func(a *string) { shouldNotBeCalled() }, "func has unsupported parameter type: the data type of parameter 0 type *string is not supported")
+	test(t, func(a *float64) { shouldNotBeCalled() }, "func has unsupported parameter type: the data type of parameter 0 type *float64 is not supported")
+	test(t, func(a *float32) { shouldNotBeCalled() }, "func has unsupported parameter type: the data type of parameter 0 type *float32 is not supported")
 
 	// I cannot pass structures
-	test(t, func(a godog.Table) error { return nil })
-	test(t, func(a godog.DocString) error { return nil })
-	test(t, func(a testStruct) error { return nil })
+	test(t, func(a godog.Table) { shouldNotBeCalled() }, "func has unsupported parameter type: the struct parameter 0 type messages.PickleTable is not supported")
+	test(t, func(a godog.DocString) { shouldNotBeCalled() }, "func has unsupported parameter type: the struct parameter 0 type messages.PickleDocString is not supported")
+	test(t, func(a testStruct) { shouldNotBeCalled() }, "func has unsupported parameter type: the struct parameter 0 type models_test.testStruct is not supported")
 
-	// I cannot use maps
-	test(t, func(a map[string]interface{}) error { return nil })
-	test(t, func(a map[string]int) error { return nil })
+	// // I cannot use maps
+	test(t, func(a map[string]interface{ body() }) { shouldNotBeCalled() }, "func has unsupported parameter type: the parameter 0 type map is not supported")
+	test(t, func(a map[string]int) { shouldNotBeCalled() }, "func has unsupported parameter type: the parameter 0 type map is not supported")
 
-	// Slice works only for byte
-	test(t, func(a []int) error { return nil })
-	test(t, func(a []string) error { return nil })
-	test(t, func(a []bool) error { return nil })
+	// // Slice works only for byte
+	test(t, func(a []int) { shouldNotBeCalled() }, "func has unsupported parameter type: the slice parameter 0 type []int is not supported")
+	test(t, func(a []string) { shouldNotBeCalled() }, "func has unsupported parameter type: the slice parameter 0 type []string is not supported")
+	test(t, func(a []bool) { shouldNotBeCalled() }, "func has unsupported parameter type: the slice parameter 0 type []bool is not supported")
 
-	// I cannot use bool
-	test(t, func(a bool) error { return nil })
+	// // I cannot use bool
+	test(t, func(a bool) { shouldNotBeCalled() }, "func has unsupported parameter type: the parameter 0 type bool is not supported")
 
 }
 
 func TestStepDefinition_Run_StringConversionToFunctionType(t *testing.T) {
-	test := func(t *testing.T, fn interface{}, args []interface{}) {
+	test := func(t *testing.T, fn interface{}, args []interface{}, expectedError string) {
 		def := &models.StepDefinition{
 			StepDefinition: formatters.StepDefinition{
 				Handler: fn,
@@ -344,26 +539,30 @@ func TestStepDefinition_Run_StringConversionToFunctionType(t *testing.T) {
 		if !errors.Is(err, models.ErrCannotConvert) {
 			t.Fatalf("expected a cannot convert argument type error, but got '%v' instead", err)
 		}
+
+		assert.Equal(t, expectedError, err.Error())
 	}
 
+	shouldNotBeCalled := func() { assert.Fail(t, "shound not be called") }
+
 	// Lists some unsupported argument types for step handler.
 
 	// Cannot convert invalid int
-	test(t, func(a int) error { return nil }, []interface{}{"a"})
-	test(t, func(a int64) error { return nil }, []interface{}{"a"})
-	test(t, func(a int32) error { return nil }, []interface{}{"a"})
-	test(t, func(a int16) error { return nil }, []interface{}{"a"})
-	test(t, func(a int8) error { return nil }, []interface{}{"a"})
+	test(t, func(a int) { shouldNotBeCalled() }, []interface{}{"a"}, `cannot convert argument 0: "a" to int: strconv.ParseInt: parsing "a": invalid syntax`)
+	test(t, func(a int64) { shouldNotBeCalled() }, []interface{}{"a"}, `cannot convert argument 0: "a" to int64: strconv.ParseInt: parsing "a": invalid syntax`)
+	test(t, func(a int32) { shouldNotBeCalled() }, []interface{}{"a"}, `cannot convert argument 0: "a" to int32: strconv.ParseInt: parsing "a": invalid syntax`)
+	test(t, func(a int16) { shouldNotBeCalled() }, []interface{}{"a"}, `cannot convert argument 0: "a" to int16: strconv.ParseInt: parsing "a": invalid syntax`)
+	test(t, func(a int8) { shouldNotBeCalled() }, []interface{}{"a"}, `cannot convert argument 0: "a" to int8: strconv.ParseInt: parsing "a": invalid syntax`)
 
 	// Cannot convert invalid float
-	test(t, func(a float32) error { return nil }, []interface{}{"a"})
-	test(t, func(a float64) error { return nil }, []interface{}{"a"})
+	test(t, func(a float32) { shouldNotBeCalled() }, []interface{}{"a"}, `cannot convert argument 0: "a" to float32: strconv.ParseFloat: parsing "a": invalid syntax`)
+	test(t, func(a float64) { shouldNotBeCalled() }, []interface{}{"a"}, `cannot convert argument 0: "a" to float64: strconv.ParseFloat: parsing "a": invalid syntax`)
 
 	// Cannot convert to DataArg
-	test(t, func(a *godog.Table) error { return nil }, []interface{}{"194"})
+	test(t, func(a *godog.Table) { shouldNotBeCalled() }, []interface{}{"194"}, `cannot convert argument 0: "194" of type "string" to *messages.PickleTable`)
 
 	// Cannot convert to DocString ?
-	test(t, func(a *godog.DocString) error { return nil }, []interface{}{"194"})
+	test(t, func(a *godog.DocString) { shouldNotBeCalled() }, []interface{}{"194"}, `cannot convert argument 0: "194" of type "string" to *messages.PickleDocString`)
 
 }
 
@@ -381,11 +580,9 @@ type testStruct struct {
 }
 
 func TestShouldSupportDocStringToStringConversion(t *testing.T) {
-	fn := func(a string) error {
-		if a != "hello" {
-			return fmt.Errorf("did not get hello")
-		}
-		return nil
+	var aActual string
+	fn := func(a string) {
+		aActual = a
 	}
 
 	def := &models.StepDefinition{
@@ -398,7 +595,7 @@ func TestShouldSupportDocStringToStringConversion(t *testing.T) {
 		}},
 	}
 
-	if _, err := def.Run(context.Background()); err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
+	_, err := def.Run(context.Background())
+	assert.Nil(t, err)
+	assert.Equal(t, "hello", aActual)
 }

run_progress_test.go

@@ -75,11 +75,11 @@ func Test_ProgressFormatterWithPanicInMultistep(t *testing.T) {
 		fmt:      formatters.ProgressFormatterFunc("progress", w),
 		features: []*models.Feature{&ft},
 		scenarioInitializer: func(ctx *ScenarioContext) {
-			ctx.Step(`^sub1$`, func() error { return nil })
+			ctx.Step(`^sub1$`, func() error { panic("DELIBERATE FAILURE") })
 			ctx.Step(`^sub-sub$`, func() error { return nil })
-			ctx.Step(`^sub2$`, func() []string { return []string{"sub-sub", "sub1", "one"} })
+			ctx.Step(`^sub2$`, func() Steps { return Steps{"sub-sub", "sub1", "one"} })
 			ctx.Step(`^one$`, func() error { return nil })
-			ctx.Step(`^two$`, func() []string { return []string{"sub1", "sub2"} })
+			ctx.Step(`^two$`, func() Steps { return []string{"sub1", "sub2"} })
 		},
 	}
 

suite_context_test.go

@@ -1149,7 +1149,6 @@ func TestTestSuite_Run(t *testing.T) {
 				<<<< After suite`,
 		},
 	} {
-		// JL
 		t.Run(tc.name, func(t *testing.T) {
 			afterScenarioCnt := 0
 			beforeScenarioCnt := 0

test_context.go

@@ -281,6 +281,7 @@ func (ctx ScenarioContext) Then(expr, stepFunc interface{}) {
 func (ctx ScenarioContext) stepWithKeyword(expr interface{}, stepFunc interface{}, keyword formatters.Keyword) {
 	var regex *regexp.Regexp
 
+	// Validate the first input param is regex compatible
 	switch t := expr.(type) {
 	case *regexp.Regexp:
 		regex = t
@@ -289,45 +290,59 @@ func (ctx ScenarioContext) stepWithKeyword(expr interface{}, stepFunc interface{
 	case []byte:
 		regex = regexp.MustCompile(string(t))
 	default:
-		panic(fmt.Sprintf("expecting expr to be a *regexp.Regexp or a string, got type: %T", expr))
+		panic(fmt.Sprintf("expecting expr to be a *regexp.Regexp or a string or []byte, got type: %T", expr))
 	}
 
-	v := reflect.ValueOf(stepFunc)
-	typ := v.Type()
-	if typ.Kind() != reflect.Func {
+	// Validate that the handler is a function.
+	handlerType := reflect.TypeOf(stepFunc)
+	if handlerType.Kind() != reflect.Func {
 		panic(fmt.Sprintf("expected handler to be func, but got: %T", stepFunc))
 	}
 
-	if typ.NumOut() > 2 {
-		panic(fmt.Sprintf("expected handler to return either zero, one or two values, but it has: %d", typ.NumOut()))
+	// FIXME = Validate the handler function param types here so
+	// that any errors are discovered early.
+	// StepDefinition.Run defines the supported types but fails at run time not registration time
+
+	// Validate the function's return types.
+	helpPrefix := "expected handler to return one of error or context.Context or godog.Steps or (context.Context, error)"
+	isNested := false
+
+	numOut := handlerType.NumOut()
+	switch numOut {
+	case 0:
+		// No return values.
+	case 1:
+		// One return value: should be error, Steps, or context.Context.
+		outType := handlerType.Out(0)
+		if outType == reflect.TypeOf(Steps{}) {
+			isNested = true
+		} else {
+			if outType != errorInterface && outType != contextInterface {
+				panic(fmt.Sprintf("%s, but got: %v", helpPrefix, outType))
+			}
+		}
+	case 2:
+		// Two return values: should be (context.Context, error).
+		if handlerType.Out(0) != contextInterface || handlerType.Out(1) != errorInterface {
+			panic(fmt.Sprintf("%s, but got: %v, %v", helpPrefix, handlerType.Out(0), handlerType.Out(1)))
+		}
+	default:
+		// More than two return values.
+		panic(fmt.Sprintf("expected handler to return either zero, one or two values, but it has: %d", numOut))
 	}
 
+	// Register the handler
 	def := &models.StepDefinition{
 		StepDefinition: formatters.StepDefinition{
 			Handler: stepFunc,
 			Expr:    regex,
 			Keyword: keyword,
 		},
-		HandlerValue: v,
-	}
-
-	if typ.NumOut() == 1 {
-		typ = typ.Out(0)
-		switch typ.Kind() {
-		case reflect.Interface:
-			if !typ.Implements(errorInterface) && !typ.Implements(contextInterface) {
-				panic(fmt.Sprintf("expected handler to return an error or context.Context, but got: %s", typ.Kind()))
-			}
-		case reflect.Slice:
-			if typ.Elem().Kind() != reflect.String {
-				panic(fmt.Sprintf("expected handler to return []string for multistep, but got: []%s", typ.Elem().Kind()))
-			}
-			def.Nested = true
-		default:
-			panic(fmt.Sprintf("expected handler to return an error or []string, but got: %s", typ.Kind()))
-		}
+		HandlerValue: reflect.ValueOf(stepFunc),
+		Nested:       isNested,
 	}
 
+	// stash the step
 	ctx.suite.steps = append(ctx.suite.steps, def)
 }
 

test_context_test.go

@@ -1,14 +1,16 @@
 package godog
 
 import (
-	"github.com/stretchr/testify/assert"
+	"context"
 	"regexp"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestScenarioContext_Step(t *testing.T) {
 	ctx := ScenarioContext{suite: &suite{}}
-	re := regexp.MustCompile(`(?:it is a test)?.{10}x*`)
+	re := `(?:it is a test)?.{10}x*`
 
 	type tc struct {
 		f func()
@@ -18,15 +20,18 @@ func TestScenarioContext_Step(t *testing.T) {
 
 	for _, c := range []tc{
 		{n: "ScenarioContext should accept steps defined with regexp.Regexp",
-			f: func() { ctx.Step(re, okEmptyResult) }},
+			f: func() { ctx.Step(regexp.MustCompile(re), okVoidResult) }},
 		{n: "ScenarioContext should accept steps defined with bytes slice",
-			f: func() { ctx.Step([]byte("(?:it is a test)?.{10}x*"), okEmptyResult) }},
-		{n: "ScenarioContext should accept steps handler with error return",
-			f: func() { ctx.Step(".*", okEmptyResult) }},
+			f: func() { ctx.Step([]byte(re), okVoidResult) }},
+
+		{n: "ScenarioContext should accept steps handler with no return",
+			f: func() { ctx.Step(".*", okVoidResult) }},
 		{n: "ScenarioContext should accept steps handler with error return",
 			f: func() { ctx.Step(".*", okErrorResult) }},
-		{n: "ScenarioContext should accept steps handler with string slice return",
-			f: func() { ctx.Step(".*", okSliceResult) }},
+		{n: "ScenarioContext should accept steps handler with godog.Steps return",
+			f: func() { ctx.Step(".*", okStepsResult) }},
+		{n: "ScenarioContext should accept steps handler with (Context, error) return",
+			f: func() { ctx.Step(".*", okContextErrorResult) }},
 	} {
 		t.Run(c.n, func(t *testing.T) {
 			assert.NotPanics(t, c.f)
@@ -35,30 +40,33 @@ func TestScenarioContext_Step(t *testing.T) {
 
 	for _, c := range []tc{
 		{n: "ScenarioContext should panic if step expression is neither a string, regex or byte slice",
-			p: "expecting expr to be a *regexp.Regexp or a string, got type: int",
-			f: func() { ctx.Step(1251, okSliceResult) }},
+			p: "expecting expr to be a *regexp.Regexp or a string or []byte, got type: int",
+			f: func() { ctx.Step(1251, okVoidResult) }},
 		{n: "ScenarioContext should panic if step handler is not a function",
 			p: "expected handler to be func, but got: int",
 			f: func() { ctx.Step(".*", 124) }},
 		{n: "ScenarioContext should panic if step handler has more than 2 return values",
 			p: "expected handler to return either zero, one or two values, but it has: 3",
-			f: func() { ctx.Step(".*", nokLimitCase) }},
+			f: func() { ctx.Step(".*", nokLimitCase3) }},
 		{n: "ScenarioContext should panic if step handler has more than 2 return values (5)",
 			p: "expected handler to return either zero, one or two values, but it has: 5",
-			f: func() { ctx.Step(".*", nokMore) }},
+			f: func() { ctx.Step(".*", nokLimitCase5) }},
 
 		{n: "ScenarioContext should panic if step expression is neither a string, regex or byte slice",
-			p: "expecting expr to be a *regexp.Regexp or a string, got type: int",
-			f: func() { ctx.Step(1251, okSliceResult) }},
+			p: "expecting expr to be a *regexp.Regexp or a string or []byte, got type: int",
+			f: func() { ctx.Step(1251, okVoidResult) }},
 
+		{n: "ScenarioContext should panic if step return type is []string",
+			p: "expected handler to return one of error or context.Context or godog.Steps or (context.Context, error), but got: []string",
+			f: func() { ctx.Step(".*", nokSliceStringResult) }},
 		{n: "ScenarioContext should panic if step handler return type is not an error or string slice or void (interface)",
-			p: "expected handler to return an error or context.Context, but got: interface",
+			p: "expected handler to return one of error or context.Context or godog.Steps or (context.Context, error), but got: interface {}",
 			f: func() { ctx.Step(".*", nokInvalidReturnInterfaceType) }},
 		{n: "ScenarioContext should panic if step handler return type is not an error or string slice or void (slice)",
-			p: "expected handler to return []string for multistep, but got: []int",
+			p: "expected handler to return one of error or context.Context or godog.Steps or (context.Context, error), but got: []int",
 			f: func() { ctx.Step(".*", nokInvalidReturnSliceType) }},
 		{n: "ScenarioContext should panic if step handler return type is not an error or string slice or void (other)",
-			p: "expected handler to return an error or []string, but got: chan",
+			p: "expected handler to return one of error or context.Context or godog.Steps or (context.Context, error), but got: chan int",
 			f: func() { ctx.Step(".*", nokInvalidReturnOtherType) }},
 	} {
 		t.Run(c.n, func(t *testing.T) {
@@ -67,11 +75,13 @@ func TestScenarioContext_Step(t *testing.T) {
 	}
 }
 
-func okEmptyResult()                             {}
-func okErrorResult() error                       { return nil }
-func okSliceResult() []string                    { return nil }
-func nokLimitCase() (string, int, error)         { return "", 0, nil }
-func nokMore() (int, int, int, int, error)       { return 0, 0, 0, 0, nil }
-func nokInvalidReturnInterfaceType() interface{} { return 0 }
-func nokInvalidReturnSliceType() []int           { return nil }
-func nokInvalidReturnOtherType() chan int        { return nil }
+func okVoidResult()                                  {}
+func okErrorResult() error                           { return nil }
+func okStepsResult() Steps                           { return nil }
+func okContextErrorResult() (context.Context, error) { return nil, nil }
+func nokSliceStringResult() []string                 { return nil }
+func nokLimitCase3() (string, int, error)            { return "", 0, nil }
+func nokLimitCase5() (int, int, int, int, error)     { return 0, 0, 0, 0, nil }
+func nokInvalidReturnInterfaceType() interface{}     { return 0 }
+func nokInvalidReturnSliceType() []int               { return nil }
+func nokInvalidReturnOtherType() chan int            { return nil }