Reporting errors from hooks

[mattwynne]

If you run cucumber features/docs/writing_support_code/tagged_hooks.feature:22 on the new-core branch, you'll see that the Before hook raises an error correctly, causing the step to be skipped. All that's wrong here is that the error is not reported in the formatter.

[mattwynne]

@os97673 have a look at this one if you want. I've got my head around it now so I can help if you get stuck.

[os97673]

Sure, I will.

[os97673]

@mattwynne as far as I can see the problem is not the exception is not printed, but that its location is incorrect :(
We expect "./features/support/hooks.rb:2:in Before'" but cucumber reports "./features/support/hooks.rb:2:inlocation'"

[os97673]

@mattwynne I've fixed the scenario somehow (see comment for the latter commit). Will investigate why specs are failing later.

[os97673]

Looks like specs a re failing due to #563
So, the only unresolved problem is source indentation calculation.
We should either fix it or change PrettyFormatter to not add additional space.

[os97673]

@mattwynne @tooky I've reverted my changes in expected output and changed PrettyFormatter to use new indent correctly. But it would be nice to get your feedback on this.

[tooky]

@os97673 there's a ReportAdapter which is where we are joining the new core to the formatters - I think this line might be the problem: https://github.com/cucumber/cucumber/blob/new-core/lib/cucumber/formatter/report_adapter.rb#L428

[os97673]

@tooky I'm not sure which line you tried to point (the line you pointed contains class Indent) :(

[mattwynne]

@os97673 try https://github.com/cucumber/cucumber/blob/new-core/lib/cucumber/formatter/report_adapter.rb#L460

[os97673]

@mattwynne I've see this line and decided that this is correct calculation and it is PrettyFormatter who should not add additional space after it. Does this sound reasonable for you?

[os97673]

@mattwynne @tooky it looks like we currently call before hook BEFORE a scenario, but the tests expect to have exception reported inside the scenario thus we should either store the exception somehow and report it later or run the hook as the very first step INSIDE a scenario. I personally prefer the latter. So what do you think?

[brasmusson]

@os97673 @mattwynne @tooky If that is the case then it is a change from the behavior of Cucumber 1.3.x, but interestingly enough the same behavior as Cucumber-JVM. When working on cucumber/gherkin#270 I concluded that Cucumber (1.3.x) and Cucumber-JVM behaves differently with respect to the call sequence that a formatter experience for scenarios with before hooks. Cucumber call the formatter in the sequence:

Scenario
Before
Step
...

whereas Cucumber-JVM call the formatter in the sequence:

Before
Scenario
Step
...

As a consequence the test of before hook results had to be removed from json_parser_spec.rb, since the Ruby JsonFormatter stores the before hook result in the scenario of the latest call to Scenario (to work together with Cucumber), and the Java JsonFormatter stores the before hook result in the scenario of the next call Scenario (to work together with Cucumber-JVM). With different behavior of the handling of before hooks results, a spec common to both has a hard time test that aspect of the formatters behavior.
What behavior is "right", "best" or what I would personally prefer, I have not formed an opinion of (yet, at least), I just what to point out what I got out from earlier analysis.

[mattwynne]

@os97673 it's not quite a simple as that. The compilation that happens in core[1][2] makes a Cucumber::Core::Test::Case which contains several "test steps". Some of those may be Cucumber::Core::Test::HookSteps, and some may be regular Cucumber::Core::Test::Steps from a scenario or scenario outline.

Both types of test steps have the same interface, which includes a describe_source_to method that allows you to discover the original AST source nodes that the test step was generated from. So for example:

Feature: a feature
  Scenario: a scenario
    Given step 1
    And step 2

This would generate a single Test::Case with two Test::Steps. If you call describe_source_to on the first Test::Step you'll be called back with:

feature('a feature'), scenario('a scenario'), step('step 1')

When we execute the test case, the core calls the report API with the after_test_step message for each of those steps. We then call back on the test step to ask it to describe its source. That happens here: https://github.com/cucumber/cucumber/blob/master/lib/cucumber/formatter/report_adapter.rb#L30

The thing is that right now, the hook doesn't consider the AST scenario to be part of it's source. So when you ask it to describe it's source, it doesn't tell you about the scenario. I think that's arguable. If we change the way the Cucumber::Core::HookCompiler works to include the test case's source (i.e. the scenario) then that might solve this problem.

I hope that makes some kind of sense. Please do ask more questions - the more people who understand this new model the better.

[1] https://github.com/cucumber/cucumber-ruby-core/blob/master/lib/cucumber/core/compiler.rb
[2] https://github.com/cucumber/cucumber-ruby-core/blob/master/lib/cucumber/core/test/hook_compiler.rb

[os97673]

@mattwynne base on your explanation I'd say that scenario should be part of hook's source.
If you do not know a reason why this may be a bad idea I'll try to implement this.

[mattwynne]

@os97673 I've remembered why this might be a bad idea.

If the scenario has a background, the hook won't know about that. Here's how the compiled nodes would look for this scenario:

Feature: f
  Background: b
    Given b1

  Scenario: s
    Given s1

This is pseudo-code - you can't really transform the test tree into JSON!

{
  test_cases: [
    { 
      source: [ scenario: "s", feature: "f" ],
      test_steps: [
        { source: [ { hook: "features/support/hooks:15" }, { scenario: "s"}, { feature: "f" } ] },
        { source: [ { step: "b1" }, { background: "b" }, { feature: "f" } ] },
        { source: [ { step: "s1" }, { scenario: "s" }, { feature: "f" } ] },
      ]
  ]
}

So when the hook is printed, we'll look at it's source, print the scenario, and then only when the first step runs we'll print the background. That's wrong.

I'm pretty sure the current behaviour is that the background steps should be printed (showing status skipped, which they should have also in the new model) and then the hook error is printed after the Scenario: keyword is output. So it seems like we'll need to store the hook exception somewhere and look for it as we first print the scenario.

A bit messy, but it preserves existing behaviour at least. If you can see a way to improve the model above to allow for this elegantly, please suggest!

[brasmusson]

@mattwynne I double checked in Cucumber v1.3.5, and it is a little more complex than that. If a feature has background, the before hook exception will be printed by the PrettyFormatter after the Background: keyword line when the first scenario of the feature is executed. For the other scenarios of the feature than the first (or if the feature has no background), the exception will be printed after the Scenario: keyword line as you said.

To preserve this behaviour it seems that the ReporterAdapter need to store the before hook results, and when triggering the Background: or Scenario: output from the formatter, check if there are any stored before hook results to handle and let the formatter handle them as well.

[mattwynne]

@brasmusson that makes sense. Thanks for looking into it for us.

[mattwynne]

Looks like this is dealt with in other tickets now.

[lock]

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.