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

JSON formatter for cucumber-js #79

Closed
wants to merge 48 commits into from
Closed

JSON formatter for cucumber-js #79

wants to merge 48 commits into from

Conversation

worldofchris
Copy link
Contributor

Hi Julien,

As discussed, here are the changes to add JSON output to cucumber.js

Let me know if you need anything else.

Cheers

Chris

Have had to deprecate the uri attribute as I can't figure out where to
get it from and this is holding up getting on with all the remaining
good stuff.
…rrect feature/scenario with its child steps. Added more Jasmine tests
Conflicts:
	README.md
	lib/cucumber/ast/feature.js
	lib/cucumber/listener.js
	lib/cucumber/parser.js
	spec/cucumber/ast/feature_spec.js
	spec/cucumber/cli_spec.js
…to the new Gherkin provided formatter. All features are now passing but there not all the jasmine tests.
…rent listener class. Added partial support for background. Does not deal with results of background steps yet.
Conflicts:
	lib/cucumber/cli.js
	lib/cucumber/cli/configuration.js
@@ -56,6 +56,12 @@ describe("Cucumber.Ast.Feature", function() {
});
});

describe("getUri()", function() {
Copy link
Member

Choose a reason for hiding this comment

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

This method is already speced :) The original example description is wrong though, it is about background while we're describing feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies. I started adding this before uris were available in cucumber.js. I then did a fetch of the upstream changes which introduced uris and clearly didn't remove this spec. Will do so and push up in the morning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed duplicate:

worldofchris@80b8b56

On 24 Jul 2012, at 23:05, Julien Biezemans wrote:

@@ -56,6 +56,12 @@ describe("Cucumber.Ast.Feature", function() {
});
});

  • describe("getUri()", function() {

This method is already speced :) The original example description is wrong though, it is about background while we're describing feature.


Reply to this email directly or view it on GitHub:
https://github.com/cucumber/cucumber-js/pull/79/files#r1230303

@jbpros
Copy link
Member

jbpros commented Jul 24, 2012

@dmcaulay Alright then, we'll close your pull request. Thank you for your work.

Ideally, I'd like all the formatters to inherit from the base formatter. However, the JSON formatter is a special beast because - as you said - it's just a wrapper around the formatter supplied by Gherkin itself. Thanks for pointing this, I'll try to find a way to use the base formatter.

@@ -0,0 +1,5 @@
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this feature?

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'm glad you asked :-) I can't see a great deal of value in it myself. Do you know how widely used it is in cucumber-ruby / jvm land?

Copy link
Member

Choose a reason for hiding this comment

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

Mmm, does it come from -ruby or -jvm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I've only used it with -ruby. I tested my features against cucumber-ruby before writing the -js impl.

Copy link
Member

Choose a reason for hiding this comment

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

That's clever.

This feature is a duplicate of cli.feature#95 anyway. Let's kill it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I think I was talking at cross purposes there. I thought you meant the inclusion of tables in the json output.

The simple.feature is plain old cruft. Will delete it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

…g those strings in the actual json output that contain the paths to either the cucumber.js source or the sandbox with sanitised versions of the strings. The two elements affected are uri and error_message, the content of which are not the primary concern of the formatter nor of these tests. Replacing them with sanitised versions makes the tests more portable and resiliant to chnge.
@@ -26,6 +26,15 @@ var Feature = function(keyword, name, description, uri, line) {
return line;
},

getUri: function getUri() {
//TODO: Add uri to feature
Copy link
Member

Choose a reason for hiding this comment

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

As you might have noticed, I don't like lingering TODOs in the codebase.

What can we do to remove this guy? :) Do we care about undefined values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

'undefined' is good. It does what is says on the tin.

@jbpros
Copy link
Member

jbpros commented Aug 4, 2012

I still got the feature suite broken. The specs are also failing.

@worldofchris
Copy link
Contributor Author

The update to package.json, see comment above, should fix the problem with the features and specs failing. It now refers to the latest version of gherkin which includes the missing json formatter.

…no arguments.

Removed 'TODO' strings from output of formatter.  Where a value is undefined it is passed to the Gherkin formatter as undefined.
This as the effect of supressing these values in the final JSON output.
Changed underscore_separated_variable to be camelCase.
@worldofchris
Copy link
Contributor Author

Any update on this pull request now the package.json has been fixed?

@jbpros
Copy link
Member

jbpros commented Aug 27, 2012

Sorry for the late reply, got some pretty tough days lately.

It's all green on my side now, thank you! I'll review it properly tonight or so. I spotted some things that can be improved in the features and specs, I'll suggest some changes and hopefully we'll merge your work soon.

Thanks again!

@jbpros
Copy link
Member

jbpros commented Sep 6, 2012

Hey Chris,

Sorry for all the delays.

I've cleaned up the code a bit and re-styled it to follow the project conventions.

In addition to making the JSON feature pass with Cucumber-ruby, I changed the way paths are handled within JSON payloads: placeholders are substituted at runtime with the real paths. I think it's more robust this way.

I'm doing a final review of this; I will probably push a few more small changes before merging this too-long-awaited thing :)

@jbpros
Copy link
Member

jbpros commented Sep 6, 2012

For your convenience, here is the diff between your branch and mine.

@worldofchris
Copy link
Contributor Author

Nice, I like the use of the as you say, more robust.

I've been using 'Should' like this for some time now. It goes back to this from the RSpec book which made a big impression on me when I first read it back in 2010:

"In executable code examples, we are setting an expectation of what should happen rather than what will happen. In fact we've embedded the word should right into RSpec's expectation framework..." p157 - The RSpec Book, Chelimsky et al. 2010

I use 'should' in Specification Workshops to make it clear we are talking about an expectation rather than an assertion.

That's just me giving you the back story for my choice of language, which believe me is something I agonise over.

Makes sense not to use 'should' here it it makes for consistency with the existing code / features.

@jbpros
Copy link
Member

jbpros commented Sep 6, 2012

I used to adhere to these theoretical ideas about should and I still agree with them.

However, Then already expresses the expectation aspect of the steps it prefixes. Also, lots of (not-so-useful) repetitions in scenarios lead to boring scenarios. With time and experience I came to appreciate shorter step sentences in which every single word carries some piece of information or clarification. Should is just a redundancy of Then.

The only solid argument in favor of should in gherkin scenarios is to differenciate a step from a synonymous one used as a Given or When:

Scenario: ambiguous steps
  The first and last steps of this scenario would resolve to the same stepdef while
  we'd probably use a factory in the Given and an assertion in the Then.

  Given I have 3 cucumbers
  When I eat a cucumber
  Then I have 2 cucumbers

Scenario: ambiguity solved thanks to "should"
  Given I have 3 cucumbers
  When I eat a cucumber
  Then I should have 2 cucumbers

However, after 2 years of not using should, I came up with this solution: using clarification words:

Scenario:
  Given I already have 3 cucumbers
  When I eat a cucumber
  Then I have 2 cucumbers

The word already clearly differentiates the context (Given) step from the outcome (When) step.

In the end, it really is a subjective appreciation and I'm definitely not against should.

As you wrote: let's be consistent with the rest of the feature suite.

@jbpros jbpros closed this in 7fd3bdd Sep 6, 2012
@jbpros
Copy link
Member

jbpros commented Sep 6, 2012

About time.

@jbpros
Copy link
Member

jbpros commented Sep 6, 2012

Oh and thank you Chris, this is a great contribution.

@worldofchris
Copy link
Contributor Author

Julien,

Thank you so much for your help and support with this contribution, it's been a real pleasure to work on cucumber.js.

Having been a technology manager for the last ten or so years I made a conscious decision this year to return to writing code.

About twenty years ago I cut my teeth writing 6502 assembler on a Commodore 64. Contributing to cucumber.js is the most fun I've had programming since then.

This is down to a number of things, first off the sheer quality of the code base. It is clear, well factored and easy to navigate. Second the quality of the tests, both at the Feature level and the Specs.

Finally, and most importantly, the quality of your reviewing and mentoring / directing of my code once the pull request had gone in. Peer review and mentoring through collaboration is at the heart of building and maintaining great software.

Cheers

Chris

@jbpros
Copy link
Member

jbpros commented Sep 6, 2012

I'm glad you enjoyed contributing to this project. This kind of feedback and cheerful words is what makes it exhilarating and making me think we're doing something useful here.

I know new features are sometimes slow to get into the codebase, compared to some other OSS projects. However, since day one I wanted Cucumber.js to be reliable, efficient, easy to use, close to its brothers (-rb, -jvm, ...) and easy for contributors to pick up. I think all of these criteria are fulfilled at the moment and I mean to keep it that way. I plan on publishing precise rules as to how pull requests should be articulated, what kinds of tests are mandatory, etc.

The build is rarely broken (well.. not the 3 last ones, there were a few issues with the JSON formatter specs on Travis ;)) and very few defects are being reported, even though the userbase is growing. I guess we're in the right direction.

So, thanks again for helping and complying to the rather strong constraints of the project. :)

@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