with_filtered_backtrace called on Test::Result::Unknown with strange feature file

[krukow]

To reproduce, create a trivial feature file:

➜  tmp mkdir features
➜  tmp touch features/accidental.feature

edit the file to become:

➜  tmp cat features/accidental.feature
Feature: A

  Scenario: S
  Scenario: S
    Given I begin

and run dry-run: cucumber -d

so you get:

undefined method `with_filtered_backtrace' for #<Cucumber::Core::Test::Result::Unknown:0x007fa2d1e32b98> (NoMethodError)
/Users/krukow/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/cucumber-2.3.3/lib/cucumber/formatter/legacy_api/adapter.rb:44:in `after_test_case'
/Users/krukow/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/cucumber-2.3.3/lib/cucumber/formatter/fanout.rb:16:in `block in method_missing'
/Users/krukow/.rbenv/versions/2.2.2/lib/ruby/gems/2.2.0/gems/cucumber-2.3.3/lib/cucumber/formatter/fanout.rb:15:in `each'

[danascheider]

I was able to repro this issue on my machine and determined it was introduced in version 2.0.1. When I remove the first Scenario: S (the empty scenario), the error goes away. I'll look into fixing this - at the least, it seems like it should fail gracefully.

[danascheider]

I created a PR against the cucumber-ruby-core repo correcting this error by adding a #with_filtered_backtrace method to the Cucumber::Core::Test::Result::Unknown class. cucumber/cucumber-ruby-core#107

EDIT: Once that PR is merged, we will need to test to make sure that it fully addresses this issue.

[brasmusson]

In Gherkin v4.0 the compiler does not create a pickle/test case if there are no steps in a scenario (see for instance feature file and pickles reference). Cucumber-Ruby does not use the Gherkin compiler, but maybe also the compiler of Cucumber-Ruby(-Core) should also be changed to not create test cases for scenarios without steps.

[danascheider]

That would probably be a more ideal solution.

[mattwynne]

IMO the most reasonable behaviour for an empty scenario like this is an undefined result, since the scenario has yet to be defined with any steps.

in the pickles model, this empty scenario would disappear from the results, but I don't think that's what a business user who has recorded the name of the scenario would expect to happen.

I wonder if we should get rid of the Result::Unknown status altogether and default to Undefined

[danascheider]

Maybe, although I've written scenarios like this just with the intention of not letting myself forget which ones I meant to write. I didn't notice or care whether they were included in the results.

[danascheider]

Then again, I guess I am not really a business user, per se.

[mattwynne]

Anyway that's an academic discussion we can have later. This looks like a good fix for @krukow's immediate problem.

[danascheider]

I'm looking at the core code to figure out how I can make the result undefined since I agree that would be better. Shouldn't be too hard. (Famous last words!)

[mattwynne]

I'm looking at the core code to figure out how I can make the result undefined since I agree that would be better. Shouldn't be too hard. (Famous last words!)

How about we merge the current solution first and then do that as a second iteration?

[danascheider]

Works for me.

[krukow]

thanks 👍

[brasmusson]

I'm looking at the core code to figure out how I can make the result undefined since I agree that would be better. Shouldn't be too hard. (Famous last words!)

How about we merge the current solution first and then do that as a second iteration?

Could this hide problem that should be fixed. If the Result::Unknown status should not end up somewhere but it does, isn't failing immediately better than continue?

[danascheider]

@brasmusson I don't think so, I just think we should open a second issue to make sure the better fix stays on our radar.

[brasmusson]

IMO the most reasonable behaviour for an empty scenario like this is an undefined result, since the scenario has yet to be defined with any steps.

The current behaviour when running and empty scenario is that its result is passed, so is it really reasonable that when using --dry-run its result becomes undefined?

[brasmusson]

The root cause of this issue is that when executing and empty scenario there will still be hooks executed (for instance the hooks applied by the PrepareWorld filter), which will change the status/result of the test case from Unknown to Passed, before any scenario step is executed. But when using --dry-run only the steps of the scenario will be "executed" by the runner and the status/result of the test case will changed from Unknownto either Skipped or Undefined. However if the scenario is empty nothing is changing the status/result of the test case from Unknown.

One option for fixing this root cause (to mimic the difference when using --dry-run or not for scenarios will steps), would be to when using --dry-run apply a SkippingBeforeHook (with the same behaviour as SkippingStepMatch) to change the status/result of the test case from Unknown to Skipped even when the scenario is empty.

[danascheider]

Truthfully, I think it's more reasonable that the result be undefined even when there is no --dry-run flag.

[mattwynne]

Truthfully, I think it's more reasonable that the result be undefined even when there is no --dry-run flag.

👍

Having thought about it, I don't think we can simply eliminate the concept of an Unknown result - after all that is the true status of a test step that has been activated but is yet to be executed. I hadn't thought about those filters though Bjorn. Perhaps we need to adjust them to no-op on scenarios with no steps.

We could also possibly add a new filter (on the end of that chain) that implements this new rule about empty scenarios being undefined, by just adding a test step that fails with an undefined result.

[brasmusson]

I see a couple of arguments why undefined is not the right result of an empty scenario. Today undefined for a scenario mean, "there is at least one step the does not have a matching step definition". Using undefined for empty scenarios changes its meaning to something more vague like "you are not finished yet". But then also a scenario with only a Given step should be undefined, because until you have written a Then step you are not finished, right? not-executed seems to be a more accurate status for an empty scenario, since there were no step - nothing was executed.

Thinking ahead to parallel execution (discussed for instance in #924), the runner could in many cases be in a another process or on a another machine then the parser/compiler/UI. It that case there is a need for a specified interface between them. I think a lot of the success with the Gherkin re-write ("Gherkin 3"), parser/compiler implemented in 8 languages and counting, is due to the acceptance test data which are used to test all the implementations. I think that acceptance test data already has defined the interface from the parser/compiler/UI component to the runner component: the pickles. That is one reason why empty scenarios should no reach the runner, the other is, why send data to another machine for execution (a fairly expensive operation) when you know what is going to happen - nothing. I empty scenarios will not be sent to the runner in that case, why do it just because the parser/compiler/UI and the runner executes in the same Ruby interpreter?

I also like to point out that with the event based formatter like https://github.com/cucumber/cucumber-pretty-formatter, the formatter get the content of the feature file passed separate from the data from the runner, so such a formatter are able to determine whether any scenarios in the feature file was not executed. It may not easy/possible for the formatter to determine why, did not match tag expression, fail fast options used and failure occurred, empty scenario, etc, but as least it can determine what was not executed.

[matzke]

--dry-run shouldn't differ ... expected behavior is "run as regular test run" except the execution of steps ...
adding emtpy steps is "hacky" (just my 2cents)

[mattwynne]

I've manually tested Karl's bug and this should now be fixed in the next release. We can discuss this other issue (about how to handle scenarios without steps) elsewhere.

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