Store in Result the embeddings returned by a step

[Sigill]

Summary

In order to allow embeddings to be transferred from a wire response to a step Result, the invocation of a step needs to be capable of returning a data structure. This patch use that data structure (if any, previous design is still allowed) to build the Result object.

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 2nd 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 embeddings will then be stored in the step Result in order to be accessible by the formatter.

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 (#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 (cucumber/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

In the current implementation, it's not possible to access data from the wire response as nothing is returned from the wire invocation when a step succeed (a step is considered successful by default, an exception needs to be raised in case of failing or pending step).

Successful step

Cucumber::Core::Test::Runner::RunningTestCase::Status::Base.execute
  Cucumber::Core::Test::Step.execute
    Cucumber::Core::Test::Action.execute
      block -> Cucumber::StepMatch.invoke
        Cucumber::Wire::StepDefinition.invoke
          Cucumber::Wire::Protocol.invoke
            Cucumber::Wire::Protocol::Requests::Invoke.execute
              Cucumber::Wire::RequestHandler.execute
                Cucumber::Wire::Connection.call_remote
                  Cucumber::Wire::DataPacket reponse = // parsed packet
                  Cucumber::Wire::DataPacket.handle_with
                    Cucumber::Wire::DataPacket.handle_success // noop
                  // response discarded
      Cucumber::Core::Test::Action.passed -> return Cucumber::Core::Test::Result::Passed
  Cucumber::Core::Test::Result::Passed.describe_to(Cucumber::Core::Test::Runner::RunningTestCase)
    Cucumber::Core::Test::Runner::RunningTestCase.passed
      Cucumber::Core::Test::Runner::RunningTestCase@status = Cucumber::Core::Test::Runner::RunningTestCase::Status::Passing

Failing step

Cucumber::Core::Test::Runner::RunningTestCase::Status::Base.execute
  Cucumber::Core::Test::Step.execute
    Cucumber::Core::Test::Action.execute
      block -> Cucumber::StepMatch.invoke
        Cucumber::Wire::StepDefinition.invoke
          Cucumber::Wire::Protocol.invoke
            Cucumber::Wire::Protocol::Requests::Invoke.execute
              Cucumber::Wire::RequestHandler.execute
                Cucumber::Wire::Connection.call_remote
                  Cucumber::Wire::DataPacket reponse = // parsed packet
                  Cucumber::Wire::DataPacket.handle_with
                    Cucumber::Wire::RequestHandler.handle_fail -> throw Cucumber::Wire::Exception // error message (from response) embedded in exception
                  // response discarded
      Rescue in Cucumber::Core::Test::Action.execute
        Cucumber::Core::Test::Action.failed -> return Cucumber::Core::Test::Result::Failed
  Cucumber::Core::Test::Result::Failed.describe_to(Cucumber::Core::Test::Runner::RunningTestCase)
    Cucumber::Core::Test::Runner::RunningTestCase.failed
      Cucumber::Core::Test::Runner::RunningTestCase@status = Cucumber::Core::Test::Runner::RunningTestCase::Status::Failing

This patch does three things:

• It adds an embeddings field to the Passed and Failed responses and make their describe_to methods notify visitors from the availability of embeddings.
• It creates two new classes, PassedInvokeResult and FailedInvokeResult, to be returned by what is effectively invoking the step.
• Add code to allow an Action to check if the invocation of a step returned one of those objects, and create a Passed or Failed result accordingly.

Those changes should be backward compatible: successful step invocations that return nothing, failing step invocations that throw an exception and other behaviors are still supported.

How Has This Been Tested?

Add the embeddings field to the unit tests verifying the visitor mechanism of Result.
Add unit tests to verify that Action.execute returns a Passed/Failed object when the invocation of a step returns an PassedInvokeResult/FailedInvokeResult object (previous tests already verify that current behaviors still continue to work).

Types of changes

• Refactor (code change that does not change external functionality)
• 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:

• I've added tests for my code
• My change requires a change to the documentation.
• I have updated the documentation accordingly.

[luke-hill]

This seems really good, and keeps most / all of the return values which in a highly linked set of steps made it really easy to review.

I'll let someone more senior comment more on it, as it is the core - which I've not landed commits in.

[luke-hill]

good use of ordered specs here 👍 Although does it strictly matter? (Question, not request to change)

[Sigill]

Principle of least surprise, I think anybody should expect the order of embeddings to be maintained.
Probably overkill considering the underlying code, but nice to have.

[luke-hill]

should there be a sister test to this now that says embeddings are blank by default? If you're trying to keep it symmetrical.

EDIT: Same for the test below that requires 2 args

[Sigill]

I'm actually tempted to remove that test as it basically tests a setter+getter.
And I've not been able to find a way to keep things DRY and symmetrical without being overkill.

[luke-hill]

Re-review. Feedback needed here. What are your thoughts / plans?

[Sigill]

I've found how to DRY those tests using shared_examples.

[luke-hill]

I've re-reviewed / marked previous items as fixed up. Couple of outstanding questions but other than that this is good.

[luke-hill]

Re-review. Feedback needed here. What are your thoughts / plans?

[luke-hill]

I've reviewed all apart from result_spec and it seems fine. I'll get onto that in the next couple of days.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

[Sigill]

bump

[mattwynne]

@Sigill thanks for such a tidily-put together set of changes, and I'm sorry I've not had a chance to give them decent attention up until now.

My initial reaction is that I don't like changing the protocol for invoking a step to have the return value of the block become meaningful. I think it's fine passing the embed info in the wire message, but I'd like to see if we can work out a way to do it on the Ruby side so we can just call the #embed method like you would from a regular Ruby step defintion.

Did you have a go at that, were there any blockers?

If I can find some more time I can have a look at this myself, assuming this is still a priority for you?

[Sigill]

It's fine, it fell out of my (company's) priorities shortly after I submitted those patches, but I'm trying to bring everything back in 2020 (and I actually think that's a quite important feature missing from Cucumber).

First note: we have to modify the protocol, it's the only channel of communication between the step definitions (written in C++ in my case) and Ruby's Cucumber runner.

To be honest, I'm not a huge fan of the the way the Ruby RbWorld#embed method works.

It looks like formatters were originally based on an event notification mechanism (e.g. on_test_step_finished), but RbWorld#embed (which is provided by some globally available object...) is able to directly call the formatter's #embed method.

At a first glance, this might just look like a syntax issue: adding an embedding might be seen as an event, and the formatter's #embed method could probably be transformed to an event handler, but it does not makes sense when communicating through Wire as the Wire protocol is currently synchronous and unidirectional: when the server is processing an "invoke" request (executing a step), the client (the Cucumber runner) is waiting for exactly one response corresponding to the step invocation result (this is why I'm transferring the embeddings in the step invocation result).

We could transform the Wire protocol into an asynchronous communication channel, but that might be a bit complex, and most importantly, I'm not sure to see the benefit of being able to process the embeddings as soon as they occur (even with the current implementation provided by RbWorld#embed).

Actually, I think that applying my new logic (transferring the embeddings through the step invocation structure) when not using Wire could simplify/clarify a few things.

Forgive me if I'm a bit rusty about the subject, I haven't looked ad Cucumber's code for the past 6 months, but I'll be happy to discuss that with you @mattwynne .

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

[stale]

This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective.