cucumber / compatibility-kit

Platform-agnostic set of acceptance tests for validating cucumber implementations
https://cucumber.io/
MIT License
1 stars 4 forks source link

cck: Improve attachment message #16

Closed mpkorstanje closed 4 years ago

mpkorstanje commented 4 years ago

Currently the Java implementation has two issues with the CCK.

  1. When provided with the mediaType text/plain the reference implementation will embed the contents as text. This seems rather unnecessary as there is a dedicated log method to do this. Changing the reference implementation to embed always write attachments as binary would simplify both the reference implementation and Cucumber-JVM.

  2. Cucumber-JVM supports file names in attachments. The reference implementation currently does not.

    @Override
    public void embed(byte[] data, String mediaType, String name) {
        bus.send(new EmbedEvent(bus.getInstant(), testCase, data, mediaType, name));

        // TODO: Remove this weird exception from the cck
        if (mediaType.equals("text/plain")) {
            bus.send(Messages.Envelope.newBuilder()
                .setAttachment(
                    Messages.Attachment.newBuilder()
                        .setTestCaseStartedId(testExecutionId.toString())
                        .setTestStepId(currentTestStepId.toString())
                        .setText(new String(data, UTF_8))
                        //TODO: Add file name to message protocol
                        .setMediaType(mediaType)
                        .build()
                )
                .build()
            );
        } else {
            bus.send(Messages.Envelope.newBuilder()
                .setAttachment(
                    Messages.Attachment.newBuilder()
                        .setTestCaseStartedId(testExecutionId.toString())
                        .setTestStepId(currentTestStepId.toString())
                        .setBinary(ByteString.copyFrom(data))
                        //TODO: Add file name to message protocol
                        .setMediaType(mediaType)
                        .build()
                )
                .build()
            );
        }
    }
aslakhellesoy commented 4 years ago

Is this stil relevant now that cucumber/common#947 has landed on master?

mpkorstanje commented 4 years ago

Looking at react/javascript/src/components/gherkin/Attachment.tsx there is still a coupling between the encoding and the media type. There shouldn't be one.

aslakhellesoy commented 4 years ago

What kind of coupling @mpkorstanje? Do you have an example of content encoding and media type that would cause a problem?

mpkorstanje commented 4 years ago

Currently:

I would expect every embedding to use BASE64 and depending on the media type be decoded by cucumber react. The event producer shouldn't have to parse the media type to know how to encode it. Currently it has to.

mpkorstanje commented 4 years ago

For an example of supported embeddings in the wild:

https://github.com/damianszczepanik/cucumber-reporting/blob/master/src/main/resources/templates/macros/json/embeddings.vm#L6-L31

https://github.com/trivago/cluecumber-report-plugin/blob/master/plugin-code/src/main/resources/template/macros/scenario.ftl#L140-L147