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

Use Gherkin3 instead of Gherkin2 #93

Merged
merged 7 commits into from
Sep 11, 2015
Merged

Conversation

brasmusson
Copy link
Contributor

This PR make Cucumber-Ruby-Core use (the parser of) Gherkin3 instead of Gherkin2. Some notes:

  • The one file apart from the parser that was used from Gherkin2 (tag_expression.rb), is copied into Cucumber::Core:Gherkin to remove all dependencies to Gherkin2.
  • To be able to use the Gherkin3 parser, a hook method is needed in the parser to be able to subclass the Gherkin3::AstBuilder (the integrate-ruby-core branch)
  • The Cucumber-Ruby-Core AST classes are not created directly from the parser, instead builder are still created by Cucumber::Core::Gherkin:AstBuilder which delays the creation of the AST classes until the whole feature file has been parsed. The main reason to do this is to be able to set the language for the Step and ExamplesTableRow AST classes on creation (so that the AST classes can be immutable). The Step AST class needs the language to be able to calculate the actual keyword for the snippets ("And", "But" and "*" are converted to the appropriate of "Given", "When" or "Then"). The ExamplesTableRow AST class needs the language to be able to supply the "Scenario" keyword in the --expand mode.
  • The Gherkin3 parser associates all comments (wherever they occur in the file) with the Feature AST node. To lessen the impact in Cucumber (particular the Pretty and HTML formatter), the comments are distributed down the AST tree at creation, so that the comment appear in the same Cucumber-Ruby-Core AST class as when using Gherkin2 (to be able to do this is another reason to use builders to delay the creation of the AST classes).

@brasmusson brasmusson force-pushed the integrate-gherkin3-parser branch from be287df to d2dc089 Compare July 6, 2015 14:38
@mattwynne
Copy link
Member

mattwynne commented Jul 6, 2015 via email

@brasmusson
Copy link
Contributor Author

I do not really see any reason that this can't go into production. The three things to do to get it there are:

  • Release a Gherkin3 gem (with the change in the branch integrate-ruby-core), now Gherkin3 use the name 'gherkin3' that could be v0.1.0 or something.
  • Release a new version of Cucumber-Ruby-Core
  • Release a new version of Cucumber-Ruby (with Use Gherkin3 instead of Gherkin2 cucumber-ruby#884)

The defensive approach would be to first release a Cucumber-Ruby v2.0.1 with what already is on the master of Cucumber-Ruby and Cucumber-Ruby-Core, and then (at the same time almost) release a Cucumber-Ruby v2.1.0 using Gherkin3. Then no unforeseen issue with the Gherkin3 integration would block anyone to use the changes already made after Cucumber-Ruby v2.0.0.

@tooky
Copy link
Member

tooky commented Jul 7, 2015

@mattwynne @brasmusson I guess we should probably benchmark it with a large set of features before we release it. We are moving from a C parser to ruby parser...

History: https://groups.google.com/forum/#!topic/cukes/usj_xgK0aKg

@mattwynne
Copy link
Member

Do you have release karma for cucumber-ruby and cucumber-ruby-core yet @brasmusson? Want ‘it?

@brasmusson
Copy link
Contributor Author

@mattwynne No, I do not have release karma for cucumber-ruby and cucumber-ruby-core. Sure, why not? You can give it to me.

@mattwynne
Copy link
Member

On 7 Jul 2015, at 16:34, Björn Rasmusson [email protected] wrote:
@mattwynne https://github.com/mattwynne No, I do not have release karma for cucumber-ruby and cucumber-ruby-core. Sure, why not? You can give it to me.

Great! Please see https://github.com/cucumber/cucumber-ruby/blob/master/CONTRIBUTING.md#gaining-release-karma https://github.com/cucumber/cucumber-ruby/blob/master/CONTRIBUTING.md#gaining-release-karma

@brasmusson
Copy link
Contributor Author

I used the parser_spec.rb to create time measurements for the parsing Gherkin files to Cucumber-Ruby-Core AST:s. Simply put one feature file with (for details see 238d43b):

  • 1 four step Backgound
  • n four step Scenarios
  • n three step Scenario Outlines each with
    • 2 Examples tables with two body rows each

(and also with some comments, descriptions, and tags).

With Ruby 2.1 on my (not so modern/fast) Linux box the results were:

n Gherkin3 (seconds) Gherkin2 (seconds)
100 1.1 0.4
200 3.2 0.9
400 10.7 1.9

With Ruby 2.1 and Gherkin3 on Travis the results were:

n time (seconds)
100 0.7
200 2.1
400 5.9

@aslakhellesoy
Copy link
Contributor

So it's 3-4 times slower than Gherkin2. Not too shabby considering this is pure ruby competing against C. Not worth optimising IMO - this should be fast enough.

@mattwynne
Copy link
Member

It doesn't make for a great headline, but I agree that parse time is rarely the bottleneck for someone's feature suite.

Presumably, if this does start to look like a problem for our users, we can use Berp to generate a C-based parser for Ruby to use?

@aslakhellesoy
Copy link
Contributor

The bottleneck is most likely in string scanning in the lexer, so it would have to be hand-written in C. Berp only generates the parser, not the lexer. Doable for sure if we need it.

@mattwynne mattwynne force-pushed the integrate-gherkin3-parser branch 2 times, most recently from 33b0bdf to 6014548 Compare September 11, 2015 13:35
tooky and others added 6 commits September 11, 2015 14:41
Revert back to using the ast_builder to be able to distribute the
language and comments from the feature down the tree before the ast
classes are created.
That is instead of subclassing the Gherkin3::AstBuilder, feed the
gherkin3 ast into the Cucumber::Core::Gherkin::AstBuilder to transform
it to a cucumber-core ast.
@mattwynne mattwynne force-pushed the integrate-gherkin3-parser branch from 6014548 to 9d135f3 Compare September 11, 2015 13:41
@mattwynne mattwynne merged commit 4eaa907 into master Sep 11, 2015
@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
@luke-hill luke-hill deleted the integrate-gherkin3-parser branch August 13, 2019 07:25
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.

4 participants