Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to send embeddings through the wire protocol #223

Closed
wants to merge 3 commits into from

Conversation

Sigill
Copy link

@Sigill Sigill commented Jun 15, 2019

Summary

Motivation and Context

I'm looking into cucumber-cpp for my company.
We need to be able to embed text or images into the reports, but that feature is not available when using the wire protocol.

This patch is the 4th of a series of 4, respectively impacting cucumber-ruby, cucumber-ruby-core, cucumber-ruby-wire and cucumber-cpp.

Basically, the idea is to allow embeddings to be specified in the success or fail responses sent through the wire protocol. E.g.:

["success",{"embeddings":[{"label":"Embedded text","mime_type":"text/plain","src":"Some text"}]}]
["fail",{"embeddings":[{"label":"Embedded image","mime_type":"image/png;base64","src":"iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg=="}],"message":"There was no  banana"}]

The patch on cucumber-ruby (cucumber/cucumber-ruby#1354) will let formatters access embeddings stored in a step result.
The patch on cucumber-ruby-core (cucumber/cucumber-ruby-core#170) will store in the step result the embeddings returned by the step invocation.
The patch on cucumber-ruby-wire (cucumber/cucumber-ruby-wire#20) will allow the embeddings stored in the wire response to be returned by the wire-based step invocation process.
The patch on cucumber-cpp (#223) will allow C++ steps to send embeddings through wire responses.

Note: embeddings were not considered to be useful for pending steps, but support should be possible if required.

Details

This patch:

  • Redesign the invokeStep() method to return an InvokeResult instead of relying on implicit success or exceptions.
  • Add an embed() function to allow steps to declare embeddings. The embeddings of a step are temporarily stored within that step.
  • Once the result of a step is known, store the embeddings in the InvokeResult, then in the WireResponse object.
  • Serialize the embeddings from the WireResponse in the packet to be sent.

Patches on cucumber-ruby, cucumber-ruby-core, cucumber-ruby-wire have been submitted for integration in cucumber 4.x, but the changes on cucumber-cpp should be backward compatible with cucumber 2.x. Embeddings will just be ignored.

How Has This Been Tested?

Unit test on the redesign of the invokeStep() method, serialization of embeddings and creation of WireResponse from InvokeResult.
Still need to look at E2E tests (suggestions welcomed).

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • It is my own work, its copyright is implicitly assigned to the project and no substantial part of it has been copied from other sources (including Stack Overflow). In rare occasions this is acceptable, like in CMake modules where the original copyright information should be kept.
  • I'm using the same code standards as the existing code (indentation, spacing, variable naming, ...).
  • I've added tests for my code.
  • I have verified whether my change requires changes to the documentation
  • My change either requires no documentation change or I've updated the documentation accordingly.
  • My branch has been rebased to master, keeping only relevant commits.

@coveralls
Copy link

coveralls commented Jun 16, 2019

Coverage Status

Coverage decreased (-0.8%) to 61.69% when pulling 7e01c43 on Sigill:embeddings_support into dd424c1 on cucumber:master.

@Sigill Sigill force-pushed the embeddings_support branch 2 times, most recently from 2a6604c to 5ace79b Compare June 16, 2019 17:32
@jermus67
Copy link
Contributor

jermus67 commented Sep 18, 2021

This PR is closed without merging.

The PR was the last of sequence of four PRs, that had to be merged in a specific sequence as each PR in the series depended on the previous one having been merged before.

  • The first PR was to update the json parser in cucumber-ruby, this PR has been merged.
  • The second PR was to update cucumber-ruby-core to allow embeddings in step results, this one never got merger (closed by stale-bot).
  • The third PR was to update cucumber-ruby-wire to support these embeddings over the wire protocol (also close by stale-bot).

Since the second and third PR have been closed without merging, there is no point in merging this one. If this functionality is required first the two other PRs need to be revived before it makes sense to have another look at this one.

@jermus67 jermus67 closed this Sep 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants