Bugfix afterscenario attachments (#646)

* Minor doc and comment corrections

* Fixed bug where it was impossible to make attachments from 'after scenario' hook, also removed some dud comments.

* typo

CHANGELOG.md

@@ -8,7 +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))
+- 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))
 
 ## [v0.14.1]

_examples/attachments/attachments_test.go

@@ -1,6 +1,8 @@
 package attachments_test
 
-// This example shows how to attach data to the cucumber reports
+// This "demo" doesn't actually get run as a test by the build.
+
+// This "example" shows how to attach data to the cucumber reports
 // Run the sample with : go test -v attachments_test.go
 // Then review the "embeddings" within the JSON emitted on the console.
 
@@ -39,15 +41,28 @@ func TestFeatures(t *testing.T) {
 
 func InitializeScenario(ctx *godog.ScenarioContext) {
 
+	ctx.Before(func(ctx context.Context, sc *godog.Scenario) (context.Context, error) {
+		ctx = godog.Attach(ctx,
+			godog.Attachment{Body: []byte("BeforeScenarioAttachment"), FileName: "Step Attachment 1", MediaType: "text/plain"},
+		)
+		return ctx, nil
+	})
+	ctx.After(func(ctx context.Context, sc *godog.Scenario, err error) (context.Context, error) {
+		ctx = godog.Attach(ctx,
+			godog.Attachment{Body: []byte("AfterScenarioAttachment"), FileName: "Step Attachment 2", MediaType: "text/plain"},
+		)
+		return ctx, nil
+	})
+
 	ctx.StepContext().Before(func(ctx context.Context, st *godog.Step) (context.Context, error) {
 		ctx = godog.Attach(ctx,
-			godog.Attachment{Body: []byte("BeforeStepAttachment"), FileName: "Data Attachment", MediaType: "text/plain"},
+			godog.Attachment{Body: []byte("BeforeStepAttachment"), FileName: "Step Attachment 3", MediaType: "text/plain"},
 		)
 		return ctx, nil
 	})
 	ctx.StepContext().After(func(ctx context.Context, st *godog.Step, status godog.StepResultStatus, err error) (context.Context, error) {
 		ctx = godog.Attach(ctx,
-			godog.Attachment{Body: []byte("AfterStepAttachment"), FileName: "Data Attachment", MediaType: "text/plain"},
+			godog.Attachment{Body: []byte("AfterStepAttachment"), FileName: "Step Attachment 4", MediaType: "text/plain"},
 		)
 		return ctx, nil
 	})
@@ -55,18 +70,18 @@ func InitializeScenario(ctx *godog.ScenarioContext) {
 	ctx.Step(`^I have attached two documents in sequence$`, func(ctx context.Context) (context.Context, error) {
 		// the attached bytes will be base64 encoded by the framework and placed in the embeddings section of the cuke report
 		ctx = godog.Attach(ctx,
-			godog.Attachment{Body: []byte("TheData1"), FileName: "Data Attachment", MediaType: "text/plain"},
+			godog.Attachment{Body: []byte("TheData1"), FileName: "Step Attachment 5", MediaType: "text/plain"},
 		)
 		ctx = godog.Attach(ctx,
-			godog.Attachment{Body: []byte("{ \"a\" : 1 }"), FileName: "Json Attachment", MediaType: "application/json"},
+			godog.Attachment{Body: []byte("{ \"a\" : 1 }"), FileName: "Step Attachment 6", MediaType: "application/json"},
 		)
 
 		return ctx, nil
 	})
 	ctx.Step(`^I have attached two documents at once$`, func(ctx context.Context) (context.Context, error) {
 		ctx = godog.Attach(ctx,
-			godog.Attachment{Body: []byte("TheData1"), FileName: "Data Attachment 1", MediaType: "text/plain"},
+			godog.Attachment{Body: []byte("TheData1"), FileName: "Step Attachment 7", MediaType: "text/plain"},
-			godog.Attachment{Body: []byte("TheData2"), FileName: "Data Attachment 2", MediaType: "text/plain"},
+			godog.Attachment{Body: []byte("TheData2"), FileName: "Step Attachment 8", MediaType: "text/plain"},
 		)
 
 		return ctx, nil

internal/formatters/fmt_output_test.go

@@ -63,31 +63,79 @@ func listFmtOutputTestsFeatureFiles() (featureFiles []string, err error) {
 
 func fmtOutputTest(fmtName, testName, featureFilePath string) func(*testing.T) {
 	fmtOutputScenarioInitializer := func(ctx *godog.ScenarioContext) {
-
+		stepIndex := 0
-		ctx.StepContext().Before(func(ctx context.Context, st *godog.Step) (context.Context, error) {
+		ctx.Before(func(ctx context.Context, sc *godog.Scenario) (context.Context, error) {
+			if strings.Contains(sc.Name, "attachment") {
 				att := godog.Attachments(ctx)
 				attCount := len(att)
-			if attCount > 0 {
+				if attCount != 0 {
-				assert.FailNow(tT, fmt.Sprintf("Unexpected Attachments found - should have been empty, found %d\n%+v", attCount, att))
+					assert.FailNowf(tT, "Unexpected attachments: "+sc.Name, "should have been empty, found %d", attCount)
 				}
 
-			if st.Text == "a step with multiple attachment calls" {
 				ctx = godog.Attach(ctx,
-					godog.Attachment{Body: []byte("BeforeStepAttachment"), FileName: "Data Attachment", MediaType: "text/plain"},
+					godog.Attachment{Body: []byte("BeforeScenarioAttachment"), FileName: "Before Scenario Attachment 1", MediaType: "text/plain"},
+				)
+			}
+			return ctx, nil
+		})
+
+		ctx.After(func(ctx context.Context, sc *godog.Scenario, err error) (context.Context, error) {
+
+			if strings.Contains(sc.Name, "attachment") {
+				att := godog.Attachments(ctx)
+				attCount := len(att)
+				if attCount != 4 {
+					assert.FailNow(tT, "Unexpected attachements: "+sc.Name, "expected 4, found %d", attCount)
+				}
+				ctx = godog.Attach(ctx,
+					godog.Attachment{Body: []byte("AfterScenarioAttachment"), FileName: "After Scenario Attachment 2", MediaType: "text/plain"},
+				)
+			}
+			return ctx, nil
+		})
+
+		ctx.StepContext().Before(func(ctx context.Context, st *godog.Step) (context.Context, error) {
+			stepIndex++
+
+			if strings.Contains(st.Text, "attachment") {
+				att := godog.Attachments(ctx)
+				attCount := len(att)
+
+				// 1 for before scenario ONLY if this is the 1st step
+				expectedAttCount := 0
+				if stepIndex == 1 {
+					expectedAttCount = 1
+				}
+
+				if attCount != expectedAttCount {
+					assert.FailNow(tT, "Unexpected attachments: "+st.Text, "expected 1, found %d\n%+v", attCount, att)
+				}
+				ctx = godog.Attach(ctx,
+					godog.Attachment{Body: []byte("BeforeStepAttachment"), FileName: "Before Step Attachment 3", MediaType: "text/plain"},
 				)
 			}
 			return ctx, nil
 		})
 		ctx.StepContext().After(func(ctx context.Context, st *godog.Step, status godog.StepResultStatus, err error) (context.Context, error) {
 
-			if st.Text == "a step with multiple attachment calls" {
+			if strings.Contains(st.Text, "attachment") {
 				att := godog.Attachments(ctx)
 				attCount := len(att)
-				if attCount != 3 {
+
-					assert.FailNow(tT, fmt.Sprintf("Expected 3 Attachments - 1 from the before step and 2 from the step, found %d\n%+v", attCount, att))
+				// 1 for before scenario ONLY if this is the 1st step
+				// 1 for before before step
+				// 2 from from step
+				expectedAttCount := 3
+				if stepIndex == 1 {
+					expectedAttCount = 4
+				}
+
+				if attCount != expectedAttCount {
+					// 1 from before scenario, 1 from before step, 1 from step
+					assert.FailNow(tT, "Unexpected attachments: "+st.Text, "expected 4, found %d", attCount)
 				}
 				ctx = godog.Attach(ctx,
-					godog.Attachment{Body: []byte("AfterStepAttachment"), FileName: "Data Attachment", MediaType: "text/plain"},
+					godog.Attachment{Body: []byte("AfterStepAttachment"), FileName: "After Step Attachment 4", MediaType: "text/plain"},
 				)
 			}
 			return ctx, nil
@@ -105,7 +153,8 @@ func fmtOutputTest(fmtName, testName, featureFilePath string) func(*testing.T) {
 		expectOutputPath := strings.Replace(featureFilePath, "features", fmtName, 1)
 		expectOutputPath = strings.TrimSuffix(expectOutputPath, path.Ext(expectOutputPath))
 		if _, err := os.Stat(expectOutputPath); err != nil {
-			t.Skipf("Couldn't find expected output file %q", expectOutputPath)
+			// the test author needs to write an "expected output" file for any formats they want the test feature to be verified against
+			t.Skipf("Skipping test for feature '%v' for format '%v', because no 'expected output' file %q", featureFilePath, fmtName, expectOutputPath)
 		}
 
 		expectedOutput, err := os.ReadFile(expectOutputPath)
@@ -130,6 +179,9 @@ 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")
+		}
 	}
 }
 
@@ -162,8 +214,10 @@ func pendingStepDef() error { return godog.ErrPending }
 func failingStepDef() error { return fmt.Errorf("step failed") }
 
 func stepWithSingleAttachmentCall(ctx context.Context) (context.Context, error) {
-	if len(godog.Attachments(ctx)) > 0 {
+	aCount := len(godog.Attachments(ctx))
-		assert.FailNow(tT, "Unexpected Attachments found - should have been empty")
+	if aCount != 2 {
+		// 1 from before scenario, 1 from before step
+		assert.FailNowf(tT, "Unexpected Attachments found", "should have been 2, but found %v", aCount)
 	}
 
 	ctx = godog.Attach(ctx,
@@ -174,15 +228,16 @@ func stepWithSingleAttachmentCall(ctx context.Context) (context.Context, error)
 	return ctx, nil
 }
 func stepWithMultipleAttachmentCalls(ctx context.Context) (context.Context, error) {
-	if len(godog.Attachments(ctx)) != 1 {
+	aCount := len(godog.Attachments(ctx))
-		assert.FailNow(tT, "Expected 1 Attachment that should have been inserted by before step")
+	if aCount != 1 {
+		assert.FailNowf(tT, "Unexpected Attachments found", "Expected 1 Attachment, but found %v", aCount)
 	}
 
 	ctx = godog.Attach(ctx,
-		godog.Attachment{Body: []byte("TheData1"), FileName: "TheFilename1", MediaType: "text/plain"},
+		godog.Attachment{Body: []byte("TheData1"), FileName: "TheFilename3", MediaType: "text/plain"},
 	)
 	ctx = godog.Attach(ctx,
-		godog.Attachment{Body: []byte("TheData2"), FileName: "TheFilename2", MediaType: "text/plain"},
+		godog.Attachment{Body: []byte("TheData2"), FileName: "TheFilename4", MediaType: "text/plain"},
 	)
 
 	return ctx, nil

internal/formatters/formatter-tests/cucumber/scenario_with_attachment

@@ -1,16 +1,16 @@
 [
     {
         "uri": "formatter-tests/features/scenario_with_attachment.feature",
-        "id": "scenario-with-attachment",
+        "id": "feature-with-attachment",
         "keyword": "Feature",
-        "name": "scenario with attachment",
+        "name": "feature with attachment",
         "description": "  describes\n  an attachment\n  feature",
         "line": 1,
         "elements": [
             {
-                "id": "scenario-with-attachment;step-with-attachment",
+                "id": "feature-with-attachment;scenario-with-attachment",
                 "keyword": "Scenario",
-                "name": "step with attachment",
+                "name": "scenario with attachment",
                 "description": "",
                 "line": 6,
                 "type": "scenario",
@@ -27,6 +27,16 @@
                             "duration": 0
                         },
                         "embeddings": [
+                            {
+                                "name": "Before Scenario Attachment 1",
+                                "mime_type": "text/plain",
+                                "data": "QmVmb3JlU2NlbmFyaW9BdHRhY2htZW50"
+                            },
+                            {
+                                "name": "Before Step Attachment 3",
+                                "mime_type": "text/plain",
+                                "data": "QmVmb3JlU3RlcEF0dGFjaG1lbnQ="
+                            },
                             {
                                 "name": "TheFilename1",
                                 "mime_type": "text/plain",
@@ -36,6 +46,11 @@
                                 "name": "TheFilename2",
                                 "mime_type": "text/plain",
                                 "data": "VGhlRGF0YTI="
+                            },
+                            {
+                                "name": "After Step Attachment 4",
+                                "mime_type": "text/plain",
+                                "data": "QWZ0ZXJTdGVwQXR0YWNobWVudA=="
                             }
                         ]
                     },
@@ -52,24 +67,29 @@
                         },
                         "embeddings": [
                             {
-                                "name": "Data Attachment",
+                                "name": "Before Step Attachment 3",
                                 "mime_type": "text/plain",
                                 "data": "QmVmb3JlU3RlcEF0dGFjaG1lbnQ="
                             },
                             {
-                                "name": "TheFilename1",
+                                "name": "TheFilename3",
                                 "mime_type": "text/plain",
                                 "data": "VGhlRGF0YTE="
                             },
                             {
-                                "name": "TheFilename2",
+                                "name": "TheFilename4",
                                 "mime_type": "text/plain",
                                 "data": "VGhlRGF0YTI="
                             },
                             {
-                                "name": "Data Attachment",
+                                "name": "After Step Attachment 4",
                                 "mime_type": "text/plain",
                                 "data": "QWZ0ZXJTdGVwQXR0YWNobWVudA=="
+                            },
+                            {
+                                "name": "After Scenario Attachment 2",
+                                "mime_type": "text/plain",
+                                "data": "QWZ0ZXJTY2VuYXJpb0F0dGFjaG1lbnQ="
                             }
                         ]
                     }

internal/formatters/formatter-tests/events/scenario_with_attachment

@@ -1,17 +1,21 @@
 {"event":"TestRunStarted","version":"0.1.0","timestamp":-6795364578871,"suite":"events"}
-{"event":"TestSource","location":"formatter-tests/features/scenario_with_attachment.feature:1","source":"Feature: scenario with attachment\n  describes\n  an attachment\n  feature\n\n  Scenario: step with attachment\n    Given a step with a single attachment call for multiple attachments\n    And a step with multiple attachment calls\n"}
+{"event":"TestSource","location":"formatter-tests/features/scenario_with_attachment.feature:1","source":"Feature: feature with attachment\n  describes\n  an attachment\n  feature\n\n  Scenario: scenario with attachment\n    Given a step with a single attachment call for multiple attachments\n    And a step with multiple attachment calls\n"}
 {"event":"TestCaseStarted","location":"formatter-tests/features/scenario_with_attachment.feature:6","timestamp":-6795364578871}
 {"event":"StepDefinitionFound","location":"formatter-tests/features/scenario_with_attachment.feature:7","definition_id":"fmt_output_test.go:XXX -\u003e github.com/cucumber/godog/internal/formatters_test.stepWithSingleAttachmentCall","arguments":[]}
 {"event":"TestStepStarted","location":"formatter-tests/features/scenario_with_attachment.feature:7","timestamp":-6795364578871}
+{"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:7","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"Before Scenario Attachment 1","mimeType":"text/plain","body":"BeforeScenarioAttachment"}
+{"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:7","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"Before Step Attachment 3","mimeType":"text/plain","body":"BeforeStepAttachment"}
 {"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:7","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"TheFilename1","mimeType":"text/plain","body":"TheData1"}
 {"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:7","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"TheFilename2","mimeType":"text/plain","body":"TheData2"}
+{"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:7","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"After Step Attachment 4","mimeType":"text/plain","body":"AfterStepAttachment"}
 {"event":"TestStepFinished","location":"formatter-tests/features/scenario_with_attachment.feature:7","timestamp":-6795364578871,"status":"passed"}
 {"event":"StepDefinitionFound","location":"formatter-tests/features/scenario_with_attachment.feature:8","definition_id":"fmt_output_test.go:XXX -\u003e github.com/cucumber/godog/internal/formatters_test.stepWithMultipleAttachmentCalls","arguments":[]}
 {"event":"TestStepStarted","location":"formatter-tests/features/scenario_with_attachment.feature:8","timestamp":-6795364578871}
-{"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:8","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"Data Attachment","mimeType":"text/plain","body":"BeforeStepAttachment"}
+{"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:8","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"Before Step Attachment 3","mimeType":"text/plain","body":"BeforeStepAttachment"}
-{"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:8","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"TheFilename1","mimeType":"text/plain","body":"TheData1"}
+{"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:8","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"TheFilename3","mimeType":"text/plain","body":"TheData1"}
-{"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:8","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"TheFilename2","mimeType":"text/plain","body":"TheData2"}
+{"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:8","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"TheFilename4","mimeType":"text/plain","body":"TheData2"}
-{"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:8","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"Data Attachment","mimeType":"text/plain","body":"AfterStepAttachment"}
+{"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:8","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"After Step Attachment 4","mimeType":"text/plain","body":"AfterStepAttachment"}
+{"event":"Attachment","location":"formatter-tests/features/scenario_with_attachment.feature:8","timestamp":-6795364578871,"contentEncoding":"BASE64","fileName":"After Scenario Attachment 2","mimeType":"text/plain","body":"AfterScenarioAttachment"}
 {"event":"TestStepFinished","location":"formatter-tests/features/scenario_with_attachment.feature:8","timestamp":-6795364578871,"status":"passed"}
 {"event":"TestCaseFinished","location":"formatter-tests/features/scenario_with_attachment.feature:6","timestamp":-6795364578871,"status":"passed"}
 {"event":"TestRunFinished","status":"passed","timestamp":-6795364578871,"snippets":"","memory":""}

internal/formatters/formatter-tests/features/scenario_with_attachment.feature

@@ -1,8 +1,8 @@
-Feature: scenario with attachment
+Feature: feature with attachment
   describes
   an attachment
   feature
 
-  Scenario: step with attachment
+  Scenario: scenario with attachment
     Given a step with a single attachment call for multiple attachments
     And a step with multiple attachment calls

suite.go

@@ -182,15 +182,15 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scena
 		// Run after step handlers.
 		rctx, err = s.runAfterStepHooks(ctx, step, status, err)
 
-		// extract any accumulated attachments and clear them
-		pickledAttachments := pickleAttachments(rctx)
-		rctx = clearAttach(rctx)
-
 		// Trigger after scenario on failing or last step to attach possible hook error to step.
 		if !s.shouldFail(scenarioErr) && (isLast || s.shouldFail(err)) {
 			rctx, err = s.runAfterScenarioHooks(rctx, pickle, err)
 		}
 
+		// extract any accumulated attachments and clear them
+		pickledAttachments := pickleAttachments(rctx)
+		rctx = clearAttach(rctx)
+
 		if earlyReturn {
 			return
 		}
@@ -227,7 +227,6 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scena
 	// run before step handlers
 	ctx, err = s.runBeforeStepHooks(ctx, step, err)
 
-	// TODO JL MOVE THIS TO XXXX
 	var matchError error
 	match, matchError = s.matchStep(step)
 
@@ -235,7 +234,6 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scena
 	s.fmt.Defined(pickle, step, match.GetInternalStepDefinition())
 
 	if err != nil {
-
 		pickledAttachments := pickleAttachments(ctx)
 		ctx = clearAttach(ctx)
 
@@ -244,7 +242,6 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scena
 		return ctx, err
 	}
 
-	// XXXXX
 	if matchError != nil {
 		return ctx, matchError
 	}