Add ambiguous result type #139

MadameSheema · 2017-07-09T21:12:39Z

Summary

Added a new result type for tests that encounter ambiguous steps.

Details

With this PR and the PR: cucumber/cucumber-ruby#1132 we are trying to fix the issue #1113

Motivation and Context

cucumber/cucumber-ruby#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.

Added a new result type for tests that encounter ambiguous steps.

MadameSheema · 2017-07-09T21:13:50Z

@mattwynne @tooky please take a look at this and the new changes in the old PR in order to know if we are going to the right direction or not. Thx :)

mattwynne · 2017-07-09T22:21:49Z

Hi @MadameSheema you're definitely going in the right direction! Using this Raisable type should automatically count the step as failed if you raise it as the step is executed.

I think there's a subtle question about whether this specialisation of the Result::Raisable needs to live in the core, or whether it could just live in the "front-end" cucumber gem. However I think let's leave that question for now and we can look at refactoring once you have something working.

enkessler · 2017-07-09T22:56:21Z

@mattwynne So far, we've been putting the new stuff next to its relatives. So if these new classes don't belong here, then there's a good chance that the existing ones don't belong here either.

We've got one of the tests to the point where it just fails instead of exploding, but we haven't yet found the spot in the code that is responsible for outputting the ambiguous step message. The message that is in the error class class looks exactly like what we want but we need to get it output somewhere.

brasmusson · 2017-07-10T18:29:29Z

Using this Raisable type should automatically count the step as failed if you raise it as the step is executed.

@mattwynne Really? Undefined, Skipped, and Pending are Raisable, but not Failed. Isn't it the other way around, when a Raisable is raised in the step definition the result becomes the type raised? But this is what we want here, that the result becomes Ambiguous?

brasmusson · 2017-07-10T18:30:18Z

lib/cucumber/core/test/result.rb

+          end
+
+          def to_s
+            "F"


This should rather be "A" - for "Ambiguous".

I agree, I hit this in Cucumber JS the other day, and I wonder if we should just do exactly the same as them to stay consistent (except offering to --guess).

💡 in fact, why don't we just steal their acceptance test...

https://github.com/cucumber/cucumber-js/blob/master/features/ambiguous_step.feature

Theft? In Open Source? For shame! ;)

Seriously though, yes, that was an initial idea but it somehow ended up starting with the even more basic case of failure rather than a special return type like ambiguous. Given what i have now seen of the code, however, I may be able to make it return the correct type, even if I still can't get the output correct.

Let's steal the idea :P

starting with the even more basic case of failure

In that case, an Ambiguous class should not be defined here, so defining it here implies a new result category in the summary (N Scenarios (X failed, Y ambiguous, ... )

brasmusson · 2017-07-10T18:31:46Z

lib/cucumber/core/test/result.rb

+          end
+
+          def ok?(be_strict = false)
+            !be_strict


Shouldn't this be false so that the exit code becomes non-zero when an ambiguous step is found?

It's based on what the Undefined type is does. It's not good but it's not bad unless --strict is used.

But the feature file says Then it should fail with:, and if it is not bad unless --strict then the scenario should say Then it should pass with:

brasmusson · 2017-07-10T18:38:58Z

The other result types have specs in result_spec.rb, so it should be updated to cover also the Ambiguous result type.

mattwynne · 2017-07-11T16:02:01Z

@mattwynne Really? Undefined, Skipped, and Pending are Raisable, but not Failed. Isn't it the other way around, when a Raisable is raised in the step definition the result becomes the type raised? But this is what we want here, that the result becomes Ambiguous?

You're right, as usual. 😀 Sorry for adding any confusion.

brasmusson

This PR needs to be rebased since #140 has made some ground work for adding new result types.

brasmusson · 2017-07-16T18:05:31Z

lib/cucumber/core/test/result.rb

+          end
+
+          def ok?(be_strict = false)
+            !be_strict


But the feature file says Then it should fail with:, and if it is not bad unless --strict then the scenario should say Then it should pass with:

brasmusson · 2017-07-16T18:09:09Z

lib/cucumber/core/test/result.rb

+          end
+
+          def to_s
+            "F"


starting with the even more basic case of failure

In that case, an Ambiguous class should not be defined here, so defining it here implies a new result category in the summary (N Scenarios (X failed, Y ambiguous, ... )

brasmusson · 2017-07-16T18:11:17Z

lib/cucumber/core/test/result.rb

@@ -16,7 +16,7 @@ def self.query_methods(result_type)
              result_type
            end

-            [:passed, :failed, :undefined, :unknown, :skipped, :pending].each do |possible_result_type|
+            [:passed, :failed, :undefined, :unknown, :skipped, :pending, :ambiguous].each do |possible_result_type|


These symbols definitions has been moved to a module level constant, so :ambiguous should be added there instead, and since the order control the order in the summary listing, I think is should be defined directly after :failed

brasmusson · 2017-07-16T18:15:45Z

lib/cucumber/core/test/result.rb

+          include Result.query_methods :ambiguous
+
+          def describe_to(visitor, *args)
+            visitor.failed(*args)


The ambiguous result should describe itself as ambiguous to the visitors (which also requires and ambiguous method in the RunningTestCase - same definition as undefined works)

brasmusson · 2017-07-16T18:19:14Z

lib/cucumber/core/test/result.rb

+            "F"
+          end
+
+          def ok?(be_strict = false)


Now the ok? methods are defined as class methods - and for Ambiguous is should always be false (as long as the scenarios says Then it should fail with)

stale · 2017-11-08T09:43:11Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

Add ambiguous result type

944461d

Added a new result type for tests that encounter ambiguous steps.

MadameSheema closed this Jul 9, 2017

MadameSheema reopened this Jul 9, 2017

brasmusson reviewed Jul 10, 2017

View reviewed changes

brasmusson requested changes Jul 16, 2017

View reviewed changes

brasmusson mentioned this pull request Aug 2, 2017

Use a special result type for ambiguous steps and scenarios cucumber/cucumber-ruby#1168

Closed

6 tasks

stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label Nov 8, 2017

brasmusson added the 🧷 pinned Tells Stalebot not to close this issue label Nov 10, 2017

stale bot removed the ⌛ stale Will soon be closed by stalebot unless there is activity label Nov 10, 2017

mattwynne closed this May 1, 2021

mattwynne deleted the branch cucumber:master May 1, 2021 05:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ambiguous result type #139

Add ambiguous result type #139

MadameSheema commented Jul 9, 2017 •

edited by enkessler

Loading

MadameSheema commented Jul 9, 2017

mattwynne commented Jul 9, 2017 •

edited

Loading

enkessler commented Jul 9, 2017

brasmusson commented Jul 10, 2017

brasmusson Jul 10, 2017

tooky Jul 10, 2017

enkessler Jul 10, 2017

MadameSheema Jul 11, 2017

brasmusson Jul 16, 2017

brasmusson Jul 10, 2017

enkessler Jul 11, 2017

brasmusson Jul 16, 2017

brasmusson commented Jul 10, 2017

mattwynne commented Jul 11, 2017 •

edited

Loading

brasmusson left a comment

brasmusson Jul 16, 2017

brasmusson Jul 16, 2017

brasmusson Jul 16, 2017

brasmusson Jul 16, 2017

brasmusson Jul 16, 2017

stale bot commented Nov 8, 2017

Add ambiguous result type #139

Add ambiguous result type #139

Conversation

MadameSheema commented Jul 9, 2017 • edited by enkessler Loading

Summary

Details

Motivation and Context

Types of changes

Checklist:

MadameSheema commented Jul 9, 2017

mattwynne commented Jul 9, 2017 • edited Loading

enkessler commented Jul 9, 2017

brasmusson commented Jul 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brasmusson commented Jul 10, 2017

mattwynne commented Jul 11, 2017 • edited Loading

brasmusson left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Nov 8, 2017

MadameSheema commented Jul 9, 2017 •

edited by enkessler

Loading

mattwynne commented Jul 9, 2017 •

edited

Loading

mattwynne commented Jul 11, 2017 •

edited

Loading