From 223efc3b14f42a5d094de7ddf30ea264b3e4397a Mon Sep 17 00:00:00 2001 From: John Lonergan <836248+Johnlon@users.noreply.github.com> Date: Tue, 15 Oct 2024 13:25:13 +0100 Subject: [PATCH] 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 | 1 + internal/formatters/fmt_output_test.go | 3 - internal/models/stepdef.go | 51 ++- internal/models/stepdef_test.go | 493 +++++++++++++++++-------- run_progress_test.go | 6 +- suite_context_test.go | 1 - test_context.go | 63 ++-- test_context_test.go | 60 +-- 8 files changed, 465 insertions(+), 213 deletions(-) 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/formatters/fmt_output_test.go b/internal/formatters/fmt_output_test.go index 5690b6d3..46ac0e7f 100644 --- a/internal/formatters/fmt_output_test.go +++ b/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") - } } } 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 }