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

fail in AfterStep leaves step successful but scenario failed #1101

Closed
rosskevin opened this Issue Mar 31, 2017 · 11 comments

Comments

Projects
None yet
5 participants
@rosskevin

rosskevin commented Mar 31, 2017

Summary

AfterStep do
   fail 'foo'
end

Results in all steps being indicated as passing.

1 scenario (1 failed)
7 steps (7 passed)

I am trying to indicate a step has failed if I find javascript errors in the after step. All steps are marked as passed, even though I should be able to fail the given step in the AfterStep hook.

Expected Behavior

When I run a scenario
If I fail in an AfterStep
Then the step should be marked as failed.

Current Behavior

When I run a scenario
If I fail in an AfterStep
Then the step is left as passed.

Steps to Reproduce (for bugs)

  1. Use any scenario
  2. Add
AfterStep do
   fail 'foo'
end

Context & Motivation

Visually, especially if it is the last step in the scenario, it is quite confusing to see all steps passed but the scenario fails.

Your Environment

  • Version used: 2.4.0
  • Operating System and version: OSX
@danascheider

This comment has been minimized.

Show comment
Hide comment
@danascheider

danascheider Apr 3, 2017

Contributor

@brasmusson or @mattwynne is this considered expected behavior?

Contributor

danascheider commented Apr 3, 2017

@brasmusson or @mattwynne is this considered expected behavior?

@mattwynne

This comment has been minimized.

Show comment
Hide comment
@mattwynne

mattwynne Apr 3, 2017

Member

I can understand why it happens, but it does seem surprising. The problem is that the way we currently model it, the after-step hook is just another test step in sequence after the gherkin step it was "hooked" onto. So that original gherkin step has already run and passed.

I've been thinking for a while that it would help us to be able to model "nested" steps or sub-steps, as this would also help us get rid of a lot of the edge-cases with around hooks. But this is just vague blue-sky thinking, I don't have anything concrete for how we would do it.

In this case, I wonder if it would be better for now if we were more transparent about the hook steps that are also running, and showing their pass / fail status (or at least their fail status).

Maybe we could include hooks in the summary counts?

e.g.

7 steps (7 passed)
4 hooks (3 passed, 1 failed)
Member

mattwynne commented Apr 3, 2017

I can understand why it happens, but it does seem surprising. The problem is that the way we currently model it, the after-step hook is just another test step in sequence after the gherkin step it was "hooked" onto. So that original gherkin step has already run and passed.

I've been thinking for a while that it would help us to be able to model "nested" steps or sub-steps, as this would also help us get rid of a lot of the edge-cases with around hooks. But this is just vague blue-sky thinking, I don't have anything concrete for how we would do it.

In this case, I wonder if it would be better for now if we were more transparent about the hook steps that are also running, and showing their pass / fail status (or at least their fail status).

Maybe we could include hooks in the summary counts?

e.g.

7 steps (7 passed)
4 hooks (3 passed, 1 failed)
@danascheider

This comment has been minimized.

Show comment
Hide comment
@danascheider

danascheider Apr 3, 2017

Contributor

I like that idea, @mattwynne. I definitely think we could be more transparent about the source of failures coming from hooks, because I've had trouble before figuring out exactly what went wrong in cases like this.

Contributor

danascheider commented Apr 3, 2017

I like that idea, @mattwynne. I definitely think we could be more transparent about the source of failures coming from hooks, because I've had trouble before figuring out exactly what went wrong in cases like this.

@brasmusson

This comment has been minimized.

Show comment
Hide comment
@brasmusson

brasmusson Apr 3, 2017

Contributor

I doubt that after-step hooks were introduced to do extra verification in addition to the verification in the step definitions, so from that perspective it is not expected behaviour that an failure in an after-step hook fails the step it is after-step hook for.

Comparing with scenarios and their before and after hooks, we consider both the results of each step definition and hook individually, and combine their results to the result for the scenario (the individual results are clearly documented in the json formatter output). To let an after-step hook fail the step would then mean that we both consider the result of the step definition and the after-step hook(s) individually, and also calculate a result for the step and after-step hook combo. This looks like a major change of the implemented execution model we have today, I don't think it should be considered before Cucumber-Ruby version 4.

Contributor

brasmusson commented Apr 3, 2017

I doubt that after-step hooks were introduced to do extra verification in addition to the verification in the step definitions, so from that perspective it is not expected behaviour that an failure in an after-step hook fails the step it is after-step hook for.

Comparing with scenarios and their before and after hooks, we consider both the results of each step definition and hook individually, and combine their results to the result for the scenario (the individual results are clearly documented in the json formatter output). To let an after-step hook fail the step would then mean that we both consider the result of the step definition and the after-step hook(s) individually, and also calculate a result for the step and after-step hook combo. This looks like a major change of the implemented execution model we have today, I don't think it should be considered before Cucumber-Ruby version 4.

@jaysonesmith

This comment has been minimized.

Show comment
Hide comment
@jaysonesmith

jaysonesmith Sep 6, 2017

Member

I'm on board with @brasmusson 's thoughts here about verification in the after step. I would consider the after to be used for things like cleanup if needed.

Member

jaysonesmith commented Sep 6, 2017

I'm on board with @brasmusson 's thoughts here about verification in the after step. I would consider the after to be used for things like cleanup if needed.

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Nov 8, 2017

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.

stale bot commented Nov 8, 2017

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.

@stale stale bot added the Stale label Nov 8, 2017

@rosskevin

This comment has been minimized.

Show comment
Hide comment
@rosskevin

rosskevin Nov 8, 2017

I think there is value in not closing this due to staleness, but if it is deemed not worth fixing, I guess I'll let it go.

rosskevin commented Nov 8, 2017

I think there is value in not closing this due to staleness, but if it is deemed not worth fixing, I guess I'll let it go.

@stale stale bot removed the Stale label Nov 8, 2017

@mattwynne

This comment has been minimized.

Show comment
Hide comment
@mattwynne

mattwynne Nov 8, 2017

Member

I think of it more as an enhancement than a fix @rosskevin, and quite a bit of work from the look of it. We could definitely keep it open if someone was going to try and tackle it. Are you interested @rosskevin?

Member

mattwynne commented Nov 8, 2017

I think of it more as an enhancement than a fix @rosskevin, and quite a bit of work from the look of it. We could definitely keep it open if someone was going to try and tackle it. Are you interested @rosskevin?

@rosskevin

This comment has been minimized.

Show comment
Hide comment
@rosskevin

rosskevin Nov 8, 2017

I'm not interested in the effort. If you view it as enhancement and not a bug, let's close it out. Please label as an enhancement in case this gets resurrected/found later.

rosskevin commented Nov 8, 2017

I'm not interested in the effort. If you view it as enhancement and not a bug, let's close it out. Please label as an enhancement in case this gets resurrected/found later.

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Jan 7, 2018

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.

stale bot commented Jan 7, 2018

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.

@stale stale bot added the Stale label Jan 7, 2018

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Jan 14, 2018

This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective.

stale bot commented Jan 14, 2018

This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective.

@stale stale bot closed this Jan 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment