Proposal to make `body` of Attachment's optional

[badeball]

🤔 What's the problem you're trying to solve?

This is a product of a discussion in Discord, which I will attempt to summarize here.

I maintain a Cypress library whose goal is to implement the Cucumber experience in the Cypress domain. Cypress has an option to record videos in case of test failures, which is useful to put into reports - especially the HTML report, where they can be viewed inline.

A video can be large. A typical, naive message writer and a reader will both read attachments into memory, during writing and parsing, in leue of streaming based JSON writers/readers.

In Cypress, these videos appear on FS after a run, IE. they are already externalized.

We came to the conclussion that a pragmatic approach is to emit Attachment envelopes indicating an already externalized attachment, as compared to leaving the job of externalizing it up to the formatter.

The Attachment type on the other hand has a required body attribute. I propose to make it optional.

Personally, I enjoy union types in TS, something along the lines of Attachment = InlineAttachment | ExternalAttachment, which would leave little room to mix up body and url.

✨ What's your proposed solution?

Make the body attribute optional, possible even making the Attachment type a union type, as described above.

⛏ Have you considered any alternatives or workarounds?

I can forcefully work around TS and emit an Attachment without body, which the HTML formatter will happily accept and render correctly. However, it feels a bit hacky.

📚 Any additional context?

No response

[vitalets]

+1 for playwright-bdd. We are setting body: '' for external attachments.

[luke-hill]

We are/have discussed this in the contributor meeting. We'll update when we have something more formalised.

[mpkorstanje]

We currently have a the following fields in the Attachment message:

{ 
  body
  contentEncoding
  fileName?
  mediaType?
  source?
  testCaseStartedId?
  testStepId?
  url?
  testRunStartedId?
  testRunHookStartedId?
  timestamp?
}

Making the required body property optional would be quite annoying from a software evolution point of view. Consumers might expect it to be there and not be updated immediately. But we could add an ExternalAttachment (provisional name) message with the following fields.

{ 
  url
  mediaType?
  testRunStartedId?
  testStepId?
  testRunStartedId?
  testRunHookStartedId?
  timestamp?
}

Further more we could deprecate Attachment.url and Attachment.source. The former would be redundant given that ExternalAttachment exists. The latter seems to be unused. Consumers can then then transform Attachments into ExternalAttachment or ExternalAttachment into Attachments as needed (given no deprecated fields are used).

Thoughts?

[badeball]

Sounds good! And I agree that suddenly making it optional could be troublesome to existing consumers of inline attachments.

[davidjgoss]

A few notes:

• react-components already has support (as a consumer) for the current somewhat fuzzy way of externalising attachments via https://github.com/cucumber/react-components/pull/353 - this would need some light tweaking to support the new model
• cucumber-js implementation of the HTML formatter has support (as a producer) via https://github.com/cucumber/cucumber-js/pull/2413 - this would also need to be tweaked to do the new thing, and should ideally also be shifted left into html-formatter so all JS-based runners can benefit from it
• query has the method findAttachmentsBy which current returns a collection of Attachment - functionality we'd want to evolve this to include external ones too. This might have to vary a bit per platform norms e.g. a simple union in TypeScript but maybe some common interface or wrapper object in Java

More broadly though I'm in favour of this.