Fix some type checks on the signatures of nested step handlers

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

@@ -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
 
@@ -184,17 +187,31 @@ func (sd *StepDefinition) Run(ctx context.Context) (context.Context, interface{}
 		return ctx, nil
 	}
 
+	// Note that the step fn return types were validated at Initialise in test_context.go stepWithKeyword()
+
+	// single return value may be one of ...
+	// error
+	// context.Context
+	// godog.Steps
+	result0 := res[0].Interface()
 	if len(res) == 1 {
-		r := res[0].Interface()
 
-		if ctx, ok := r.(context.Context); ok {
+		// 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
+	}
+
+	// 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()
 	}
 
-	return res[0].Interface().(context.Context), res[1].Interface()
+	panic(fmt.Errorf("step should have returned (context.Context, error), but found %v rather than a context.Context value", result0))
 }
 
 func (sd *StepDefinition) shouldBeString(idx int) (string, error) {

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

@@ -293,13 +293,13 @@ func (ctx ScenarioContext) stepWithKeyword(expr interface{}, stepFunc interface{
 	}
 
 	v := reflect.ValueOf(stepFunc)
-	typ := v.Type()
-	if typ.Kind() != reflect.Func {
+	fnTyp := v.Type()
+	if fnTyp.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()))
+	if fnTyp.NumOut() > 2 {
+		panic(fmt.Sprintf("expected handler to return either zero, one or two values, but it has: %d", fnTyp.NumOut()))
 	}
 
 	def := &models.StepDefinition{
@@ -311,23 +311,39 @@ func (ctx ScenarioContext) stepWithKeyword(expr interface{}, stepFunc interface{
 		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()))
-			}
+	// verify valid return types
+	helpPrefix := "expected handler to return one of error or context.Context or godog.Steps or (context.Context, error)"
+
+	if fnTyp.NumOut() == 1 {
+		typ0 := fnTyp.Out(0)
+
+		if typ0 == reflect.TypeOf(Steps{}) {
+			// a return value of Steps is ok
 			def.Nested = true
-		default:
-			panic(fmt.Sprintf("expected handler to return an error or []string, but got: %s", typ.Kind()))
+		} else {
+			switch typ0.Kind() {
+			case reflect.Interface:
+				// error and context are ok
+				if !typ0.Implements(errorInterface) && !typ0.Implements(contextInterface) {
+					panic(fmt.Sprintf("%s, but got: %s", helpPrefix, typ0.Kind()))
+				}
+			case reflect.Slice:
+				panic(fmt.Sprintf("%s, but got: []%s", helpPrefix, typ0.Elem().Kind()))
+			default:
+				panic(fmt.Sprintf("%s, but got: %s", helpPrefix, typ0.Kind()))
+			}
+		}
+	}
+
+	if fnTyp.NumOut() == 2 {
+		typ0 := fnTyp.Out(0)
+		typ1 := fnTyp.Out(1)
+		if !typ0.Implements(contextInterface) || !typ1.Implements(errorInterface) {
+			panic(fmt.Sprintf("%s, but got (%v, %s)", helpPrefix, typ0.Name(), typ1.Name()))
 		}
 	}
 
+	// 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)
@@ -36,29 +41,32 @@ 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) }},
+			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) }},
+			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",
 			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 }