cucumber-attic / gherkin2

A fast Gherkin parser in Ragel (The parser behind Cucumber)
MIT License
382 stars 221 forks source link

Handle interleaved calls to step, match and result in the java PrettyFormatter #261

Closed brasmusson closed 10 years ago

brasmusson commented 11 years ago

The java PrettyFormatter has relied on that the call to step() for all the steps in a scenario, come before any call to match() for any step in the scenario. When the call to formatter.step() in cucumber.runtime.model.StepContainer was moved from the format() method to runStep() method in the commit https://github.com/cucumber/cucumber-jvm/commit/4fba53c89f206363004fb81893606f9bd7695c2f this assumption is not true anymore. Therefore IndexOutOfBoundsException is thrown from the PrettyFormatter in Cucumber-JVM version 1.1.3 on the second step of any scenario. This has been noted several times, for instance in https://github.com/cucumber/gherkin/pull/252 and https://github.com/cucumber/cucumber-jvm/issues/491, but proposed solutions have masked the problem rather than adapting to the root cause of the problem.

To solve the root cause of the exception, the java PrettyFormatter in this pull request is changed to handle the interleaved sequence of calls to step(), match() and result() that Cucumber-JVM version 1.1.3 is issuing. The solution in the java PrettyFormatter is to also store the received match and result data in a list until the end of the scenario. Then the indentations for the step definition locations are calculated, and the whole scenario is printed at once.

The final formatted result is not changed, but for long running scenarios the delay of printing until the end of the scenario will be notable. Since the printing is delayed until the end of the scenario, the printing of the current step as executing is also removed. This has the side effect that most of the ANSI up cursor magic mentioned in https://github.com/cucumber/gherkin/issues/187 is removed.

Note, for this pull request the java code has been tested through mvn install in the java directory, with the test passed. Other tests, for instance through rake, have not been performed.

os97673 commented 11 years ago

Tests failed :( Please fix them.

brasmusson commented 11 years ago

Yep, I have seen the failed tests. I was aware - and not very happy about - that I had not executed other tests that the java tests for the change. But I gave up after struggling with making it through 'rake spec' on Ruby 1.9.3 on Windows. When I got an "expecting '...\r\n...' got '...\n...'" from a spec that did not seem related at all to the PrettyFormatter, I just gave up. On the other hand, if I had succeeded with running the tests on Ruby 1.9.3, I would likely have been so happy that is would not occur to me to rerun test test on JRuby.

There are two problems to be handled in the failing spec:

  1. Since the java PrettyFormatter now waits until the end of the Scenario to print it, the formatter need to be told that the scenario is finished, "@f.eof()" need to be added to the failing test cases.
  2. Since the java PrettyFormatter now waits until the end of the Scenario to print it, the usefulness of the "executing" output basically none and therefore has been removed, so the asserts in the failing test cases need to be relaxed to allow but not require that "executing" lines are included in the output.

I have made the changes in the spec and tested them on Ruby 1.9.3, but have not yet succeeded with running that spec on JRuby (JRuby 1.7.4 on Windows - RVM is not available on Windows) with the gherkin jar-file I have build (with maven). Currently I am stuck on the error: NoMethodError: "undefined method 'append' for #StringIO:0x..." for all test cases in the spec (pretty_formatter_spec.rb)

Chances are that I see no other option than to push up those changes without succeeding to have tested them locally, and I do not like that option at all.

brasmusson commented 11 years ago

JRuby, Windows and myself do not go together very well. But finally I have managed to execute the updated failing spec both on the Ruby side and the JRuby+Java side. A new commit is coming soon...

brasmusson commented 10 years ago

Now that https://github.com/cucumber/cucumber-jvm/pull/557 has been merged into Cucumber-JVM, this PR is practice not needed anymore. Unless it is important that the java PrettyFormatter handles both "all steps first" and "one step at the time" execution. Given that Gherkin3 is coming, it is probably not that important.

aslakhellesoy commented 10 years ago

The decision to do

was probably not a good decision, so I'm happy to see this fix. It'll make it easier to revert to the old