Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

catch ambiguous step definitions #442

Closed
wants to merge 13 commits into from
Closed

catch ambiguous step definitions #442

wants to merge 13 commits into from

Conversation

charlierudolph
Copy link
Member

@jbpros

resolves #176

@jbpros
Copy link
Member

jbpros commented Oct 28, 2015

@charlierudolph you wrote that ambiguous steps shouldn't throw an error but simply be marked as ambiguous. I think they should be failures. Ambiguous steps are the result of a mistake in stepdef patterns that needs to be fixed.

Here is an example of the consequences this can have: consider a scenario which is passing. One of its steps suddenly becomes ambiguous because of a new step definition. That scenario will be reported as ambiguous, which is not considered a failure (Cucumber's return code is still 0). Let's assume the behaviour described in that scenario was modified and got broken recently. It is now possible that nobody will pay attention to it: the build is still green but in fact that piece of behaviour is not tested anymore and is broken.

The strict mode would prevent that situation but we cannot expect everyone to switch it on.

I like the ambiguous scenario/step state this PR introduces. But I think it should be reported as a hard failure (in red!).

@jlin412
Copy link

jlin412 commented Oct 28, 2015

@jbpros Though I prefer your approach, it will deviate from the standard in other languages. I use Ruby cucumber before so I know ambiguous scenario/step state should only fail in strict mode. I think that it may be sufficient for me as long as there is a warning message so I can debug.

@charlierudolph
Copy link
Member Author

In cucumber ruby, ambiguous steps throw an error, stopping the run. @jbpros I consider ambiguous steps similar to undefined steps as for both cucumber can't get the step definition for the step. One is caused by a lack of step definitions and one is too many step definitions.

I'm happy to make ambiguous steps cause an exit code of 1, but I think undefined steps need to be treated the same way.

@jbpros
Copy link
Member

jbpros commented Oct 28, 2015

I quite like that idea, @charlierudolph. It could be discussed on https://github.com/cucumber/cucumber/issues.

However, one could argue that undefined steps are like empty examples in mocha (it("does something") with no function passed as param #2), which I believe will not be considered an error, just "not ready yet".

@charlierudolph
Copy link
Member Author

Empty examples in mocha are pending tests and we have our own version of pending steps (though I've suggested before switching to mocha's approach for pending steps)

charlierudolph added 2 commits November 1, 2015 09:53
@charlierudolph
Copy link
Member Author

Alright @jbpros this is updated so ambiguous steps are by treated as failures. Used "magenta" for the color at the moment since ambiguous is its own status.

@jbpros
Copy link
Member

jbpros commented Nov 5, 2015

👍 good stuff!

# Conflicts:
#	package.json
@lock
Copy link

lock bot commented Oct 25, 2018

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.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicated stepdefs do not throwing any error?!
3 participants