diff --git a/CHANGELOG.md b/CHANGELOG.md index 6cc87e75..f1a46785 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/internal/models/stepdef.go b/internal/models/stepdef.go index 497ffc7c..90ffef54 100644 --- a/internal/models/stepdef.go +++ b/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 } + // 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() + } + + 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 for the context.Context value%s", text, errMsg)) } - return res[0].Interface().(context.Context), res[1].Interface() + 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) { diff --git a/internal/models/stepdef_test.go b/internal/models/stepdef_test.go index 6617f48e..318f375a 100644 --- a/internal/models/stepdef_test.go +++ b/internal/models/stepdef_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "reflect" + "regexp" "strings" "testing" @@ -18,13 +19,42 @@ import ( type ctxKey string -func TestShouldSupportContext(t *testing.T) { - ctx := context.WithValue(context.Background(), ctxKey("original"), 123) +func TestShouldSupportVoidHandlerReturn(t *testing.T) { + wasCalled := false + initialCtx := 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) { + wasCalled = true assert.Equal(t, 123, ctx.Value(ctxKey("original"))) + } - return context.WithValue(ctx, ctxKey("updated"), 321) + 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{ @@ -34,21 +64,49 @@ func TestShouldSupportContext(t *testing.T) { HandlerValue: reflect.ValueOf(fn), } - def.Args = []interface{}{"1", "1", "1", "1"} - ctx, err := def.Run(ctx) + 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) - assert.Equal(t, 123, ctx.Value(ctxKey("original"))) - assert.Equal(t, 321, ctx.Value(ctxKey("updated"))) } -func TestShouldSupportContextAndError(t *testing.T) { +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, error) { + fn := func(ctx context.Context) context.Context { assert.Equal(t, 123, ctx.Value(ctxKey("original"))) - return context.WithValue(ctx, ctxKey("updated"), 321), nil + return context.WithValue(ctx, ctxKey("updated"), 321) } def := &models.StepDefinition{ @@ -58,15 +116,23 @@ func TestShouldSupportContextAndError(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 TestShouldSupportEmptyHandlerReturn(t *testing.T) { - fn := func(a int64, b int32, c int16, d int8) {} +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{ @@ -75,19 +141,48 @@ func TestShouldSupportEmptyHandlerReturn(t *testing.T) { 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{}{} + 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.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") + 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 TestShouldSupportIntTypes(t *testing.T) { - fn := func(a int64, b int32, c int16, d int8) error { return nil } +func TestShouldSupportContextAndNilErrorReturn(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"))) + + return context.WithValue(ctx, ctxKey("updated"), 321), nil + } def := &models.StepDefinition{ StepDefinition: formatters.StepDefinition{ @@ -96,19 +191,53 @@ func TestShouldSupportIntTypes(t *testing.T) { 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{}{} + 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 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.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") + def := &models.StepDefinition{ + StepDefinition: formatters.StepDefinition{ + Handler: fn, + Expr: regexp.MustCompile("some regex string"), + }, + HandlerValue: reflect.ValueOf(fn), } + + def.Args = []interface{}{} + + 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 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 TestShouldSupportFloatTypes(t *testing.T) { - fn := func(a float64, b float32) error { return nil } +func TestArgumentCountChecks(t *testing.T) { + + wasCalled := false + fn := func(a int, b int) { + wasCalled = true + } def := &models.StepDefinition{ StepDefinition: formatters.StepDefinition{ @@ -117,60 +246,140 @@ func TestShouldSupportFloatTypes(t *testing.T) { 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"} + _, 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)) + + // 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 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.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") + 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 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 } +func TestShouldSupportFloatTypes(t *testing.T) { + var aActual float64 + var bActual float32 + fn := func(a float64, b float32) { + aActual = a + bActual = b + } - def1 := &models.StepDefinition{ + def := &models.StepDefinition{ StepDefinition: formatters.StepDefinition{ - Handler: fn1, + Handler: fn, }, - HandlerValue: reflect.ValueOf(fn1), - Args: []interface{}{(*int)(nil)}, + HandlerValue: reflect.ValueOf(fn), } - def2 := &models.StepDefinition{ - StepDefinition: formatters.StepDefinition{ - Handler: fn2, - }, - HandlerValue: reflect.ValueOf(fn2), - Args: []interface{}{&messages.PickleDocString{}}, + + 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 } - def3 := &models.StepDefinition{ + + expectedDocString := &messages.PickleDocString{Content: "hello"} + defDocstring := &models.StepDefinition{ StepDefinition: formatters.StepDefinition{ - Handler: fn3, + Handler: fnDocstring, }, - HandlerValue: reflect.ValueOf(fn3), - Args: []interface{}{(*messages.PickleTable)(nil)}, + HandlerValue: reflect.ValueOf(fnDocstring), + Args: []interface{}{expectedDocString}, } - if _, err := def1.Run(context.Background()); err == nil { - t.Fatalf("expected conversion error, but got none") - } + _, err := defDocstring.Run(context.Background()) + assert.Nil(t, err) + assert.Equal(t, expectedDocString, actualDocString) +} + +func TestShouldSupportGherkinTable(t *testing.T) { - if _, err := def2.Run(context.Background()); err != nil { - t.Fatalf("unexpected error: %v", err) + var actualTable *messages.PickleTable + fnTable := func(a *messages.PickleTable) { + actualTable = a } - if _, err := def3.Run(context.Background()); err != nil { - t.Fatalf("unexpected error: %v", err) + 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) { - fn1 := func(a []byte) error { return nil } - fn2 := func(a []string) error { return nil } + 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{ @@ -179,6 +388,7 @@ func TestShouldSupportOnlyByteSlice(t *testing.T) { HandlerValue: reflect.ValueOf(fn1), Args: []interface{}{"str"}, } + def2 := &models.StepDefinition{ StepDefinition: formatters.StepDefinition{ Handler: fn2, @@ -187,43 +397,18 @@ func TestShouldSupportOnlyByteSlice(t *testing.T) { 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, - }, - HandlerValue: reflect.ValueOf(fn), - } - - def.Args = []interface{}{"1"} - - _, 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) - } + _, err := def1.Run(context.Background()) + assert.Nil(t, err) + assert.Equal(t, []byte{'s', 't', 'r'}, aActual) - if !errors.Is(err, models.ErrUnmatchedStepArgumentNumber) { - t.Fatalf("expected an error due to wrong number of arguments, but got %v instead", err) - } + _, 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)) } -func TestStepDefinition_Run_StepShouldBeString(t *testing.T) { - test := func(t *testing.T, fn interface{}) { +// 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) } diff --git a/run_progress_test.go b/run_progress_test.go index 6b66d013..841b7185 100644 --- a/run_progress_test.go +++ b/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"} }) }, } diff --git a/suite_context_test.go b/suite_context_test.go index 68c2839a..b7b5e9de 100644 --- a/suite_context_test.go +++ b/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 diff --git a/test_context.go b/test_context.go index b1006415..8156c6d7 100644 --- a/test_context.go +++ b/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) } diff --git a/test_context_test.go b/test_context_test.go index 7b2dd900..381c4f9d 100644 --- a/test_context_test.go +++ b/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 }