-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Support attachments, also known as embeddings #189
Conversation
Travis CI seems to be failing with npm install issues. It's something to do with bower. Any ideas? It doesn't seem to be related to this pull request (there are no changes to the |
Wow, thanks for this big piece of work. I'd like to fix this Travis issue that has been bugging us for some time now before going any further with the PRs. |
@jbpros have the Travis issues been happening for long? Is there anything I can do to help? |
I've answered my own question :-) It looks like the dev dependency on Bower is causing the Travis CI build failures. I've submitted a pull request that removes the dependency: #191 |
Please merge this to master :) |
Hi @simondean, I have used the the following commit - my structure code is as follows -
the feature file is as following -
When I try to change the Before() or After() to add the scenario as 1st argument, I see that it is still passed with the next details rather than the scenario. What am I missing here or infact - doing it wrong? thank you for your time and help. oh! and for reference, this is what the
|
Hi @simondean can you drop your last 3 commits and rebase off of master. Hopefully then we can get this sucker merged in. I will do a deep review as soon as you rebase, just ping me 👍 |
Thanks @samccone. I'll hopefully be able to drop the commits and do the rebase before the end of the week. |
Stream.Readable is not available in node v0.6 and v0.8 so it is not possible to test Stream.Readable functionality under those versions of node
Hi @samccone. I've removed the two unnecessary commits. I've kept the other commit as it fixes an issue with node v0.6 and v0.8. I've also rebased with master. |
I've reviewed the whole thing, it's impressive. Thanks for this great work @simondean. I'm playing a bit more with it and then we'll merge if everything looks ok. |
return self; | ||
}; | ||
|
||
HookStep.NAME = 'scenario'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be undefined
as well, this arbitrary value seems artificial.
Hooks should not be output by the pretty formatter and I think it would be nice to flag steps that are coming from hooks. Adding a |
@jbpros I've just pushed a change which removes the names from hook steps. isHidden sounds good. I'll have a look at that. |
…er and stats journal
@jbpros I've just pushed a commit that adds the isHidden functionality. In the end I had to change summary formatter and stats journal as well as pretty formatter. As part of the commit I've created feature tests for the pretty formatter and summary formatter. |
…cucumber tests strip-ansi did not work under node v0.6 and v0.8
Hi @jbpros. The building is passing now. I had to remove the strip-ansi dependency I'd added. I found that the cucumber-js tests already had some regex to strip ANSI color codes so I used that in step. As part of the commit, I fixed some undefined steps for the Ruby cucumber tests. The undefined steps where in features/cli.feature |
@jbpros do you think it's worth adding a "hidden": true attribute to the output of the JSON formatter for hidden steps? |
@simondean thanks mate, this is amazing :) Yes, I think a |
… attribute to JSON formatter
Hi @jbpros. I realised that the Pretty Formatter would print "undefined" as the step name for the hook steps. I've just pushed a commit that fixes that. The commit also changes the JSON formatter to add the Hopefully that's everything 😄 |
@simondean this has been merged, thanks again! It'd be nice to document this new feature in the README. Can you do that? |
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. |
Hi. This pull request implements:
It also fixes:
The pull request includes several new cucumber tests under
features/
in the codebase which provide examples of i) how attachments can be used from hooks and from step definitions, ii) how hooks are now reported as steps by the JSON formatter and iii) how you can fail a step using callback("fail") and callback.fail().Here's a quick example of how the hooks appear as steps in the JSON (taken from
features/hooks.feature
):Here's an example of how to attach an attachment from a before hook (these attachment examples are taken from
features/attachments.feature
):Attaching from an around hook:
I've implemented #179 "Hooks as steps" as part of this pull request to avoid over complicating the way reporters/listeners/formatters work when attaching from a hook. See the getCurrentStep method in https://github.com/cucumber/gherkin/blob/master/java/src/main/java/gherkin/formatter/JSONFormatter.java to contrast with the other way this could have been implemented (Gerkin's Java JSONFormatter has to rewind time to earlier steps in order to add attachments to the last step that executed/failed).