Additional code review observations on Attach() functionality from https://github.com/cucumber/godog/pull/623 (#628)

* support multiple calls to the Attach() function from a single step

* run_progress_test.go changed so it's not sensitive to the name of the clone target directory

* applied code review comments also added _example/attachments

* applied code review comments also added _example/attachments

* applied code review comments also added _example/attachments

CHANGELOG.md

@@ -8,7 +8,7 @@ This document is formatted according to the principles of [Keep A CHANGELOG](htt
 
 ## Unreleased
 
-- Provide support for attachments / embeddings  - ([623](https://github.com/cucumber/godog/pull/623) - [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]
 

README.md

@@ -292,6 +292,10 @@ When steps are orthogonal and small, you can combine them just like you do with
 
 `TestFeatures` acts as a regular Go test, so you can leverage your IDE facilities to run and debug it.
 
+### Attachments
+
+An example showing how to make attachments (aka embeddings) to the results is shown in [_examples/attachments](/_examples/attachments/)
+
 ## Code of Conduct
 
 Everyone interacting in this codebase and issue tracker is expected to follow the Cucumber [code of conduct](https://github.com/cucumber/cucumber/blob/master/CODE_OF_CONDUCT.md).

_examples/attachments/README.md

@@ -0,0 +1,16 @@
+# An example of Making attachments to the reports
+
+The JSON (and in future NDJSON) report formats allow the inclusion of data attachments.
+
+These attachments could be console logs or file data or images for instance.
+
+The example in this directory shows how the godog API is used to add attachments to the JSON report.
+
+
+## Run the example
+
+You must use the '-v' flag or you will not see the cucumber JSON output.
+
+go test -v atttachment_test.go
+
+

_examples/attachments/attachments_test.go

@@ -0,0 +1,68 @@
+package attachments_test
+
+// This example shows how to set up test suite runner with Go subtests and godog command line parameters.
+// Sample commands:
+// * run all scenarios from default directory (features): go test -test.run "^TestFeatures/"
+// * run all scenarios and list subtest names: go test -test.v -test.run "^TestFeatures/"
+// * run all scenarios from one feature file: go test -test.v -godog.paths features/nodogs.feature -test.run "^TestFeatures/"
+// * run all scenarios from multiple feature files: go test -test.v -godog.paths features/nodogs.feature,features/godogs.feature -test.run "^TestFeatures/"
+// * run single scenario as a subtest: go test -test.v -test.run "^TestFeatures/Eat_5_out_of_12$"
+// * show usage help: go test -godog.help
+// * show usage help if there were other test files in directory: go test -godog.help godogs_test.go
+// * run scenarios with multiple formatters: go test -test.v -godog.format cucumber:cuc.json,pretty -test.run "^TestFeatures/"
+
+import (
+	"context"
+	"os"
+	"testing"
+
+	"github.com/cucumber/godog"
+	"github.com/cucumber/godog/colors"
+)
+
+var opts = godog.Options{
+	Output: colors.Colored(os.Stdout),
+	Format: "cucumber", // cucumber json format
+}
+
+func TestFeatures(t *testing.T) {
+	o := opts
+	o.TestingT = t
+
+	status := godog.TestSuite{
+		Name:                "attachments",
+		Options:             &o,
+		ScenarioInitializer: InitializeScenario,
+	}.Run()
+
+	if status == 2 {
+		t.SkipNow()
+	}
+
+	if status != 0 {
+		t.Fatalf("zero status code expected, %d received", status)
+	}
+}
+
+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"},
+		)
+		ctx = godog.Attach(ctx,
+			godog.Attachment{Body: []byte("{ \"a\" : 1 }"), FileName: "Json Attachment", 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("TheData2"), FileName: "Data Attachment 2", MediaType: "text/plain"},
+		)
+
+		return ctx, nil
+	})
+}

_examples/attachments/features/attachments.feature

@@ -0,0 +1,7 @@
+Feature: Attaching content to the cucumber report
+  The cucumber JSON and NDJSON support the inclusion of attachments.
+  These can be text or images or any data really. 
+
+  Scenario: Attaching files to the report
+    Given I have attached two documents in sequence 
+    And I have attached two documents at once

internal/formatters/fmt_cucumber.go

@@ -154,7 +154,7 @@ type cukeStep struct {
 	Match      cukeMatch           `json:"match"`
 	Result     cukeResult          `json:"result"`
 	DataTable  []*cukeDataTableRow `json:"rows,omitempty"`
-	Embeddings []*cukeEmbedding    `json:"embeddings,omitempty"`
+	Embeddings []cukeEmbedding     `json:"embeddings,omitempty"`
 }
 
 type cukeDataTableRow struct {
@@ -303,10 +303,10 @@ func (f *Cuke) buildCukeStep(pickle *messages.Pickle, stepResult models.PickleSt
 	}
 
 	if stepResult.Attachments != nil {
-		attachments := []*cukeEmbedding{}
+		attachments := []cukeEmbedding{}
 
 		for _, a := range stepResult.Attachments {
-			attachments = append(attachments, &cukeEmbedding{
+			attachments = append(attachments, cukeEmbedding{
 				Name:     a.Name,
 				Data:     base64.RawStdEncoding.EncodeToString(a.Data),
 				MimeType: a.MimeType,

internal/formatters/fmt_output_test.go

@@ -19,7 +19,10 @@ import (
 
 const fmtOutputTestsFeatureDir = "formatter-tests/features"
 
+var tT *testing.T
+
 func Test_FmtOutput(t *testing.T) {
+	tT = t
 	pkg := os.Getenv("GODOG_TESTED_PACKAGE")
 	os.Setenv("GODOG_TESTED_PACKAGE", "github.com/cucumber/godog")
 
@@ -64,7 +67,8 @@ func fmtOutputTest(fmtName, testName, featureFilePath string) func(*testing.T) {
 		ctx.Step(`^(?:a )?pending step$`, pendingStepDef)
 		ctx.Step(`^(?:a )?passing step$`, passingStepDef)
 		ctx.Step(`^odd (\d+) and even (\d+) number$`, oddEvenStepDef)
-		ctx.Step(`^(?:a )?a step with attachment$`, stepWithAttachment)
+		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) {
@@ -127,11 +131,29 @@ func pendingStepDef() error { return godog.ErrPending }
 
 func failingStepDef() error { return fmt.Errorf("step failed") }
 
-func stepWithAttachment(ctx context.Context) (context.Context, error) {
+func stepWithSingleAttachmentCall(ctx context.Context) (context.Context, error) {
-	ctxOut := godog.Attach(ctx,
+	if len(godog.Attachments(ctx)) > 0 {
+		assert.FailNow(tT, "Unexpected Attachments found - should have been empty")
+	}
+
+	ctx = godog.Attach(ctx,
 		godog.Attachment{Body: []byte("TheData1"), FileName: "TheFilename1", MediaType: "text/plain"},
 		godog.Attachment{Body: []byte("TheData2"), FileName: "TheFilename2", MediaType: "text/plain"},
 	)
 
-	return ctxOut, nil
+	return ctx, nil
+}
+func stepWithMultipleAttachmentCalls(ctx context.Context) (context.Context, error) {
+	if len(godog.Attachments(ctx)) > 0 {
+		assert.FailNow(tT, "Unexpected Attachments found - should have been empty")
+	}
+
+	ctx = godog.Attach(ctx,
+		godog.Attachment{Body: []byte("TheData1"), FileName: "TheFilename1", MediaType: "text/plain"},
+	)
+	ctx = godog.Attach(ctx,
+		godog.Attachment{Body: []byte("TheData2"), FileName: "TheFilename2", MediaType: "text/plain"},
+	)
+
+	return ctx, nil
 }

internal/formatters/formatter-tests/cucumber/scenario_with_attachment

@@ -17,7 +17,7 @@
                 "steps": [
                     {
                         "keyword": "Given ",
-                        "name": "a step with attachment",
+                        "name": "a step with a single attachment call for multiple attachments",
                         "line": 7,
                         "match": {
                             "location": "fmt_output_test.go:119"
@@ -38,6 +38,30 @@
                                 "data": "VGhlRGF0YTI"
                             }
                         ]
+                    },
+                    {
+                        "keyword": "And ",
+                        "name": "a step with multiple attachment calls",
+                        "line": 8,
+                        "match": {
+                            "location": "fmt_output_test.go:119"
+                        },
+                        "result": {
+                            "status": "passed",
+                            "duration": 0
+                        },
+                        "embeddings": [
+                            {
+                                "name": "TheFilename1",
+                                "mime_type": "text/plain",
+                                "data": "VGhlRGF0YTE"
+                            },
+                            {
+                                "name": "TheFilename2",
+                                "mime_type": "text/plain",
+                                "data": "VGhlRGF0YTI"
+                            }
+                        ]
                     }
                 ]
             }

internal/formatters/formatter-tests/events/scenario_with_attachment

@@ -1,10 +1,15 @@
 {"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 attachment\n"}
+{"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":"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.stepWithAttachment","arguments":[]}
+{"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":"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":"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":"TheFilename1","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":"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

@@ -4,4 +4,5 @@ Feature: scenario with attachment
   feature
 
   Scenario: step with attachment
-    Given a step with attachment
+    Given a step with a single attachment call for multiple attachments
+    And a step with multiple attachment calls

internal/models/results.go

@@ -36,7 +36,7 @@ type PickleStepResult struct {
 
 	Def *StepDefinition
 
-	Attachments []*PickleAttachment
+	Attachments []PickleAttachment
 }
 
 // NewStepResult ...
@@ -44,7 +44,7 @@ func NewStepResult(
 	status StepResultStatus,
 	pickleID, pickleStepID string,
 	match *StepDefinition,
-	attachments []*PickleAttachment,
+	attachments []PickleAttachment,
 	err error,
 ) PickleStepResult {
 	return PickleStepResult{

run_progress_test.go

@@ -56,7 +56,7 @@ func Test_ProgressFormatterWhenStepPanics(t *testing.T) {
 	require.True(t, failed)
 
 	actual := buf.String()
-	assert.Contains(t, actual, "godog/run_progress_test.go:")
+	assert.Contains(t, actual, "run_progress_test.go:")
 }
 
 func Test_ProgressFormatterWithPanicInMultistep(t *testing.T) {

suite.go

@@ -77,8 +77,11 @@ type Attachment struct {
 type attachmentKey struct{}
 
 func Attach(ctx context.Context, attachments ...Attachment) context.Context {
-	return context.WithValue(ctx, attachmentKey{}, attachments)
+	existing := Attachments(ctx)
+	updated := append(existing, attachments...)
+	return context.WithValue(ctx, attachmentKey{}, updated)
 }
+
 func Attachments(ctx context.Context) []Attachment {
 	v := ctx.Value(attachmentKey{})
 
@@ -88,13 +91,17 @@ func Attachments(ctx context.Context) []Attachment {
 	return v.([]Attachment)
 }
 
-func pickleAttachments(ctx context.Context) []*models.PickleAttachment {
+func clearAttach(ctx context.Context) context.Context {
+	return context.WithValue(ctx, attachmentKey{}, nil)
+}
 
-	pickledAttachments := []*models.PickleAttachment{}
+func pickleAttachments(ctx context.Context) []models.PickleAttachment {
+
+	pickledAttachments := []models.PickleAttachment{}
 	attachments := Attachments(ctx)
 
 	for _, a := range attachments {
-		pickledAttachments = append(pickledAttachments, &models.PickleAttachment{
+		pickledAttachments = append(pickledAttachments, models.PickleAttachment{
 			Name:     a.FileName,
 			Data:     a.Body,
 			MimeType: a.MediaType,
@@ -161,7 +168,7 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scena
 		}
 
 		pickledAttachments := pickleAttachments(ctx)
-		ctx = Attach(ctx)
+		ctx = clearAttach(ctx)
 
 		// Run after step handlers.
 		rctx, err = s.runAfterStepHooks(ctx, step, status, err)
@@ -212,7 +219,7 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scena
 	if err != nil {
 
 		pickledAttachments := pickleAttachments(ctx)
-		ctx = Attach(ctx)
+		ctx = clearAttach(ctx)
 
 		sr := models.NewStepResult(models.Failed, pickle.Id, step.Id, match, pickledAttachments, nil)
 		s.storage.MustInsertPickleStepResult(sr)
@@ -237,7 +244,7 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scena
 		}
 
 		pickledAttachments := pickleAttachments(ctx)
-		ctx = Attach(ctx)
+		ctx = clearAttach(ctx)
 
 		sr := models.NewStepResult(models.Undefined, pickle.Id, step.Id, match, pickledAttachments, nil)
 		s.storage.MustInsertPickleStepResult(sr)
@@ -248,7 +255,7 @@ func (s *suite) runStep(ctx context.Context, pickle *Scenario, step *Step, scena
 
 	if scenarioErr != nil {
 		pickledAttachments := pickleAttachments(ctx)
-		ctx = Attach(ctx)
+		ctx = clearAttach(ctx)
 
 		sr := models.NewStepResult(models.Skipped, pickle.Id, step.Id, match, pickledAttachments, nil)
 		s.storage.MustInsertPickleStepResult(sr)