added support for Attachments (aka Embedddings) by Johnlon · Pull Request #623 · cucumber/godog

Johnlon · 2024-05-08T23:30:05Z

🤔 What's changed?

The cucumber and events formatters support the inclusion of Attachments.

The proposed API is demonstrated in the tests as ...

func stepWithAttachment(ctx context.Context) (context.Context, error) {
    ctxOut := 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
}

⚡️ What's your motivation?

#617

Want support for Attachments from steps.
This PR proposes to use the context.Context object as a means to attach content from the step.

🏷️ What kind of change is this?

⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

This change involves minimal changes to patterns of use of godog API's.

I added tests for the cuke and events formats but the coverage isn't being recorded even on some lines I didn't modify.

fmt_output_test.go now has a normalise() fn that eliminates some extraneous diffs in the actual/expected files such as line endings and also line numbers so that the tests are easier to maintain and cross platform,

📋 Checklist:

I agree to respect and uphold the Cucumber Community Code of Conduct
I've changed the behaviour of the code
- I have added/updated tests to cover my changes.
My change requires a change to the documentation.
- I have updated the documentation accordingly.
Users should know about my change
- I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

codecov · 2024-05-08T23:33:06Z

Codecov Report

Attention: Patch coverage is 88.70968% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 79.69%. Comparing base (153db4e) to head (9de0ee7).
Report is 8 commits behind head on main.

Files	Patch %	Lines
suite.go	81.25%	5 Missing and 1 partial ⚠️
internal/models/results.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #623      +/-   ##
==========================================
- Coverage   83.21%   79.69%   -3.52%     
==========================================
  Files          28       40      +12     
  Lines        3413     3113     -300     
==========================================
- Hits         2840     2481     -359     
- Misses        458      513      +55     
- Partials      115      119       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… formats - done by sneaking the attachment into the context.Context object (but not letting that context value propagate into the next step, so it's kinda bending the context.Context purpose)

mattwynne · 2024-05-27T20:31:48Z

@Johnlon thanks for this, it looks great!

Are you able to take a look at why the build is failing? It seems to be just some noise from go fmt, maybe your editor has added some whitespace or something?

mattwynne · 2024-05-27T20:33:40Z

@Johnlon it would help the other maintainers if you could add an entry to the changelog as part of this PR. You can use this tool if you like, or just copy and adapt other existing entries.

Johnlon · 2024-05-27T21:19:36Z

@mattwynne thanks - will do.
Still confused by the coverage problems.
Any ideas?
Cheers John

Johnlon · 2024-05-27T21:20:38Z

@Johnlon thanks for this, it looks great!

Are you able to take a look at why the build is failing? It seems to be just some noise from go fmt, maybe your editor has added some whitespace or something?

At go fmt sorry .
Hmm will see if I can reverse??

Johnlon · 2024-05-28T23:04:44Z

@MattWynn Err Matt - I fixed the fmt issue and added the CHANGELOG - but it looks like I also merged to main (which wasn't my intention). Apols - please give it the once over. John

I see a put the CHANGELOG entry in the wrong please - please review this fix - #626

mattwynne · 2024-05-29T07:23:05Z

Hey @Johnlon no worries, looks like the build on main is green which is the main thing I was worried about, so all good.

As I said in the other thread, we favour Ship/Show/Ask in the Cucumber org, so feel free to merge your own PRs if you feel you've had enough feedback. Try and do it on purpose though! 😆

mattwynne · 2024-05-29T07:23:50Z

I really appreciate the work you've put into this, thanks!

vearutop

Thank you! Sorry, I'm late to the party, but probably we can consider/apply a few changes on top of this PR (with a new PR) before we tag a new version (to avoid backwards compatibility concerns).

internal/formatters/fmt_cucumber.go

suite.go

internal/models/results.go

suite.go

Johnlon · 2024-05-29T16:50:32Z

No prob happy to help
How do releases work?
I mean is there some governance or Star chamber who decide?
Or do I DIY here?
Thanks

mattwynne · 2024-05-29T20:33:57Z

First things first, before thinking about a release, if you can make another PR with the changes @vearutop has suggested, then we'll have the code in the best shape we can get it.

Johnlon · 2024-05-30T02:25:09Z

@vearutop Hi - Take a look at my responses to your suggestions.
PR raised here : #628

… (#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

added support for Attachments (aka Embedddings)

a945e9f

Johnlon added 2 commits May 9, 2024 00:43

typo

de4d1d1

typo

2d66c72

Johnlon mentioned this pull request May 9, 2024

Support for attachments / embeddings as per Cucumber and Behave #617

Open

Johnlon and others added 9 commits May 16, 2024 17:43

added support for attachements in the cucumner json and events output…

18b557b

… formats - done by sneaking the attachment into the context.Context object (but not letting that context value propagate into the next step, so it's kinda bending the context.Context purpose)

typo

eeabc29

README.md

31cf872

typo

bdd1082

typo

7cc1ac0

typo

b9984ba

typo

298d347

typo

2b9185b

typo

a8f193f

mattwynne requested review from iaroslav-ciupin and vearutop May 27, 2024 20:32

Johnlon added 2 commits May 28, 2024 23:45

Added CHANGELOG entry

b4652cf

go fmt issue

9de0ee7

Johnlon merged commit 201e526 into cucumber:main May 28, 2024

vearutop reviewed May 29, 2024

View reviewed changes

internal/formatters/fmt_cucumber.go Show resolved Hide resolved

suite.go Show resolved Hide resolved

internal/models/results.go Show resolved Hide resolved

internal/models/results.go Show resolved Hide resolved

suite.go Show resolved Hide resolved

iaroslav-ciupin removed their request for review May 29, 2024 17:04

Johnlon mentioned this pull request May 30, 2024

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

Merged

7 tasks

Conversation

Johnlon commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤔 What's changed?

⚡️ What's your motivation?

🏷️ What kind of change is this?

♻️ Anything particular you want feedback on?

📋 Checklist:

Uh oh!

codecov bot commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mattwynne commented May 27, 2024

Uh oh!

mattwynne commented May 27, 2024

Uh oh!

Johnlon commented May 27, 2024

Uh oh!

Johnlon commented May 27, 2024

Uh oh!

Johnlon commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattwynne commented May 29, 2024

Uh oh!

mattwynne commented May 29, 2024

Uh oh!

vearutop left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Johnlon commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattwynne commented May 29, 2024

Uh oh!

Johnlon commented May 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Johnlon commented May 8, 2024 •

edited

Loading

codecov bot commented May 8, 2024 •

edited

Loading

Johnlon commented May 28, 2024 •

edited

Loading

Johnlon commented May 29, 2024 •

edited

Loading