Adds scenario for the ambiguous step reporting #1132

MadameSheema · 2017-06-25T13:46:02Z

Summary

Added scenario about how the reporting of ambiguous step match should be. See #1113

Details

Spike of an example we think reporting could be.

Motivation and Context

Fixes #1113

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

I've added tests for my code
My change requires a change to the documentation.
I have updated the documentation accordingly.

MadameSheema · 2017-06-25T13:47:27Z

@enkessler could you please review it? :D

tooky · 2017-06-26T13:43:22Z

Thanks @MadameSheema!

I think we need to have a look at exactly what we want the behaviour for ambiguous steps to be.

I think the choice is that the step is either Pending or Failed. This PR recommends Failed.

It could be pending because the user needs to give Cucumber more information so that it can decide what to run here.

It could be failed because there is a problem with the test automation code and Cucumber is unable to execute the step.

enkessler · 2017-06-26T13:50:58Z

With --strict, an ambiguous step should definitely fail (red) and, with --guess, an ambiguous step should not be auto-failed (green, assuming the step doesn't otherwise raise an error). Lacking either of those flags, I'd lean towards some flavor of 'yellow', which I think means that it ought to be treated similar to a pending/undefined step.

brasmusson · 2017-06-26T14:21:27Z

I think Cucumber-Ruby should do what Cucumber-JS does and have a result type for Ambiguous. Without --strict I think an Ambiguous result should affect the exit code as Pending and Undefined results (exit code 0 if not --strict, exit code not 0 if --strict). With --guess thinks start to get interesting, then a step is executed (with a pass or fail) - should the result of the step be the result of the execution, or should some extra informations say something about that there were and ambiguity the was resolved by guessing?

mattwynne · 2017-06-26T20:53:16Z

It sounds like we'll need at least three scenarios here:

strict mode => exit 1
not in strict mode => exit 0
guess mode => exit 0

I'm pretty sure guess mode has always just quietly done its job in the past. I think we should focus on helping @MadameSheema get this back to working and then deal with making guess mode more fancy. WDYT?

mattwynne · 2017-06-26T20:54:10Z

features/docs/defining_steps/ambiguous_steps.feature

+      * a step
+      * an ambiguous step
+
+      Scenario: test 2


I'm wondering why we need this second scenario. I'd find it easier to read this example if it wasn't there. Do we need it to illustrate the behaviour?

@matt it's there because the current behaviour is that it just crashes the whole test run and doesn't run he second scenario.

I suppose that if it reported the status correctly it wouldn't matter how many scenarios there are.

mattwynne · 2017-06-26T20:54:49Z

features/docs/defining_steps/ambiguous_steps.feature

+    """
+    Feature:
+
+      Scenario: test 1


I tend to not even bother naming scenarios used as tests unless it adds something to the documentation. In this case I suggest you can leave it blank.

mattwynne · 2017-06-26T20:56:52Z

features/docs/defining_steps/ambiguous_steps.feature

@@ -0,0 +1,63 @@
+Feature: Ambiguous Steps


If you tag this feature as @wip then (I think) it will be expected to fail in CI, so your branch will stay green. Once we have it passing we can remove the @wip tag.

mattwynne · 2017-06-26T20:59:14Z

features/docs/defining_steps/ambiguous_steps.feature

@@ -0,0 +1,63 @@
+Feature: Ambiguous Steps
+


It would be good to add some documentation here about this feature. Something like:

When Cucumber searches for a step definition for a step, it might find multiple step definitions that could match. In that case, it will give you an error that the step definitions are ambiguous. You can also use a `--guess` mode, where it uses magic powers to try and figure out which of those two step definitions is most likely to be the one you meant it to use. Use it with caution!

mattwynne · 2017-06-26T21:01:57Z

@charlierudolph do you alter the behaviour of cucumber-js for ambiguous steps depending on strict mode, or does it just always fail?

mattwynne · 2017-06-26T21:02:48Z

@brasmusson I agree about having an Ambiguous result type.

charlierudolph · 2017-06-27T07:18:44Z

In cucumber-js ambiguous steps cause a build to fail by default (thus they are more similar to failed steps then undefined / pending), @jbpros and I debated that here: cucumber/cucumber-js#442. Thus the behavior does not change depending on strict mode.

mattwynne · 2017-06-27T07:24:36Z

Based on Charlie's comment then I suggest we stay consistent with JS, keep it simple and ignore strict mode for now. So only two scenarios needed - with / without guess mode

MadameSheema · 2017-07-03T19:08:26Z

@mattwynne could you please take a look to the scenarios? thx :)

mattwynne · 2017-07-03T21:18:22Z

I like this much better now!

I'm wondering why you've used the * keyword instead of Given / When / Then which is probably more familiar to most people. Is there a reason for that?

tooky · 2017-07-04T07:57:00Z

features/docs/defining_steps/ambiguous_steps.feature

+    You can run again with --guess to make Cucumber be more smart about it
+     (Cucumber::Ambiguous)
+
+    """


I think the other thing that's important for this scenario is to see the summary report:

1 scenario (1 failed) 2 steps (1 failed, 1 passed) 0m0.012s

I think we see an error like this in the current behaviour, but it also drops out of the running cucumber process and doesn't print the summary report?

@tooky right now, when we have an ambiguous step and we execute it without the guess mode the summary report is not displayed. I can add it as an expected behaviour :)
Is there anything else we should take into consideration for this scenario? Thx!

@MadameSheema I think we should. The issue that we based this PR on #1113 was reported because the current behaviour is wrong. When we are running without --guess we want to report this as a failed step, and continue the run as normal. So we should see the summary.

Thanks for sticking with this!

MadameSheema · 2017-07-04T19:53:55Z

@mattwynne because the example we started working on it was written like that 😊

mattwynne · 2017-07-06T21:42:00Z

It looks like the problem here is that the Ambiguous exception is being raised too soon - while we're preparing the test cases for execution, rather than when they're actually being run.

I think the change for this needs to be done in https://github.com/cucumber/cucumber-ruby/blob/master/lib/cucumber/filters/activate_steps.rb which is where we search for a step definition for each test step, and activate it with the definition if we find one.

It's probably a matter of altering the method here to understand about ambiguous steps. We could probably use a similar mechanism to the SkippingStepMatch to pass back a block that will raise that ambiguous exception when the step is actually executed.

It's going to take some fiddling around with, and might need a spike / pairing session with someone who knows the code well to avoid a lot of head-scratching. This is essentially a regression from when we went from 1.0 to 2.0 and rewrote a lot of the internals. Unfortunately because this didn't have an acceptance tests we never noticed. Thanks for adding one @MadameSheema!

Taught Cucumber how to match against ambiguous steps.

olleolleolle · 2017-07-10T06:28:41Z

lib/cucumber/step_match.rb

+    end
+
+    def activate(test_step)
+      return test_step.with_action { raise Core::Test::Result::Ambiguous.new(@error.message) }


Is the return keyword doing anything here?

Given that return value of a Ruby method is the last thing evaluated? No, it's just there because that's how someone wrote the code that I am basing this code on.

I don't necessarily understand why established patterns are there, but I can at least follow them as I attempt to fix code that I am entirely unfamiliar with. :)

mattwynne · 2017-07-10T06:57:12Z

What does the failing test say now? Can you paste the output you're seeing when you run it?

enkessler · 2017-07-10T14:24:24Z

@mattwynne Currently we see

Feature: 

  Scenario:                # features/ambiguous.feature:3
    When a step            # features/step_definitions.rb:1
    Then an ambiguous step # features/ambiguous.feature:5

1 scenario (1 failed)
2 steps (1 failed, 1 passed)

but we expect something like

Feature: 

  Scenario:                # features/ambiguous.feature:3
    When a step            # features/step_definitions.rb:1
    Then an ambiguous step # features/ambiguous.feature:5

Ambiguous match of "an ambiguous step":

    features/step_definitions.rb:1:in `/^a.*step$/'
    features/step_definitions.rb:5:in `/^an ambiguous step$/'

    You can run again with --guess to make Cucumber be more smart about it
     (Cucumber::Ambiguous)

1 scenario (1 failed)
2 steps (1 failed, 1 passed)

which is basically what it is already doing except for shoving in the helpful error snippet. This error is already made in the error class itself

    def initialize(step_name, step_definitions, used_guess)
      message = String.new
      message << "Ambiguous match of \"#{step_name}\":\n\n"
      message << step_definitions.map{|sd| sd.backtrace_line}.join("\n")
      message << "\n\n"
      message << "You can run again with --guess to make Cucumber be more smart about it\n" unless used_guess
      super(message)
    end

but it's not getting output anywhere. From what I have seen of the code, I think that outputting it happens somewhere in cucumber-ruby-core. That, or the pretty formatter is supposed to be catching something and handling it.

brasmusson · 2017-07-16T18:30:49Z

The PR should be rebased to take advantage of the ground work in #1158.

but we expect something like
Ambiguous match of "an ambiguous step":
features/step_definitions.rb:1:in /^a.*step$/' features/step_definitions.rb:5:in /^an ambiguous step$/'
You can run again with --guess to make Cucumber be more smart about it
(Cucumber::Ambiguous)

This is printed by the formatter, so (some of) the formatters need to be updated to handle ambiguous results (and some other things like defining a colour for ambiguous in ansicolor.rb)

To get something up and running I recommend using the progress formatter instead of the pretty formatter, since the pretty formatter uses the legacy_api quagmire.

aslakhellesoy · 2017-08-02T14:04:14Z

Hi @MadameSheema,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for we ask you to:

✅ Please continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

[ci skip]

brasmusson · 2017-08-02T14:19:47Z

I absolutely think that ambiguous steps should have a separate result type, but I also think that it is not necessary for v3.0.0. However, since I think fixing #1113 is required for v3.0.0, I merge an implementation reporting ambiguous steps as failed steps, and leave the rest for #1168.

lock · 2018-10-25T00:07:33Z

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.

mattwynne requested changes Jun 26, 2017

View reviewed changes

MadameSheema force-pushed the 1123-scenario_gracefully_reporting_ambiguous_steps branch from e0d7784 to 97082b2 Compare July 3, 2017 18:44

tooky reviewed Jul 4, 2017

View reviewed changes

mattwynne added status: work in progress (WIP) 🐛 bug Defect / Bug labels Jul 4, 2017

brasmusson mentioned this pull request Jul 6, 2017

[Core, JUnit, Android] Add the ambiguous result type. cucumber/cucumber-jvm#1168

Merged

6 tasks

GloriaHornero added 9 commits July 9, 2017 17:53

Adds scenario for the ambiguous step reporting

1a5ddb3

Adds @wip tag

452e2cd

Adds feature documentation

77ebe87

Simplifies tests scenarios by removing the scenario name

af9a5ab

Adds ambiguous tests with guess mode scenario

d980346

Simplifies ambiguous steps scenario

5242c69

Improves test scenarios by adding keywords

c233450

Adds summary report to ambiguous steps scenario

f3660a1

Adds @wip tag to ambiguous steps scenario

e1c1200

MadameSheema force-pushed the 1123-scenario_gracefully_reporting_ambiguous_steps branch from a591f3b to e1c1200 Compare July 9, 2017 19:30

Add ambiguous step matching

b39f24f

Taught Cucumber how to match against ambiguous steps.

MadameSheema mentioned this pull request Jul 9, 2017

Add ambiguous result type cucumber/cucumber-ruby-core#139

Closed

6 tasks

olleolleolle reviewed Jul 10, 2017

View reviewed changes

brasmusson mentioned this pull request Jul 13, 2017

List all not ok scenarios in the summary #1158

Merged

6 tasks

Fix the feature and implementation of ambiguous steps.

041536a

brasmusson merged commit f8dbb56 into cucumber:master Aug 2, 2017

ghost removed the status: work in progress (WIP) label Aug 2, 2017

brasmusson added a commit that referenced this pull request Aug 2, 2017

Attribution #1132.

297ea1c

[ci skip]

lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds scenario for the ambiguous step reporting #1132

Adds scenario for the ambiguous step reporting #1132

MadameSheema commented Jun 25, 2017 •

edited by brasmusson

Loading

MadameSheema commented Jun 25, 2017

tooky commented Jun 26, 2017 •

edited by mattwynne

Loading

enkessler commented Jun 26, 2017

brasmusson commented Jun 26, 2017

mattwynne commented Jun 26, 2017

mattwynne Jun 26, 2017

tooky Jun 26, 2017

mattwynne Jun 26, 2017

mattwynne Jun 26, 2017

mattwynne Jun 26, 2017

mattwynne commented Jun 26, 2017

mattwynne commented Jun 26, 2017

charlierudolph commented Jun 27, 2017 •

edited

Loading

mattwynne commented Jun 27, 2017

MadameSheema commented Jul 3, 2017

mattwynne commented Jul 3, 2017 •

edited

Loading

tooky Jul 4, 2017

MadameSheema Jul 4, 2017

tooky Jul 4, 2017

MadameSheema commented Jul 4, 2017

mattwynne commented Jul 6, 2017

olleolleolle Jul 10, 2017

enkessler Jul 10, 2017

mattwynne commented Jul 10, 2017

enkessler commented Jul 10, 2017 •

edited

Loading

brasmusson commented Jul 16, 2017

aslakhellesoy commented Aug 2, 2017

brasmusson commented Aug 2, 2017

lock bot commented Oct 25, 2018

Adds scenario for the ambiguous step reporting #1132

Adds scenario for the ambiguous step reporting #1132

Conversation

MadameSheema commented Jun 25, 2017 • edited by brasmusson Loading

Summary

Details

Motivation and Context

Types of changes

Checklist:

MadameSheema commented Jun 25, 2017

tooky commented Jun 26, 2017 • edited by mattwynne Loading

enkessler commented Jun 26, 2017

brasmusson commented Jun 26, 2017

mattwynne commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwynne commented Jun 26, 2017

mattwynne commented Jun 26, 2017

charlierudolph commented Jun 27, 2017 • edited Loading

mattwynne commented Jun 27, 2017

MadameSheema commented Jul 3, 2017

mattwynne commented Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MadameSheema commented Jul 4, 2017

mattwynne commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwynne commented Jul 10, 2017

enkessler commented Jul 10, 2017 • edited Loading

brasmusson commented Jul 16, 2017

aslakhellesoy commented Aug 2, 2017

brasmusson commented Aug 2, 2017

lock bot commented Oct 25, 2018

MadameSheema commented Jun 25, 2017 •

edited by brasmusson

Loading

tooky commented Jun 26, 2017 •

edited by mattwynne

Loading

charlierudolph commented Jun 27, 2017 •

edited

Loading

mattwynne commented Jul 3, 2017 •

edited

Loading

enkessler commented Jul 10, 2017 •

edited

Loading