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

Embed mime type JSON change breaking downstream consumers #978

Closed
achingbrain opened this issue Nov 26, 2017 · 10 comments · Fixed by #995
Closed

Embed mime type JSON change breaking downstream consumers #978

achingbrain opened this issue Nov 26, 2017 · 10 comments · Fixed by #995

Comments

@achingbrain
Copy link

As part of this commit (I think, there's quite a lot in there) the representation of attachments in the JSON reports changed from:

            "embeddings": [
              {
                "data": "iVBORw0KGgoA...",
                "mime_type": "image/png"
              }
            ],

to:

            "embeddings": [
              {
                "data": "iVBORw0KGgoA...",
                "media": {
                  "type": "image/png"
                }
              }
            ]

This has broken the Jenkins cucumber-report-plugin when used with (for example) attached screenshots. I've submitted a PR to the underlying library that fixes the problem but the maintainer seems unwilling to merge it, instead preferring that this library revert to the existing JSON report format that is used by other cucumber implementations.

Is there any chance of that happening?

I can submit a PR here too if it helps and is likely to be merged.

@sophiekruijt
Copy link

I would like to see this fixed too. I would prefer to not have to downgrade cucumber-js to a previous version.

@charlierudolph
Copy link
Member

Looking at the PR in cucumber-report-plugin I don't see any language that the maintainer doesn't want to merge it. Tools built off of cucumber need to update as cucumber changes (otherwise cucumber would never be able to change).

The change to the JSON formatter was to move to a more generic attachment object and one that is shared between other cucumber implementations.

@achingbrain
Copy link
Author

Looking at the PR in cucumber-report-plugin I don't see any language that the maintainer doesn't want to merge it.

His comments on that PR came after I opened this ticket. I was referring to this sort of thing on jenkinsci/cucumber-reports-plugin#193:

what I can recommend is to push cucumber-js team to fix the problem and not ask to fix all other dependent libraries team to apply workaround

To me, that reads like he'd rather the problem was fixed here and not in a PR to cucumber-report-plugin.

@achingbrain
Copy link
Author

I may be looking in the wrong place but I can't see any other cucumber implementations that use media.type - looks like the Java and Ruby ones at least still use mime_type. Is it a new standard for cucumber in the future?

@charlierudolph
Copy link
Member

The new structure was introduced in the event protocol formatter and I thought it made the most sense to carry that over to the JSON formatter. cucumber/common#172. cucumber-js uses a new event that cucumber-ruby does not so I think it is the first cucumber implementation to do this.

@aslakhellesoy @mattwynne @jbpros what do you all think about keeping this change in the json formatter or do you think it is worth reverting?

@innocentiv
Copy link
Contributor

Is my understanding that the JSON formatter is being phased out. I think it would be better to uniform the formatter behaviour between existing implementations and to use the new structure in the event-protocol formatter only.

We should not ask tools maintainers to update upon changes on a deprecated format with no official specification.

@sophiekruijt
Copy link

Is it already known whether this change with the JSON format will be reverted at some point? I'm trying to decide whether it's worth it to revert back to cucumber-js 2 for now, or whether there will be a fix at some point that will make cucumber-js 3 work again with the Jenkins-plugin.

@charlierudolph
Copy link
Member

I'll revert the change putting it back to the previous version

@khanali21
Copy link

@charlierudolph Any update on this? Are you going to revert it soon?
(Happy New Year !)

@lock
Copy link

lock bot commented Jan 4, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants