ambiguous step def detection akin to cucumber jvm (#636)

* added basic detection for ambiguous steps, but causes an error and not yet recorded in the reports as 'Ambiguous', and no test cases figured out yet

* added initial support for detection of ambiguous steps - further work take a look at how cuke jvm report ambiguous steps and sets the step status to 'ambiguous' rather than my current solution which just blows the test up as a regular step error

* added suite_context_test and also introduced missing 'ambiguous' status to make cucumber jvm'

* update CHANGELOG for ambiguous step defs

* missed file from commit

* added internal/formatters/fmt_multi_test.go

* add tests for other peoples code

* added "ambigous" to the help text

* tests

* added some more tests for attachments

* Update internal/flags/flags.go

Co-authored-by: Viacheslav Poturaev <nanopeni@gmail.com>

---------

Co-authored-by: Viacheslav Poturaev <nanopeni@gmail.com>

CHANGELOG.md

@@ -8,6 +8,7 @@ This document is formatted according to the principles of [Keep A CHANGELOG](htt
 
 ## Unreleased
 
+- Ambiguous step definitions will now be detected when strit 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))
 
 ## [v0.14.1]

attachment_test.go

@@ -0,0 +1,28 @@
+package godog
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestAttach(t *testing.T) {
+
+	ctx := context.Background()
+
+	ctx = Attach(ctx, Attachment{Body: []byte("body1"), FileName: "fileName1", MediaType: "mediaType1"})
+	ctx = Attach(ctx, Attachment{Body: []byte("body2"), FileName: "fileName2", MediaType: "mediaType2"})
+
+	attachments := Attachments(ctx)
+
+	assert.Equal(t, 2, len(attachments))
+
+	assert.Equal(t, []byte("body1"), attachments[0].Body)
+	assert.Equal(t, "fileName1", attachments[0].FileName)
+	assert.Equal(t, "mediaType1", attachments[0].MediaType)
+
+	assert.Equal(t, []byte("body2"), attachments[1].Body)
+	assert.Equal(t, "fileName2", attachments[1].FileName)
+	assert.Equal(t, "mediaType2", attachments[1].MediaType)
+}

flags.go

@@ -121,7 +121,7 @@ func BindFlags(prefix string, set *flag.FlagSet, opt *Options) {
 	set.BoolVar(&opt.ShowStepDefinitions, prefix+"definitions", defShowStepDefinitions, "Print all available step definitions.")
 	set.BoolVar(&opt.ShowStepDefinitions, prefix+"d", defShowStepDefinitions, "Print all available step definitions.")
 	set.BoolVar(&opt.StopOnFailure, prefix+"stop-on-failure", defStopOnFailure, "Stop processing on first failed scenario.")
-	set.BoolVar(&opt.Strict, prefix+"strict", defStrict, "Fail suite when there are pending or undefined steps.")
+	set.BoolVar(&opt.Strict, prefix+"strict", defStrict, "Fail suite when there are pending or undefined or ambiguous steps.")
 	set.BoolVar(&opt.NoColors, prefix+"no-colors", defNoColors, "Disable ansi colors.")
 	set.Var(&randomSeed{&opt.Randomize}, prefix+"random", descRandomOption)
 	set.BoolVar(&opt.ShowHelp, "godog.help", false, "Show usage help.")

formatters/fmt.go

@@ -70,6 +70,7 @@ type Formatter interface {
 	Skipped(*messages.Pickle, *messages.PickleStep, *StepDefinition)
 	Undefined(*messages.Pickle, *messages.PickleStep, *StepDefinition)
 	Pending(*messages.Pickle, *messages.PickleStep, *StepDefinition)
+	Ambiguous(*messages.Pickle, *messages.PickleStep, *StepDefinition, error)
 	Summary()
 }
 

internal/flags/flags.go

@@ -39,7 +39,7 @@ built-in formatters are:
 
 	flagSet.BoolVarP(&opts.ShowStepDefinitions, prefix+"definitions", "d", opts.ShowStepDefinitions, "print all available step definitions")
 	flagSet.BoolVar(&opts.StopOnFailure, prefix+"stop-on-failure", opts.StopOnFailure, "stop processing on first failed scenario")
-	flagSet.BoolVar(&opts.Strict, prefix+"strict", opts.Strict, "fail suite when there are pending or undefined steps")
+	flagSet.BoolVar(&opts.Strict, prefix+"strict", opts.Strict, "fail suite when there are pending or undefined or ambiguous steps")
 
 	flagSet.Int64Var(&opts.Randomize, prefix+"random", opts.Randomize, `randomly shuffle the scenario execution order
   --random

internal/flags/options.go

@@ -38,7 +38,7 @@ type Options struct {
 	// Stops on the first failure
 	StopOnFailure bool
 
-	// Fail suite when there are pending or undefined steps
+	// Fail suite when there are pending or undefined or ambiguous steps
 	Strict bool
 
 	// Forces ansi color stripping

internal/formatters/fmt.go

@@ -36,6 +36,7 @@ var (
 	skipped   = models.Skipped
 	undefined = models.Undefined
 	pending   = models.Pending
+	ambiguous = models.Skipped
 )
 
 type sortFeaturesByName []*models.Feature

internal/formatters/fmt_base.go

@@ -85,6 +85,10 @@ func (f *Base) Failed(*messages.Pickle, *messages.PickleStep, *formatters.StepDe
 func (f *Base) Pending(*messages.Pickle, *messages.PickleStep, *formatters.StepDefinition) {
 }
 
+// Ambiguous captures ambiguous step.
+func (f *Base) Ambiguous(*messages.Pickle, *messages.PickleStep, *formatters.StepDefinition, error) {
+}
+
 // Summary renders summary information.
 func (f *Base) Summary() {
 	var totalSc, passedSc, undefinedSc int

internal/formatters/fmt_cucumber.go

@@ -101,7 +101,8 @@ func (f *Cuke) buildCukeElements(pickles []*messages.Pickle) (res []cukeElement)
 			cukeStep.Result.Duration = &d
 			if stepResult.Status == undefined ||
 				stepResult.Status == pending ||
-				stepResult.Status == skipped {
+				stepResult.Status == skipped ||
+				stepResult.Status == ambiguous {
 				cukeStep.Result.Duration = nil
 			}
 

internal/formatters/fmt_multi.go

@@ -97,6 +97,13 @@ func (r repeater) Pending(pickle *messages.Pickle, step *messages.PickleStep, de
 	}
 }
 
+// Ambiguous triggers Ambiguous for all added formatters.
+func (r repeater) Ambiguous(pickle *messages.Pickle, step *messages.PickleStep, definition *formatters.StepDefinition, err error) {
+	for _, f := range r {
+		f.Ambiguous(pickle, step, definition, err)
+	}
+}
+
 // Summary triggers Summary for all added formatters.
 func (r repeater) Summary() {
 	for _, f := range r {

internal/formatters/fmt_multi_test.go

@@ -0,0 +1,160 @@
+package formatters
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/cucumber/godog/formatters"
+	messages "github.com/cucumber/messages/go/v21"
+	"github.com/stretchr/testify/assert"
+)
+
+var (
+	mock = DummyFormatter{}
+	base = BaseFormatter{}
+
+	document   = &messages.GherkinDocument{}
+	str        = "theString"
+	byt        = []byte("bytes")
+	pickle     = &messages.Pickle{}
+	step       = &messages.PickleStep{}
+	definition = &formatters.StepDefinition{}
+	err        = errors.New("expected")
+)
+
+// TestRepeater tests the delegation of the repeater functions.
+func TestRepeater(t *testing.T) {
+
+	mock.tt = t
+	f := make(repeater, 0)
+	f = append(f, &mock)
+	f = append(f, &mock)
+	f = append(f, &base)
+
+	f.Feature(document, str, byt)
+	f.TestRunStarted()
+	f.Pickle(pickle)
+	f.Defined(pickle, step, definition)
+	f.Passed(pickle, step, definition)
+	f.Skipped(pickle, step, definition)
+	f.Undefined(pickle, step, definition)
+	f.Failed(pickle, step, definition, err)
+	f.Pending(pickle, step, definition)
+	f.Ambiguous(pickle, step, definition, err)
+
+	assert.Equal(t, 2, mock.CountFeature)
+	assert.Equal(t, 2, mock.CountTestRunStarted)
+	assert.Equal(t, 2, mock.CountPickle)
+	assert.Equal(t, 2, mock.CountDefined)
+	assert.Equal(t, 2, mock.CountPassed)
+	assert.Equal(t, 2, mock.CountSkipped)
+	assert.Equal(t, 2, mock.CountUndefined)
+	assert.Equal(t, 2, mock.CountFailed)
+	assert.Equal(t, 2, mock.CountPending)
+	assert.Equal(t, 2, mock.CountAmbiguous)
+
+}
+
+type BaseFormatter struct {
+	*Base
+}
+
+type DummyFormatter struct {
+	*Base
+
+	tt                  *testing.T
+	CountFeature        int
+	CountTestRunStarted int
+	CountPickle         int
+	CountDefined        int
+	CountPassed         int
+	CountSkipped        int
+	CountUndefined      int
+	CountFailed         int
+	CountPending        int
+	CountAmbiguous      int
+}
+
+// SetStorage assigns gherkin data storage.
+// func (f *DummyFormatter) SetStorage(st *storage.Storage) {
+// }
+
+// TestRunStarted is triggered on test start.
+func (f *DummyFormatter) TestRunStarted() {
+	f.CountTestRunStarted++
+}
+
+// Feature receives gherkin document.
+func (f *DummyFormatter) Feature(d *messages.GherkinDocument, s string, b []byte) {
+	assert.Equal(f.tt, document, d)
+	assert.Equal(f.tt, str, s)
+	assert.Equal(f.tt, byt, b)
+	f.CountFeature++
+}
+
+// Pickle receives scenario.
+func (f *DummyFormatter) Pickle(p *messages.Pickle) {
+	assert.Equal(f.tt, pickle, p)
+	f.CountPickle++
+}
+
+// Defined receives step definition.
+func (f *DummyFormatter) Defined(p *messages.Pickle, s *messages.PickleStep, d *formatters.StepDefinition) {
+	assert.Equal(f.tt, pickle, p)
+	assert.Equal(f.tt, s, step)
+	assert.Equal(f.tt, d, definition)
+	f.CountDefined++
+}
+
+// Passed captures passed step.
+func (f *DummyFormatter) Passed(p *messages.Pickle, s *messages.PickleStep, d *formatters.StepDefinition) {
+	assert.Equal(f.tt, pickle, p)
+	assert.Equal(f.tt, s, step)
+	assert.Equal(f.tt, d, definition)
+	f.CountPassed++
+}
+
+// Skipped captures skipped step.
+func (f *DummyFormatter) Skipped(p *messages.Pickle, s *messages.PickleStep, d *formatters.StepDefinition) {
+	assert.Equal(f.tt, pickle, p)
+	assert.Equal(f.tt, s, step)
+	assert.Equal(f.tt, d, definition)
+
+	f.CountSkipped++
+}
+
+// Undefined captures undefined step.
+func (f *DummyFormatter) Undefined(p *messages.Pickle, s *messages.PickleStep, d *formatters.StepDefinition) {
+	assert.Equal(f.tt, pickle, p)
+	assert.Equal(f.tt, s, step)
+	assert.Equal(f.tt, d, definition)
+
+	f.CountUndefined++
+}
+
+// Failed captures failed step.
+func (f *DummyFormatter) Failed(p *messages.Pickle, s *messages.PickleStep, d *formatters.StepDefinition, e error) {
+	assert.Equal(f.tt, pickle, p)
+	assert.Equal(f.tt, s, step)
+	assert.Equal(f.tt, d, definition)
+	assert.Equal(f.tt, err, e)
+
+	f.CountFailed++
+}
+
+// Pending captures pending step.
+func (f *DummyFormatter) Pending(p *messages.Pickle, s *messages.PickleStep, d *formatters.StepDefinition) {
+	assert.Equal(f.tt, pickle, p)
+	assert.Equal(f.tt, s, step)
+	assert.Equal(f.tt, d, definition)
+
+	f.CountPending++
+}
+
+// Ambiguous captures ambiguous step.
+func (f *DummyFormatter) Ambiguous(p *messages.Pickle, s *messages.PickleStep, d *formatters.StepDefinition, e error) {
+	assert.Equal(f.tt, pickle, p)
+	assert.Equal(f.tt, s, step)
+	assert.Equal(f.tt, d, definition)
+	f.CountAmbiguous++
+}

internal/models/results.go

@@ -72,6 +72,8 @@ const (
 	Undefined
 	// Pending ...
 	Pending
+	// Ambiguous ...
+	Ambiguous
 )
 
 // Color ...
@@ -101,6 +103,8 @@ func (st StepResultStatus) String() string {
 		return "undefined"
 	case Pending:
 		return "pending"
+	case Ambiguous:
+		return "ambiguous"
 	default:
 		return "unknown"
 	}

internal/models/results_test.go

@@ -1,6 +1,7 @@
 package models_test
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -21,6 +22,7 @@ var stepResultStatusTestCases = []stepResultStatusTestCase{
 	{st: models.Skipped, str: "skipped", clr: colors.Cyan},
 	{st: models.Undefined, str: "undefined", clr: colors.Yellow},
 	{st: models.Pending, str: "pending", clr: colors.Yellow},
+	{st: models.Ambiguous, str: "ambiguous", clr: colors.Yellow},
 	{st: -1, str: "unknown", clr: colors.Yellow},
 }
 
@@ -32,3 +34,21 @@ func Test_StepResultStatus(t *testing.T) {
 		})
 	}
 }
+
+func Test_NewStepResuklt(t *testing.T) {
+	status := models.StepResultStatus(123)
+	pickleID := "pickleId"
+	pickleStepID := "pickleStepID"
+	match := &models.StepDefinition{}
+	attachments := make([]models.PickleAttachment, 0)
+	err := fmt.Errorf("intentional")
+
+	results := models.NewStepResult(status, pickleID, pickleStepID, match, attachments, err)
+
+	assert.Equal(t, status, results.Status)
+	assert.Equal(t, pickleID, results.PickleID)
+	assert.Equal(t, pickleStepID, results.PickleStepID)
+	assert.Equal(t, match, results.Def)
+	assert.Equal(t, attachments, results.Attachments)
+	assert.Equal(t, err, results.Err)
+}

suite.go

@@ -21,6 +21,9 @@ var (
 	contextInterface = reflect.TypeOf((*context.Context)(nil)).Elem()
 )
 
+// more than one regex matched the step text
+var ErrAmbiguous = fmt.Errorf("ambiguous step definition")
+
 // ErrUndefined is returned in case if step definition was not found
 var ErrUndefined = fmt.Errorf("step is undefined")
 
@@ -45,6 +48,8 @@ const (
 	StepUndefined = models.Undefined
 	// StepPending indicates step with pending implementation.
 	StepPending = models.Pending
+	// StepAmbiguous indicates step text matches more than one step def
+	StepAmbiguous = models.Ambiguous
 )
 
 type suite struct {
@@ -111,19 +116,22 @@ func pickleAttachments(ctx context.Context) []models.PickleAttachment {
 	return pickledAttachments
 }
 
-func (s *suite) matchStep(step *messages.PickleStep) *models.StepDefinition {
+func (s *suite) matchStep(step *messages.PickleStep) (*models.StepDefinition, error) {
-	def := s.matchStepTextAndType(step.Text, step.Type)
+	def, err := s.matchStepTextAndType(step.Text, step.Type)
+	if err != nil {
+		return nil, err
+	}
+
 	if def != nil && step.Argument != nil {
 		def.Args = append(def.Args, step.Argument)
 	}
-	return def
+	return def, nil
 }
 
 func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scenarioErr error, isFirst, isLast bool) (rctx context.Context, err error) {
 	var match *models.StepDefinition
 
 	rctx = ctx
-	status := StepUndefined
 
 	// user multistep definitions may panic
 	defer func() {
@@ -154,7 +162,11 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scena
 			err = getTestingT(ctx).isFailed()
 		}
 
+		status := StepUndefined
+
 		switch {
+		case errors.Is(err, ErrAmbiguous):
+			status = StepAmbiguous
 		case errors.Is(err, ErrPending):
 			status = StepPending
 		case errors.Is(err, ErrSkip), err == nil && scenarioErr != nil:
@@ -197,6 +209,10 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scena
 			sr := models.NewStepResult(models.Skipped, pickle.Id, step.Id, match, pickledAttachments, nil)
 			s.storage.MustInsertPickleStepResult(sr)
 			s.fmt.Skipped(pickle, step, match.GetInternalStepDefinition())
+		case errors.Is(err, ErrAmbiguous):
+			sr := models.NewStepResult(models.Ambiguous, pickle.Id, step.Id, match, pickledAttachments, err)
+			s.storage.MustInsertPickleStepResult(sr)
+			s.fmt.Ambiguous(pickle, step, match.GetInternalStepDefinition(), err)
 		default:
 			sr := models.NewStepResult(models.Failed, pickle.Id, step.Id, match, pickledAttachments, err)
 			s.storage.MustInsertPickleStepResult(sr)
@@ -212,7 +228,10 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scena
 	// run before step handlers
 	ctx, err = s.runBeforeStepHooks(ctx, step, err)
 
-	match = s.matchStep(step)
+	// TODO JL MOVE THIS TO XXXX
+	var matchError error
+	match, matchError = s.matchStep(step)
+
 	s.storage.MustInsertStepDefintionMatch(step.AstNodeIds[0], match)
 	s.fmt.Defined(pickle, step, match.GetInternalStepDefinition())
 
@@ -226,6 +245,11 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scena
 		return ctx, err
 	}
 
+	// XXXXX
+	if matchError != nil {
+		return ctx, matchError
+	}
+
 	if ctx, undef, err := s.maybeUndefined(ctx, step.Text, step.Argument, step.Type); err != nil {
 		return ctx, err
 	} else if len(undef) > 0 {
@@ -382,12 +406,16 @@ func (s *suite) runAfterScenarioHooks(ctx context.Context, pickle *messages.Pick
 }
 
 func (s *suite) maybeUndefined(ctx context.Context, text string, arg interface{}, stepType messages.PickleStepType) (context.Context, []string, error) {
-	step := s.matchStepTextAndType(text, stepType)
+	var undefined []string
+	step, err := s.matchStepTextAndType(text, stepType)
+	if err != nil {
+		return ctx, undefined, err
+	}
+
 	if nil == step {
 		return ctx, []string{text}, nil
 	}
 
-	var undefined []string
 	if !step.Nested {
 		return ctx, undefined, nil
 	}
@@ -430,10 +458,13 @@ func (s *suite) maybeSubSteps(ctx context.Context, result interface{}) (context.
 		return ctx, fmt.Errorf("unexpected error, should have been godog.Steps: %T - %+v", result, result)
 	}
 
-	var err error
-
 	for _, text := range steps {
-		if def := s.matchStepTextAndType(text, messages.PickleStepType_UNKNOWN); def == nil {
+		def, err := s.matchStepTextAndType(text, messages.PickleStepType_UNKNOWN)
+		if err != nil {
+			return ctx, err
+		}
+
+		if def == nil {
 			return ctx, ErrUndefined
 		} else {
 			ctx, err = s.runSubStep(ctx, text, def)
@@ -477,7 +508,10 @@ func (s *suite) runSubStep(ctx context.Context, text string, def *models.StepDef
 	return ctx, nil
 }
 
-func (s *suite) matchStepTextAndType(text string, stepType messages.PickleStepType) *models.StepDefinition {
+func (s *suite) matchStepTextAndType(text string, stepType messages.PickleStepType) (*models.StepDefinition, error) {
+	var first *models.StepDefinition
+	matchingExpressions := make([]string, 0)
+
 	for _, h := range s.steps {
 		if m := h.Expr.FindStringSubmatch(text); len(m) > 0 {
 			if !keywordMatches(h.Keyword, stepType) {
@@ -488,9 +522,11 @@ func (s *suite) matchStepTextAndType(text string, stepType messages.PickleStepTy
 				args = append(args, m)
 			}
 
+			matchingExpressions = append(matchingExpressions, h.Expr.String())
+
 			// since we need to assign arguments
 			// better to copy the step definition
-			return &models.StepDefinition{
+			match := &models.StepDefinition{
 				StepDefinition: formatters.StepDefinition{
 					Expr:    h.Expr,
 					Handler: h.Handler,
@@ -500,9 +536,22 @@ func (s *suite) matchStepTextAndType(text string, stepType messages.PickleStepTy
 				HandlerValue: h.HandlerValue,
 				Nested:       h.Nested,
 			}
+
+			if first == nil {
+				first = match
 			}
 		}
-	return nil
+	}
+
+	if s.strict {
+		if len(matchingExpressions) > 1 {
+			fmt.Printf("IS STRICT=%v\n", len(matchingExpressions))
+			errs := "\n\t\t" + strings.Join(matchingExpressions, "\n\t\t")
+			return nil, fmt.Errorf("%w, step text: %s\n\tmatches:%s", ErrAmbiguous, text, errs)
+		}
+	}
+
+	return first, nil
 }
 
 func keywordMatches(k formatters.Keyword, stepType messages.PickleStepType) bool {

suite_context_test.go

@@ -1112,7 +1112,44 @@ func TestTestSuite_Run(t *testing.T) {
 					<< After scenario "test", error: <nil>
 					<<<< After suite`,
 		},
+		{
+			name: "ambiguous", afterStepCnt: 1, beforeStepCnt: 1,
+			body: `
+					Then step is ambiguous`,
+			log: `
+					>>>> Before suite
+					>> Before scenario "test"
+					Before step "step is ambiguous"
+					After step "step is ambiguous", error: ambiguous step definition, step text: step is ambiguous
+		        	            		matches:
+		        	            			^step is ambiguous$
+		        	            			^step is ambiguous$, status: ambiguous
+					<< After scenario "test", error: ambiguous step definition, step text: step is ambiguous
+		        	            		matches:
+		        	            			^step is ambiguous$
+		        	            			^step is ambiguous$
+					<<<< After suite`,
+		},
+		{
+			name: "ambiguous nested steps", afterStepCnt: 1, beforeStepCnt: 1,
+			body: `
+				Then multistep has ambiguous`,
+			log: `
+				>>>> Before suite
+				>> Before scenario "test"
+				Before step "multistep has ambiguous"
+				After step "multistep has ambiguous", error: ambiguous step definition, step text: step is ambiguous
+            	            		matches:
+            	            			^step is ambiguous$
+            	            			^step is ambiguous$, status: ambiguous
+				<< After scenario "test", error: ambiguous step definition, step text: step is ambiguous
+            	            		matches:
+            	            			^step is ambiguous$
+            	            			^step is ambiguous$
+				<<<< After suite`,
+		},
 	} {
+		// JL
 		t.Run(tc.name, func(t *testing.T) {
 			afterScenarioCnt := 0
 			beforeScenarioCnt := 0
@@ -1180,6 +1217,17 @@ func TestTestSuite_Run(t *testing.T) {
 					s.Step("pending", func() error {
 						return ErrPending
 					})
+
+					s.Step("^step is ambiguous$", func() {
+						log += "<step action>\n"
+					})
+					s.Step("^step is ambiguous$", func() {
+						log += "<step action>\n"
+					})
+					s.Step("^multistep has ambiguous$", func() Steps {
+						return Steps{"step is ambiguous"}
+					})
+
 				},
 				Options: &Options{
 					Format:   "pretty",