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

(refactor) Update testCase to include pickle object #947

Merged
merged 3 commits into from
Oct 14, 2017

Conversation

gd46
Copy link
Contributor

@gd46 gd46 commented Oct 7, 2017

@charlierudolph
Copy link
Member

Can you please add a test or update the existing tests here to showcase the use of pickle?

@gd46
Copy link
Contributor Author

gd46 commented Oct 8, 2017

@charlierudolph I updated the feature tests for hooks to include some assertions for pickle. I also updated the hooks readme to include some more info on what testCase contains, let me know what you think.

Do we need to update anything in test_case_runner_spec.js ? The first time I tried to include pickle in those tests as well the test was still failing. Not sure if I missed something on my original attempt.

@manu7691
Copy link

manu7691 commented Oct 9, 2017

Hey @gd46 , any way to add also failure exception when status is failed ?
Something like :
"result": {
"duration": 155557,
"status": "failed"
"failureException": { name, stack}
}

Thanks in advance,

@charlierudolph
Copy link
Member

charlierudolph commented Oct 9, 2017

@manu7691 that is already available in the after hook under testCase.result which has the structure: {duration, exception, status} where exception is only there when status is failed

@manu7691
Copy link

@charlierudolph I have been testing it with example for node, and it's still missing.
I noticed that */runtime/step_runner.js is returning {status: 'failed', exception, duration}: testStepResult, however on */runtime/test_case_runner.js only {status: 'failed', duration} is returned.

I think it would be better to return exception as well, to know the cause of failing test on AfterHooks.

image

Should I create an issue for this? Thanks

@charlierudolph
Copy link
Member

@manu7691 yes please create a separate issue for that

@gd46
Copy link
Contributor Author

gd46 commented Oct 10, 2017

@charlierudolph Im going to tweak some more of the README soon. But is there anything else we need to update. I wasnt sure what to do with the test_runner_spec. I noticed that had some tests around this as well, and my first try at updating them didnt work. And I was going to look at updating this readme as well: api reference

@charlierudolph
Copy link
Member

charlierudolph commented Oct 13, 2017

I don't think you need to update the readme. You don't need to update test_runner_spec either. Updating the api reference is probably a good idea and you can mention that pickle comes from the gherkin library. Once the API reference is updated, this is good to merge

@gd46
Copy link
Contributor Author

gd46 commented Oct 13, 2017

Sounds good. @charlierudolph I removed the update I made to hooks readme. And I updated the api readme to include the pickle object in both the before and after hook. And I added a link in the After hooks docs to mention that pickle comes from gherkin library.

@@ -36,8 +36,9 @@ Defines a hook which is run after each scenario.
* `tags`: string tag expression used to apply this hook to only specific scenarios. See [cucumber-tag-expressions](https://docs.cucumber.io/tag-expressions/) for more information
* `timeout`: A hook-specific timeout, to override the default timeout.
* `fn`: A function, defined as follows:
* The first argument will be an object of the form `{sourceLocation: {line, uri}, result: {duration, status}}` matching the event data for `test-case-finished`
* The first argument will be an object of the form `{sourceLocation: {line, uri}, result: {duration, status}, pickle: { tags: [{ name, location: { line, column } }], name, language, locations: [{ line, column }], steps: [{ text, arguments: [], locations: [{ line, column }] }]}}` matching the event data for `test-case-finished`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the text: "matching the event data for test-case-finished" as that is no longer true. Same for the Before Hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed that text. What is the event data for, and how come it no longer needs to match? I just want to understand what it is used for.

@@ -60,7 +61,7 @@ Multiple `AfterAll` hooks are executed in the **reverse** order that they are de

#### `Before([options,] fn)`

Defines a hook which is run before each scenario. Same interface as `After` except the first argument passed to `fn` will be an object of the form `{sourceLocation: {line, uri}}` matching the event data for `test-case-started`.
Defines a hook which is run before each scenario. Same interface as `After` except the first argument passed to `fn` will be an object of the form `{sourceLocation: {line, uri}, pickle: { tags: [{ name, location: { line, column } }], name, language, locations: [{ line, column }], steps: [{ text, arguments: [], locations: [{ line, column }] }]}}` matching the event data for `test-case-started`.
Copy link
Member

@charlierudolph charlierudolph Oct 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on saying the "will be an object of the form {sourceLocation, pickle} (see After hook for more details)" instead of repeating the structure. Alternatively can reference the other way where they are defined only here and not in the After hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed I cleaned it up. I put the details in the after hook since that is what comes first in the doc.

@charlierudolph charlierudolph merged commit ca74e8d into cucumber:master Oct 14, 2017
@aslakhellesoy
Copy link
Contributor

Hi @gd46,

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:

  • ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
  • 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
  • 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
  • ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
  • 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

@gd46
Copy link
Contributor Author

gd46 commented Oct 15, 2017

@aslakhellesoy thank you so much for the warm welcome, and allowing me to be apart of the cucumber community. I have been using this library for a long time and have watched it grow, and love where it is now. I hope I can help make further contributions going further.

@lock
Copy link

lock bot commented Oct 24, 2018

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 Oct 24, 2018
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 this pull request may close these issues.

Result is missing scenario & tags
4 participants