[Core] Add name to embedding #1693
Conversation
| @@ -737,8 +753,13 @@ private void runFeaturesWithFormatter(URL outputDir) throws Throwable { | |||
| stepsToLocation.put("first step", "path/step_definitions.java:3"); | |||
| hooks.add(TestHelper.hookEntry("after", result("passed"))); | |||
| hooks.add(TestHelper.hookEntry("after", result("passed"))); | |||
| hookActions.add(createEmbedHookAction("fakedata".getBytes("US-ASCII"), "image/png")); | |||
| hookActions.add(createEmbedHookAction("dodgy stack trace here".getBytes("US-ASCII"), "text/plain")); | |||
| if (isNamedEmbedding) { | |||
There was a problem hiding this comment.
i think it would be better to remove isNamedEmbedding flag
as we can have both embeddings with and without name in one report.
There was a problem hiding this comment.
I deleted this flag and updated existing tests, so they cover case with and without name.
| if (event.name != null) { | ||
| jsFunctionCall("embedding", mimeType, fileName, event.name); | ||
| } else { | ||
| jsFunctionCall("embedding", mimeType, fileName); |
There was a problem hiding this comment.
Would it be possible to fix jsFunctionCall so this if statement isn't needed?
There was a problem hiding this comment.
Or I guess fix the embedding function that jsFunctionCall calls.
There was a problem hiding this comment.
I removed this 'if' statement.
| @@ -93,4 +96,10 @@ public Throwable getError() { | |||
|
|
|||
| return max(stepResults, Result.SEVERITY).getError(); | |||
| } | |||
|
|
|||
| private void busSend(EmbedEvent embedEvent) { | |||
| if (bus != null) { | |||
There was a problem hiding this comment.
I couldn't find any calls into the Scenario constructor where bus would be null.
There was a problem hiding this comment.
I removed the check for null, also did it in write method for consistency.
…TMLFormatter tests
|
LGTM. Cheers. 😄 |
|
Same, looks good. Merged! Thanks. |
|
Hi @Dzieciak, Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾 In return for this generous offer we hope you will:
On behalf of the Cucumber core team, |
mpkorstanje
changed the title
|
@beardedqa thanks for merging! In the future could you please use the squash merge option and add a commit message of the form Finally also please update the change-log itself occasionally. |
|
@mpkorstanje Sure, thank you for pointing that. |
Summary
This is a pull request related to #1692.