JSON formatter for cucumber-js #79

Closed
wants to merge 48 commits into
from

3 participants

@worldofchris

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

worldofchris added some commits May 21, 2012
@worldofchris worldofchris Skeletal JSON formatter a23d0b1
@worldofchris worldofchris Getting my head around Jasmine Spys... 3f23299
@worldofchris worldofchris Tests now ignore whitespace / indentation and just check JSON is valid b4f1b0c
@worldofchris worldofchris Updated README to show status of my hacks 953f824
@worldofchris worldofchris Removed redundant ProgressFormatter methods from the JsonFormatter an…
…d fixed up the Jasmine tests for the latter
8eb1074
@worldofchris worldofchris Updated the output of the tests in the json_formatter feature to matc…
…h what you get from Ruby Cucumber
fd940fa
@worldofchris worldofchris Command line says there are no commits GUI says there are… 58cd346
@worldofchris worldofchris Now have two passing scenarios.
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.
e613101
@worldofchris worldofchris Now have three scenarios passing albeit with steps:match:location not…
… yet implemented
a4a6b75
@worldofchris worldofchris Four passing scenrios. Need to add error_message to EVENT though deb9e6b
@worldofchris worldofchris Added Scenarios for failing and passing steps f91bea1
@worldofchris worldofchris Added more scenarios to the tests. Removed redundant methods copied f…
…rom progress listener
51c52af
@worldofchris worldofchris Removing redundant dir b3dc4ad
@worldofchris worldofchris Added more tests. Pushed the placeholder URI down into the event obje…
…ct. Don't know how it should be populated.
e042dae
@worldofchris worldofchris Added tests for multiple features. Fixed functionality to populate co…
…rrect feature/scenario with its child steps. Added more Jasmine tests
7035ea0
@worldofchris worldofchris Added note to readme to clarify current status of JSON formatter 3a03a91
@worldofchris worldofchris Merge remote-tracking branch 'upstream/master'
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
7dfe6e3
@worldofchris worldofchris Removed redunant require of Cucumber-js 516c5b9
@worldofchris worldofchris Updated README to show that we are now using the new Gherkin provided…
… json_formatter
109d47d
@worldofchris worldofchris Merge remote-tracking branch 'upstream/master' ebafde0
@worldofchris worldofchris Refactored to use the newly available Gherking json_formatter. Got on…
…e feature passing now. Ten to go...
a7d9747
@worldofchris worldofchris Adding renamed classes which now wrap the Gherkin JSON formatter e0cf8a6
@worldofchris worldofchris Removed my now redundant json_formatter code and replaced with calls …
…to the new Gherkin provided formatter. All features are now passing but there not all the jasmine tests.
95b7aba
@worldofchris worldofchris Jasmine tests all passing. Ugly relative path to require Gherkin remo…
…ved thanks to @jbpros.
31f260b
@worldofchris worldofchris Refactored json_formatter_wrapper to use functionality provided by pa…
…rent listener class. Added partial support for background. Does not deal with results of background steps yet.
0e5d28b
@worldofchris worldofchris Merge remote-tracking branch 'upstream/master'
Conflicts:
	lib/cucumber/cli.js
	lib/cucumber/cli/configuration.js
6ef524d
@worldofchris worldofchris Added Work In Progress DocString formatting. Fixed fail in previous m…
…erge from upstream.
f5c2112
@worldofchris worldofchris Added DocString support 8272e2f
@worldofchris worldofchris Added support for scenario level tags dd5eb0a
@worldofchris worldofchris Added support for Feature level tags 0b8d93e
@worldofchris worldofchris Added support for Data Tables but with line for each row stubbed out …
…as 'TODO'
1cdfafb
@worldofchris worldofchris Added scenario to test for a background with a table 0d62fb8
@worldofchris worldofchris Added scenario to test for background with DocString 0e92cce
@worldofchris worldofchris Made progress on the jasmine-node tests. In doing this refactored jso…
…n_formatter_wrapper to take the json_formatter as an input parameter rather than creating it itself. Makes it easier to mock and so easier to test.
b23ee5a
@worldofchris worldofchris Fixed up Jasmine tests to make them test _just_ the json_formatter_wr…
…apper by properly faking the Gherkin json_formatter and testing for calls to it.
a548a3b
@worldofchris worldofchris Added scenario to test for skipped steps cf4e301
@worldofchris worldofchris Added more jasmine tests for step results 57e6f5b
@worldofchris worldofchris Corrected formatting in features file 8a953bc
@jbpros jbpros referenced this pull request Jul 24, 2012
Closed

Json formatter #76

@jbpros
Cucumber member

This pull request is in competition with #76.

@dmcaulay

it's cool if you go with this formatter. i glanced at it briefly and it looks more complete than mine. it also uses the gherkin json formatter (gherkin/lib/gherkin/formatter/json_formatter) so it's most likely in a more standard format.

one potential issue is that it doesn't use the base formatter so it can't log to a function. not sure if you care about that.

@jbpros jbpros and 1 other commented on an outdated diff Jul 24, 2012
features/a.feature
@@ -0,0 +1,3 @@
@jbpros
Cucumber member
jbpros added a line comment Jul 24, 2012

What's this file for?

@worldofchris
worldofchris added a line comment Jul 24, 2012

Apologies, I think that is cruft. Have deleted from https://github.com/worldofchris/cucumber-js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jbpros
Cucumber member

The feature suite is broken when run by Cucumber.js. Could you tell me a little about this CUCUMBER_JS_HOME environment variable?

@jbpros jbpros commented on the diff Jul 24, 2012
features/step_definitions/cli_steps.js
if (actualOutput.indexOf(expectedOutput) == -1)
- throw new Error("Expected output to match the following:\n'" + expectedOutput + "'\nGot:\n'" + actualOutput + "'.");
+ throw new Error("Expected output to match the following:\n'" + expectedOutput + "'\nGot:\n'" + actualOutput + "'.\n" +
+ "Error:\n'" + actualError + "'.\n" +
+ "stderr:\n'" + actualStderr +"'.");
+ callback();
+ });
+
+ this.Given(/^CUCUMBER_JS_HOME environment variable has been set to the cucumber\-js install dir$/, function(callback) {
@jbpros
Cucumber member
jbpros added a line comment Jul 24, 2012

This Given step is an assertion.

Givens are not assertions but context setters. Instead of checking that the environment variable is indeed set, this step should actually define the variable.

That being said, I still don't totally understand the reason for that variable :)

@worldofchris
worldofchris added a line comment Jul 24, 2012

Is it not valid for a Given to assert that a required precondition is present?

The point of this environment variable is to provide the base path for matching the uris to features which in cucumber.js include the full path to the file.

If there is a way for cucumber.js to know where it is installed and so be able to provide this information to make the match without use of an environment variable then all the better and I am open to suggestions.

@jbpros
Cucumber member
jbpros added a line comment Jul 25, 2012

Thanks for the clarification, Chris.

No, the purpose of Givens is not to assert truths, it's to set the context in which the system is being automated.

When I read Given CUCUMBER_JS_HOME environment variable has been set to the cucumber-js install dir, I understand "consider the CUCUMBER_JS_HOME variable to be set", not "let's be sure the variable has been set before running the current feature".

There should be no implicit requirements before running feature; scenarios should be both independent and self-contained. The rule of scenario independence is not respected with this environment variable: what if you suddenly need a scenario testing the absence of that variable? You could not run the two scenarios in the same Cucumber run. That wouldn't be that great, right?

Getting Cucumber's install path should not be too complicated; I'll come back with a suggestion soon.

@worldofchris
worldofchris added a line comment Jul 25, 2012

"There should be no implicit requirements before running feature"

That's why I made it explicit by putting it in the Given step in the background.

I agree with your point re. scenario independence. I think the confusion for me is that I am coming to this from a background of using Gherkin to facilitate specification workshops, where the first thing you do is capture the preconditions that need to be in place before an action can be performed.

I can now see that this is not the case at the code level. At the code level you are 'put[ting] the system in a known state' - https://github.com/cucumber/cucumber/wiki/Given-When-Then

Very happy to lose the ENV VAR if you can provide an alternative mechanism for matching the uris.

@jbpros
Cucumber member
jbpros added a line comment Jul 25, 2012

Well, yes you are right, Given is about preconditions. But instead of simply stating (and verifying) those preconditions, the step fulfills them.

By implicit requirements I was referring to the fact that one must go through the scenarios to find out this environment variable should be set before running the feature suite. Imagine several similar preconditions on the feature suite: it would quickly become a nightmare to handle.

@worldofchris
worldofchris added a line comment Jul 25, 2012

I was agreeing with you that, at the code level, preconditions are about setting them up rather than just checking whether they have already been fulfilled. :-)

@jbpros
Cucumber member
jbpros added a line comment Jul 25, 2012

Yep, I was just rephrasing stuff. One of my defaults is that I'm a perfectionist. ;)

@worldofchris
worldofchris added a line comment Jul 25, 2012

It occurs to me, one way of resolving this would be that,
Given the part of the uri which changes from installation to installation is the path to the sandbox where the features are executed
When we compare the actual JSON with the expected JSON
Then we could only compare the part of the uri within the sandbox e.g. cucumber-js-sandbox/features/a.feature
And ignore everything before that.

WDYT?

@worldofchris
worldofchris added a line comment Jul 25, 2012

I have made this change:

worldofchris@e37a4cc

There is no longer a need for the CUCUMBER_JS_HOME env var.

Instead I have sanitised the two attribute values where it appeared, "uri" and "error_message" to replace the location specific strings with something generic.

In the case of "error_message" this also addresses the problem that whenever the JSON formatter code changed there was a chance of a side effect of the line numbers in the error message changing, so taking the actual error message text out removes this problem.

Given the content of the error message is not the primary concern of the formatter, i.e. it just prints what it is given, I don't think this is a big loss.

Let me know what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@worldofchris

Yes, the CUCUMBER_JS_HOME should be set to where cucumber-js is installed as per:

Given CUCUMBER_JS_HOME environment variable has been set to the cucumber-js install dir

See https://github.com/worldofchris/cucumber-js/blob/master/features/json_formatter.feature#L6

This is to allow us to match the URIs that cucumber-js produces which use the full path to the feature regardless of where cucumber-js has been installed.

For example:

Then it should output this json:
  """
  [
    {
      "id":"some-feature",
      "name":"some feature",
      "description":"",
      "line":1,
      "keyword":"Feature",
      "uri":"$CUCUMBER_JS_HOME/tmp/cucumber-js-sandbox/features/a.feature"
    }
  ]
  """
@jbpros jbpros and 1 other commented on an outdated diff Jul 24, 2012
lib/cucumber/cli/configuration.js
@@ -15,6 +15,11 @@ var Configuration = function(argv) {
case Configuration.PRETTY_FORMAT_NAME:
formatter = Cucumber.Listener.PrettyFormatter();
break;
+ case Configuration.JSON_FORMAT_NAME:
+ var JSONFormatter = require('gherkin/lib/gherkin/formatter/json_formatter');
+ var jsonFormatter = new JSONFormatter(process.stdout);
+ formatter = Cucumber.Listener.JsonFormatterWrapper(jsonFormatter);
@jbpros
Cucumber member
jbpros added a line comment Jul 24, 2012

I'd prefer the formatter to follow the naming conventions of the other formatters. This should be JsonFormatter.

The Gherkin formatter constructor could be known as GherkinJsonFormatter in Cucumber.js (and its instances would be gherkinJsonFormatter). I think it would clarify things.

var GherkinJsonFormatter = require('gherkin/lib/gherkin/formatter/json_formatter');
var gherkinJsonFormatter = new GherkinJsonFormatter(process.stdout);
formatter = Cucumber.Listener.JsonFormatter(gherkinJsonFormatter);
@jbpros
Cucumber member
jbpros added a line comment Jul 24, 2012

The creation of the gherkin formatter should be handled by the JsonFormatter constructor. This logic is part of its internal "wrapping deal" that should not surface. The instantiation of the JsonFormatter would become as painless as it is for the other formatters.

When running Cucumber in a browser environment, the formatter has to be instantiated manually, there is no configuration management for that matter. The simplest their constructors, the better!

@worldofchris
worldofchris added a line comment Jul 24, 2012

Happy to change that. I was anxious to distinguish it from the Gherkin formatter which is doing the actual formatting work. Will push those changes up tomorrow morning.

@worldofchris
worldofchris added a line comment Jul 24, 2012

I did have the creation of the gherkin formatter being done by the JsonFormatter constructor but then moved it out when I was writing the jasmine-node tests so as to be able to create it with a dummy gherkin formatter for test purposes. Happy to revert to having the JsonFormatter do the instantiation.

@worldofchris
worldofchris added a line comment Jul 25, 2012
@worldofchris
worldofchris added a line comment Jul 25, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jbpros jbpros and 1 other commented on an outdated diff Jul 24, 2012
spec/cucumber/ast/feature_spec.js
@@ -56,6 +56,12 @@ describe("Cucumber.Ast.Feature", function() {
});
});
+ describe("getUri()", function() {
@jbpros
Cucumber member
jbpros added a line comment Jul 24, 2012

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

@worldofchris
worldofchris added a line comment Jul 24, 2012

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.

@worldofchris
worldofchris added a line comment Jul 25, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jbpros jbpros and 1 other commented on an outdated diff Jul 24, 2012
spec/cucumber/cli/argument_parser_spec.js
@@ -109,6 +115,10 @@ describe("Cucumber.Cli.ArgumentParser", function () {
var shortenedOptionDefinitions = argumentParser.getShortenedOptionDefinitions();
expect(shortenedOptionDefinitions[aliasName]).toEqual(aliasValue);
});
+
+ it("defines an alias to --output as -o", function() {
@jbpros
Cucumber member
jbpros added a line comment Jul 24, 2012

This is empty.

@worldofchris
worldofchris added a line comment Jul 25, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jbpros
Cucumber member

@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.

@jbpros jbpros and 1 other commented on an outdated diff Jul 25, 2012
features/simple.feature
@@ -0,0 +1,5 @@
@jbpros
Cucumber member
jbpros added a line comment Jul 25, 2012

Do we need this feature?

@worldofchris
worldofchris added a line comment Jul 25, 2012

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?

@jbpros
Cucumber member
jbpros added a line comment Jul 25, 2012

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

@worldofchris
worldofchris added a line comment Jul 25, 2012

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

@jbpros
Cucumber member
jbpros added a line comment Jul 25, 2012

That's clever.

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

@worldofchris
worldofchris added a line comment Jul 25, 2012

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
worldofchris added some commits Jul 25, 2012
@worldofchris worldofchris Moved Gherkin JSON formatter back into Wrapper class. Added getter to…
… allow us to spy on this in jasmine tests
243bce8
@worldofchris worldofchris Removed redundant spec for uri method 80b8b56
@worldofchris worldofchris Removed spurious spec which has nothing to do with JSON formatter b827efb
@worldofchris worldofchris Renamed JSON Formatter to make it consistent with existing formatters 5753058
@worldofchris worldofchris Removed spurious feature file 73b52b6
@worldofchris worldofchris Removed need for CUCUMBER_JS_HOME environment variable by substitutin…
…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.
e37a4cc
@jbpros jbpros and 1 other commented on an outdated diff Aug 4, 2012
lib/cucumber/ast/feature.js
@@ -26,6 +26,15 @@ var Feature = function(keyword, name, description, uri, line) {
return line;
},
+ getUri: function getUri() {
+ //TODO: Add uri to feature
@jbpros
Cucumber member
jbpros added a line comment Aug 4, 2012

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?

@worldofchris
worldofchris added a line comment Aug 6, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jbpros jbpros and 1 other commented on an outdated diff Aug 4, 2012
lib/cucumber/cli/configuration.js
@@ -15,6 +15,9 @@ var Configuration = function(argv) {
case Configuration.PRETTY_FORMAT_NAME:
formatter = Cucumber.Listener.PrettyFormatter();
break;
+ case Configuration.JSON_FORMAT_NAME:
+ formatter = Cucumber.Listener.JsonFormatter(process.stdout);
@jbpros
Cucumber member
jbpros added a line comment Aug 4, 2012

Call me nit-picker for this but would it be possible to stay consistent with the other formatters and not pass anything (yet) to the constructor?

I/O handling is something we must improve in the near future, on a more general level.

@worldofchris
worldofchris added a line comment Aug 6, 2012

Happy to do that. I was just following the constructor of the gherkin formatter that this wraps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jbpros
Cucumber member

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

@worldofchris

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.

@worldofchris worldofchris Made JsonFormatter consistent with others by making constructor take …
…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.
2c9d5e0
@worldofchris

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

@jbpros
Cucumber member

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
Cucumber member

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
Cucumber member

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

@worldofchris

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
Cucumber member

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 added a commit that closed this pull request Sep 6, 2012
@worldofchris worldofchris Add JSON formatter (close #79) 7fd3bdd
@jbpros jbpros closed this in 7fd3bdd Sep 6, 2012
@jbpros
Cucumber member

About time.

@jbpros
Cucumber member

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

@worldofchris

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
Cucumber member

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. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment