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

Rewrite the Json Formatter to use the new formatter api #851

Merged
merged 3 commits into from
Jun 21, 2015

Conversation

brasmusson
Copy link
Contributor

The main behavioural goal of the PR is to included results for test cases from Scenario Outlines also when the --expand option is not used (currently the --expand option is need to usable data when using Scenario Outlines).

A secondary behavioural goal is to align the produced Json file with what Cucumber-JVM produces, for the benefit of people that develop third party tools to be used together with the Cucumber version. The first tools that comes to mind are the Masterthought tools and the Bamboo plugin. This means that:

  • results for all executed steps are included (that means the Background steps are included each time they are executed), and
  • the executed hooks are reported (except for Around hooks, which does not seem to be included in the test case steps).

To get the location of the step definitions and hooks was somewhat challenging since the location of the Cucumber::Core::Test::Action is where the action block is create (in Cucumber::Runtime::SupportCode) and not the location hook/step definition itself). The Hooks are changed to hold the location of the hook method. In case of step definitions, the Json Formatter queries the Runtime for the location instead (same as the LegacyAdapter).

An additional benefit of this rewrite is that the Json Formatter now is independent of the Gherkin2 library.

Related to #839.

@aslakhellesoy
Copy link
Contributor

With Gherkin3 coming soon I really want to completely redesign the formatter API to something simpler. I think the JSON format should only contain results and attachments, and no info from the source (gherkin, markdown, html).

Yep, markdown and/or html as an alternative to gherkin is coming!

We need to sync up on this. I'll post an invite to some future plans/design sessions we can run online (google hangouts).

So wrt this pr, let's hold off until we all have a clear vision of near-term direction and goals.

brasmusson referenced this pull request in cucumber/cucumber-ruby-core May 16, 2015
@brasmusson
Copy link
Contributor Author

This PR is actually a step towards Gherkin3, since it removes the dependency from Cucumber to the Gherkin2 formatters, so the argument can be turned around - let's merge this PR so we come closer to using the Gherkin3 parser in Cucumber (to among other things get away form the "can't use Cucumber on x64 Windows" issues)

I'm all for a simpler, better Json result format, but do think that because it is lending itself for further tool processing and other tools already are using it, the current version needs to be deprecated and kept (for some time) in parallel with the new and improved Json result format.

@mattwynne
Copy link
Member

Thanks Bjorn. I'm right behind your strategy here, this is great work.

Is there anything specific you want me to review before we merge this in?

@brasmusson
Copy link
Contributor Author

Specific question 1, is about 73344a5, go from hook test_step to the hook location, as it is deeper in the code base, it is more important to get right.

if result.undefined? || hook_source?(step_source)
location = step_source.location
else
location = @runtime.step_match(step_source.name).file_colon_line
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specific question 2, this is the same as how the legacy adapter finds the location of step definition, but is it the way to go? Or should it be possible to go from test_step to action to step definition (similar as from test_step of a hook to the hook definition in 73344a5)?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think we need a consistent way for you to find the location of the action of the step. I don't think you should need the if statement on like 168 - everything should quack the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For step_definition test_steps I first tried to get the location from the action of the test_step, but for step definition the location is always the line in Cucumber::StepMatch where the action is created from the step definitions, so I instead did what LegacyApi::Adapter did - asking the Cucumber::Runtime::SupportCode to get the StepMatch (with the correct location). When I came to hook, I had to "solve the problem the right way" - but then test_steps from step_definitions and hook are treated differently. With new support in Cucumber-Ruby-Core (cucumber/cucumber-ruby-core@c595f23) actions know the location of the step_definition of hook that they are based on, and test_steps can be queried for the location of their action, so no if statement is necessary any more (to pass the location of the step_definition when creating the action we easy when the support in Cucumber-Ruby-Core was in place 48c0f34).

@mattwynne
Copy link
Member

Sorry it's taken me a while to give this attention @brasmusson. I think it's already a massive improvement as-is, so we can either merge it now and carry on discussing these minor refactoring points, or we can resolve them and then merge. You decide.

@mattwynne mattwynne modified the milestone: 2.0 Jun 19, 2015
Previously the location of the hooks referred to the line in
Cucumber::Runtime::SupportCode where the action block based on the hook
was created, instead of the location of the hook method itself.
This way the location of the step definition action will be correct.
Previously the location of the action of step definitions referred to
the line in Cucumber::StepMatch where the action block based on the
step definition was created, instead of the location of the step
definition method itself.
Now the Json formatter will produce usable data for Scenario Outlines
also when not using the --expand option. All executed step will be
reported (that is background steps will be included each time they are
executed). Also hook executions will be reported. Remove the usage of
the Gherkin2 library.
@brasmusson brasmusson force-pushed the json-formatter-new-api branch from adb8d81 to 42b28bd Compare June 21, 2015 08:37
@brasmusson brasmusson merged commit 42b28bd into master Jun 21, 2015
brasmusson added a commit that referenced this pull request Jun 21, 2015
@brasmusson brasmusson deleted the json-formatter-new-api branch June 21, 2015 09:52
@mattwynne
Copy link
Member

Don't forget to update History.md :)

(I wish we had a more automated way to do that!)

@brasmusson
Copy link
Contributor Author

I updated History.md in the merge commit (a94e2ae), personally I prefere "latest change first" but this time I put it last of the new features to be consistent with the rerun-entry.

@lock
Copy link

lock bot commented Oct 25, 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 25, 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.

3 participants