Ambiguous step detection - add support to all formatters (#648)

* added the missing impl of json/events/junit/pretty - still need 'progress' and 'junit,pretty'

* added tests for "progress formatter"

* switched from tabs to spaces in the ambiguous steps error message

* rename some_scenarions_including_failing
to     some_scenarios_including_failing

* changelog

Author: Johnlon

CHANGELOG.md

@@ -9,7 +9,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))
+- Ambiguous step definitions will now be detected when strict mode is activated - ([636](https://github.com/cucumber/godog/pull/636)/([648](https://github.com/cucumber/godog/pull/648) - [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]

internal/formatters/fmt.go

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

internal/formatters/fmt_base.go

@@ -92,7 +92,7 @@ func (f *Base) Ambiguous(*messages.Pickle, *messages.PickleStep, *formatters.Ste
 // Summary renders summary information.
 func (f *Base) Summary() {
 	var totalSc, passedSc, undefinedSc int
-	var totalSt, passedSt, failedSt, skippedSt, pendingSt, undefinedSt int
+	var totalSt, passedSt, failedSt, skippedSt, pendingSt, undefinedSt, ambiguousSt int
 
 	pickleResults := f.Storage.MustGetPickleResults()
 	for _, pr := range pickleResults {
@@ -114,6 +114,9 @@ func (f *Base) Summary() {
 			case failed:
 				prStatus = failed
 				failedSt++
+			case ambiguous:
+				prStatus = ambiguous
+				ambiguousSt++
 			case skipped:
 				skippedSt++
 			case undefined:
@@ -144,6 +147,10 @@ func (f *Base) Summary() {
 		parts = append(parts, yellow(fmt.Sprintf("%d pending", pendingSt)))
 		steps = append(steps, yellow(fmt.Sprintf("%d pending", pendingSt)))
 	}
+	if ambiguousSt > 0 {
+		parts = append(parts, yellow(fmt.Sprintf("%d ambiguous", ambiguousSt)))
+		steps = append(steps, yellow(fmt.Sprintf("%d ambiguous", ambiguousSt)))
+	}
 	if undefinedSt > 0 {
 		parts = append(parts, yellow(fmt.Sprintf("%d undefined", undefinedSc)))
 		steps = append(steps, yellow(fmt.Sprintf("%d undefined", undefinedSt)))

internal/formatters/fmt_cucumber.go

@@ -299,7 +299,7 @@ func (f *Cuke) buildCukeStep(pickle *messages.Pickle, stepResult models.PickleSt
 		cukeStep.Result.Error = stepResult.Err.Error()
 	}
 
-	if stepResult.Status == undefined || stepResult.Status == pending {
+	if stepResult.Status == undefined || stepResult.Status == pending || stepResult.Status == ambiguous {
 		cukeStep.Match.Location = fmt.Sprintf("%s:%d", pickle.Uri, step.Location.Line)
 	}
 

internal/formatters/fmt_events.go

@@ -198,7 +198,7 @@ func (f *Events) step(pickle *messages.Pickle, pickleStep *messages.PickleStep)
 		pickleStepResults := f.Storage.MustGetPickleStepResultsByPickleID(pickle.Id)
 		for _, stepResult := range pickleStepResults {
 			switch stepResult.Status {
-			case passed, failed, undefined, pending:
+			case passed, failed, undefined, pending, ambiguous:
 				status = stepResult.Status.String()
 			}
 		}
@@ -318,6 +318,16 @@ func (f *Events) Pending(pickle *messages.Pickle, step *messages.PickleStep, mat
 	f.step(pickle, step)
 }
 
+// Ambiguous captures ambiguous step.
+func (f *Events) Ambiguous(pickle *messages.Pickle, step *messages.PickleStep, match *formatters.StepDefinition, err error) {
+	f.Base.Ambiguous(pickle, step, match, err)
+
+	f.Lock.Lock()
+	defer f.Lock.Unlock()
+
+	f.step(pickle, step)
+}
+
 func (f *Events) scenarioLocation(pickle *messages.Pickle) string {
 	feature := f.Storage.MustGetFeature(pickle.Uri)
 	scenario := feature.FindScenario(pickle.AstNodeIds[0])

internal/formatters/fmt_junit.go

@@ -117,6 +117,12 @@ func (f *JUnit) buildJUNITPackageSuite() JunitPackageSuite {
 					tc.Failure = &junitFailure{
 						Message: fmt.Sprintf("Step %s: %s", pickleStep.Text, stepResult.Err),
 					}
+				case ambiguous:
+					tc.Status = ambiguous.String()
+					tc.Error = append(tc.Error, &junitError{
+						Type:    "ambiguous",
+						Message: fmt.Sprintf("Step %s", pickleStep.Text),
+					})
 				case skipped:
 					tc.Error = append(tc.Error, &junitError{
 						Type:    "skipped",

internal/formatters/fmt_output_test.go

@@ -85,7 +85,7 @@ func fmtOutputTest(fmtName, testName, featureFilePath string) func(*testing.T) {
 				att := godog.Attachments(ctx)
 				attCount := len(att)
 				if attCount != 4 {
-					assert.FailNow(tT, "Unexpected attachements: "+sc.Name, "expected 4, found %d", attCount)
+					assert.FailNow(tT, "Unexpected attachments: "+sc.Name, "expected 4, found %d", attCount)
 				}
 				ctx = godog.Attach(ctx,
 					godog.Attachment{Body: []byte("AfterScenarioAttachment"), FileName: "After Scenario Attachment 2", MediaType: "text/plain"},
@@ -144,12 +144,15 @@ func fmtOutputTest(fmtName, testName, featureFilePath string) func(*testing.T) {
 		ctx.Step(`^(?:a )?failing step`, failingStepDef)
 		ctx.Step(`^(?:a )?pending step$`, pendingStepDef)
 		ctx.Step(`^(?:a )?passing step$`, passingStepDef)
+		ctx.Step(`^ambiguous step.*$`, ambiguousStepDef)
+		ctx.Step(`^ambiguous step$`, ambiguousStepDef)
 		ctx.Step(`^odd (\d+) and even (\d+) number$`, oddEvenStepDef)
 		ctx.Step(`^(?:a )?a step with a single attachment call for multiple attachments$`, stepWithSingleAttachmentCall)
 		ctx.Step(`^(?:a )?a step with multiple attachment calls$`, stepWithMultipleAttachmentCalls)
 	}
 
 	return func(t *testing.T) {
+		fmt.Printf("fmt_output_test for format %10s : sample file %v\n", fmtName, featureFilePath)
 		expectOutputPath := strings.Replace(featureFilePath, "features", fmtName, 1)
 		expectOutputPath = strings.TrimSuffix(expectOutputPath, path.Ext(expectOutputPath))
 		if _, err := os.Stat(expectOutputPath); err != nil {
@@ -167,6 +170,7 @@ func fmtOutputTest(fmtName, testName, featureFilePath string) func(*testing.T) {
 			Format: fmtName,
 			Paths:  []string{featureFilePath},
 			Output: out,
+			Strict: true,
 		}
 
 		godog.TestSuite{
@@ -178,7 +182,14 @@ func fmtOutputTest(fmtName, testName, featureFilePath string) func(*testing.T) {
 		// normalise on unix line ending so expected vs actual works cross platform
 		expected := normalise(string(expectedOutput))
 		actual := normalise(buf.String())
+
 		assert.Equalf(t, expected, actual, "path: %s", expectOutputPath)
+
+		// display as a side by side listing as the output of the assert is all one line with embedded newlines and useless
+		if expected != actual {
+			fmt.Printf("Error: fmt: %s, path: %s\n", fmtName, expectOutputPath)
+			compareLists(expected, actual)
+		}
 	}
 }
 
@@ -192,9 +203,17 @@ func normalise(s string) string {
 	return normalised
 }
 
-func passingStepDef() error { return nil }
+func passingStepDef() error {
+	return nil
+}
 
-func oddEvenStepDef(odd, even int) error { return oddOrEven(odd, even) }
+func ambiguousStepDef() error {
+	return nil
+}
+
+func oddEvenStepDef(odd, even int) error {
+	return oddOrEven(odd, even)
+}
 
 func oddOrEven(odd, even int) error {
 	if odd%2 == 0 {
@@ -239,3 +258,88 @@ func stepWithMultipleAttachmentCalls(ctx context.Context) (context.Context, erro
 
 	return ctx, nil
 }
+
+// wrapString wraps a string into chunks of the given width.
+func wrapString(s string, width int) []string {
+	var result []string
+	for len(s) > width {
+		result = append(result, s[:width])
+		s = s[width:]
+	}
+	result = append(result, s)
+	return result
+}
+
+// compareLists compares two lists of strings and prints them with wrapped text.
+func compareLists(expected, actual string) {
+	list1 := strings.Split(expected, "\n")
+	list2 := strings.Split(actual, "\n")
+
+	// Get the length of the longer list
+	maxLength := len(list1)
+	if len(list2) > maxLength {
+		maxLength = len(list2)
+	}
+
+	colWid := 60
+	fmtTitle := fmt.Sprintf("%%4s: %%-%ds | %%-%ds\n", colWid+2, colWid+2)
+	fmtData := fmt.Sprintf("%%4d: %%-%ds | %%-%ds   %%s\n", colWid+2, colWid+2)
+
+	fmt.Printf(fmtTitle, "#", "expected", "actual")
+
+	for i := 0; i < maxLength; i++ {
+		var val1, val2 string
+
+		// Get the value from list1 if it exists
+		if i < len(list1) {
+			val1 = list1[i]
+		} else {
+			val1 = "N/A"
+		}
+
+		// Get the value from list2 if it exists
+		if i < len(list2) {
+			val2 = list2[i]
+		} else {
+			val2 = "N/A"
+		}
+
+		// Wrap both strings into slices of strings with fixed width
+		wrapped1 := wrapString(val1, colWid)
+		wrapped2 := wrapString(val2, colWid)
+
+		// Find the number of wrapped lines needed for the current pair
+		maxWrappedLines := len(wrapped1)
+		if len(wrapped2) > maxWrappedLines {
+			maxWrappedLines = len(wrapped2)
+		}
+
+		// Print the wrapped lines with alignment
+		for j := 0; j < maxWrappedLines; j++ {
+			var line1, line2 string
+
+			// Get the wrapped line or use an empty string if it doesn't exist
+			if j < len(wrapped1) {
+				line1 = wrapped1[j]
+			} else {
+				line1 = ""
+			}
+
+			if j < len(wrapped2) {
+				line2 = wrapped2[j]
+			} else {
+				line2 = ""
+			}
+
+			status := "same"
+			// if val1 != val2 {
+			if line1 != line2 {
+				status = "different"
+			}
+
+			delim := "¬"
+			// Print the wrapped lines with fixed-width column
+			fmt.Printf(fmtData, i+1, delim+line1+delim, delim+line2+delim, status)
+		}
+	}
+}

internal/formatters/fmt_pretty.go

@@ -114,6 +114,16 @@ func (f *Pretty) Failed(pickle *messages.Pickle, step *messages.PickleStep, matc
 	f.printStep(pickle, step)
 }
 
+// Failed captures failed step.
+func (f *Pretty) Ambiguous(pickle *messages.Pickle, step *messages.PickleStep, match *formatters.StepDefinition, err error) {
+	f.Base.Ambiguous(pickle, step, match, err)
+
+	f.Lock.Lock()
+	defer f.Lock.Unlock()
+
+	f.printStep(pickle, step)
+}
+
 // Pending captures pending step.
 func (f *Pretty) Pending(pickle *messages.Pickle, step *messages.PickleStep, match *formatters.StepDefinition) {
 	f.Base.Pending(pickle, step, match)
@@ -269,6 +279,9 @@ func (f *Pretty) printOutlineExample(pickle *messages.Pickle, backgroundSteps in
 		case result.Status == failed:
 			errorMsg = result.Err.Error()
 			clr = result.Status.Color()
+		case result.Status == ambiguous:
+			errorMsg = result.Err.Error()
+			clr = result.Status.Color()
 		case result.Status == undefined || result.Status == pending:
 			clr = result.Status.Color()
 		case result.Status == skipped && clr == nil:

internal/formatters/fmt_progress.go

@@ -98,6 +98,8 @@ func (f *Progress) step(pickleStepID string) {
 		fmt.Fprint(f.out, red("F"))
 	case undefined:
 		fmt.Fprint(f.out, yellow("U"))
+	case ambiguous:
+		fmt.Fprint(f.out, yellow("A"))
 	case pending:
 		fmt.Fprint(f.out, yellow("P"))
 	}
@@ -149,6 +151,16 @@ func (f *Progress) Failed(pickle *messages.Pickle, step *messages.PickleStep, ma
 	f.step(step.Id)
 }
 
+// Ambiguous steps.
+func (f *Progress) Ambiguous(pickle *messages.Pickle, step *messages.PickleStep, match *formatters.StepDefinition, err error) {
+	f.Base.Ambiguous(pickle, step, match, err)
+
+	f.Lock.Lock()
+	defer f.Lock.Unlock()
+
+	f.step(step.Id)
+}
+
 // Pending captures pending step.
 func (f *Progress) Pending(pickle *messages.Pickle, step *messages.PickleStep, match *formatters.StepDefinition) {
 	f.Base.Pending(pickle, step, match)

internal/formatters/formatter-tests/cucumber/some_scenarions_including_failing renamed to internal/formatters/formatter-tests/cucumber/some_scenarios_including_failing

@@ -1,6 +1,6 @@
 [
     {
-        "uri": "formatter-tests/features/some_scenarions_including_failing.feature",
+        "uri": "formatter-tests/features/some_scenarios_including_failing.feature",
         "id": "some-scenarios",
         "keyword": "Feature",
         "name": "some scenarios",
@@ -66,7 +66,7 @@
                         "name": "pending step",
                         "line": 9,
                         "match": {
-                            "location": "formatter-tests/features/some_scenarions_including_failing.feature:9"
+                            "location": "formatter-tests/features/some_scenarios_including_failing.feature:9"
                         },
                         "result": {
                             "status": "pending"
@@ -98,7 +98,7 @@
                         "name": "undefined",
                         "line": 13,
                         "match": {
-                            "location": "formatter-tests/features/some_scenarions_including_failing.feature:13"
+                            "location": "formatter-tests/features/some_scenarios_including_failing.feature:13"
                         },
                         "result": {
                             "status": "undefined"
@@ -116,6 +116,39 @@
                         }
                     }
                 ]
+            },
+            {
+                "id": "some-scenarios;ambiguous",
+                "keyword": "Scenario",
+                "name": "ambiguous",
+                "description": "",
+                "line": 16,
+                "type": "scenario",
+                "steps": [
+                    {
+                        "keyword": "When ",
+                        "name": "ambiguous step",
+                        "line": 17,
+                        "match": {
+                            "location": "formatter-tests/features/some_scenarios_including_failing.feature:17"
+                        },
+                        "result": {
+                            "status": "ambiguous",
+                            "error_message": "ambiguous step definition, step text: ambiguous step\n    matches:\n        ^ambiguous step.*$\n        ^ambiguous step$"
+                        }
+                    },
+                    {
+                        "keyword": "Then ",
+                        "name": "passing step",
+                        "line": 18,
+                        "match": {
+                            "location": "fmt_output_test.go:XXX"
+                        },
+                        "result": {
+                            "status": "skipped"
+                        }
+                    }
+                ]
             }
         ]
     }