extension of the Formatter interface for more precise lifecycle handling

[SierraGolf]

The idea is to provide two new methods that get called at the beginning and at the end of the scenario lifecycle.

Beginning is defined through: before the first "before" hook. End is defined through: after the last "after" hook.

The current implementations have been provided with stub implementations, which could be filled with logic to solve existing bugs like #476 and #570.

The new Formatter methods will be called from within the CucumberScenario.run(Formatter, Reporter, Runtime) method like this:

@Override
public void run(Formatter formatter, Reporter reporter, Runtime runtime) {
    Set<Tag> tags = tagsAndInheritedTags();
    runtime.buildBackendWorlds(reporter, tags);
    formatter.startOfScenarioLifeCycle((Scenario) getGherkinModel());
    runtime.runBeforeHooks(reporter, tags);

    runBackground(formatter, reporter, runtime);
    format(formatter);
    runSteps(reporter, runtime);

    runtime.runAfterHooks(reporter, tags);
    formatter.endOfScenarioLifeCycle((Scenario) getGherkinModel());
    runtime.disposeBackendWorlds();
}

The pull request for the cucumber-jvm project, which will follow, will also include stub implementations.

[SierraGolf]

closing temporary for modification

[os97673]

What about tests for the changes?

[aslakhellesoy]

@SierraGolf you said you were going to add two methods, but there are a lot of other drive-by changes here. Please do this in a separate PR.

Since these methods aren't invoked by Gherkin, but by Cucumber, I think that ideally these methods should not be in Gherkin, but in Cucumber. I do understand that it's more pragmatic to add them here.

When Gherkin3 is available these APIs are likely to change a lot anyway, so I'm OK with keeping this here.

[SierraGolf]

@aslakhellesoy, what do you mean by "drive-by changes"? Is it the documentation I added to the Formatter? I guessed it would fit in quite well into this pull request. Everything else is just stub implementations of the interface which is necessary for compilation.

@os97673, what exactly would you like to test here? this pull request is actually just a preparation for another one on the cucumber-jvm project, which will have tests.

[aslakhellesoy]

@SierraGolf yes, I meant the javadoc changes. They are great additions, I just thought it was unrelated. But thinking about you again I agree with you they fit well with the PR.

Regarding the testing - since the methods you're adding aren't called from Gherkin, there is nothing to test here.

There are leaky abstractions here (execution leaking into parsing interfaces), but I'm fine with it since that will hopefully go away with Gherkin3. I'm also guilty of having added similar leaks here before.

[os97673]

@SierraGolf after re-reading and re-thinking I tend to agree that no testing is required, though I'm not a big fan of java-only API change in Gherkin, especially taking into account that at least JSonFormatter is shared between jruby and java :( Hope Gherkin 3 will be released soon ;)

[SierraGolf]

Sounds interesting, maybe we can have a little chat about Gherkin3 aside from this PR.

While we are at it, one thing that caught my attention while going through the code base and extending the documentation of the Formatter was the way the Formatter.step(Step) is called. In contrast to the other methods it is not called alongside the actual execution of a specific gherkin element, but instead all steps are iterated through the formatter before anyone is actually executed. What is the intention behind that? (There was actually a bug related to that in the AndroidReporter I was working on)

[brasmusson]

I don't know if it was the original intention, but one feature that relies on that the Formatter.step(Step) is called for each step before the actual execution of the first step is the PrettyFormatter's printing of steps as "executing". To print a step at all, the PrettyFormatter need to know the length of the printed text of all the steps (and the scenario), to be able to align the step location correctly.

Some releases ago the Java JSONFormatter did not work with this "all-steps-first" execution, but the Java PrettyFormatter did not work with the opposite "one-step-at-the-time" (so one of them would not work correctly). In next release with #261, both formatters will handle both types of execution. Then the Java PrettyFormatter will not print steps as "executing", which makes it different from the Ruby PrettyFormatter.

[SierraGolf]

@brasmusson ok, so from reading the other PR comments it seems that for the current gherkin version the behaviour will remain "all-steps-first", but it will be "interleaved" in gherkin3. I am understanding that correctly?

[brasmusson]

@SierraGolf With gherkin2 the actual control of execution order is in the Cucumber part (but the "wrong" choice may make formatters from gherkin2 misbehave). Cucumber-JVM is back in "all-steps-first" in 1.1.4 and 1.1.5, which as far as a I know is the same as Cucumber-Ruby always has used. Whether gherkin3 will assert more control over the execution order, or specifically only support one, I do not know.

[aslakhellesoy]

In Gherkin3 there will be an AST that is easier to navigate, so firing all the steps before firing the results will not be necessary.

[SierraGolf]

Ok, thanks for the info.

Anything blocking this PR from being merged?

[aslakhellesoy]

LGTM, I just need to find time to actually do it and then do a release.

[brasmusson]

@os97673 @SierraGolf About testing. Today there are test (in gherkin/java) that ensures that the JSONFormatter and PrettyFormatter produce the intended result when called with the a sequence like:

formatter.uri
formatter.feature
formatter.scenario
formatter.step
formatter.match
formatter.result
formatter.eof
formatter.done
formatter.close

These tests should be extended to include the new methods start/endOfScenarioLifeCycle so that the execution use case of the formatters use the new intended call sequence. The other use case for formatters, parsing (new Parser(formatter).parse()) is not changed by the new methods (right?)

[SierraGolf]

@brasmusson I think as long as those methods don't have an implementation, there is no need to add tests, because what would you test there? In case someone wants to fix the above mentioned bugs or refactor the code he is now free to do it and extend the tests, but imho this should not be part of this PR.

[os97673]

@brasmusson I think as long as those methods don't have an implementation, there is no need to add tests, because what would you test there? In case someone wants to fix the above mentioned bugs or refactor the code he is now free to do it and extend the tests, but imho this should not be part of this PR.

I think @brasmusson is right that we need to modify existing tests to ensure that new calls will not break expected behavior. Yes, the implementation is NOOP, but for (possible) future modifications it would be very helpful to have such tests. Perhaps it is not a must-have for the PR, but it is better to add this here.

[SierraGolf]

Excuse my stubbornness, but isn't that covered by the existing tests? Since I did not add any functionality, there should not be any changes in tests, furthermore if all existing tests are passing (assuming the coverage is high), this should be proof that the new "noop" implementations did not alter the existing behaviour.

[os97673]

Excuse my stubbornness, but isn't that covered by the existing tests?

No problem :) You have not changed the functionality, but you have changed public interface, and the new/changed API is supposed to be used in certain way, thus it is better to write new/update existing tests to document this expectation and verify that behavior is correct.
Yes, it will be test-only change and your implemenation will not break anything, but future maintainer of the code may be unaware about the expected use of the API or expect that it is already covered by existing tests and thus introduce a bug by modifying the code. So, we do not need these tests to verify that your code is correct, but we do need them for future to verify that our changes will be correct. Hope I was able to explain why it could be useful, sorry for long explanation ;)

[SierraGolf]

Yeah, actually I realised what you were referring to on my way to work. I guess I should add the calls to the new methods inside the existing tests so that they represent the actual order of execution correctly. I will append a new commit to this PR soon. Thanks for being so patient with me.

[os97673]

Thanks for being so patient with me.

No problem. I do appreciate your contribution so I'm happy to help you improve it (otherwise I will have to do this myself ;)