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

Fail background skipped tests #198

Closed
rogertorres opened this issue Jan 18, 2022 · 4 comments · Fixed by #199
Closed

Fail background skipped tests #198

rogertorres opened this issue Jan 18, 2022 · 4 comments · Fixed by #199
Assignees
Labels
bug Something isn't working
Milestone

Comments

@rogertorres
Copy link
Contributor

Currently, running a test with fail_on_skipped() does not fail Background skipped tests. The reason is because the Background steps are not being mapped as the steps directly under the Scenario are:

Cucumber::Feature(
f,
Feature::Scenario(sc, Scenario::Step(st, Step::Skipped)),
) => map_failed(f, None, sc, st),

If that was the only thing that needed to be changed, it would be easy (spoiler: it is not); all it takes is to copy code above and replace Scenario::Step with Scenario::Background.

Cucumber::Feature(
    f,
    Feature::Scenario(sc, Scenario::Background(st, Step::Skipped)),
) => map_failed(f, None, sc, st),

It does work, but it also leads to another issue. Once we force skipped background steps to fail, we should also allow the usage of @allow.skipped on these steps, which is not possible because, as you clearly stated, we cannot use tags above Background.

I know you guys went deep redesigning this, so I want to understand your reasoning before proposing anything or making changes locally. My questions are:

  • To fail skipped Background tests is not available by design?
  • What is the reason for not allowing tags above Background?

Thank you!

@ilslv
Copy link
Member

ilslv commented Jan 19, 2022

@rogertorres thanks for detailed report and diving into code!

To fail skipped Background tests is not available by design?

No, this definitely was overlooked.

It does work, but it also leads to another issue. Once we force skipped background steps to fail, we should also allow the usage of @allow.skipped on these steps, which is not possible because, as you clearly stated, we cannot use tags above Background.

Actually I don't think this is an issue, because you can still use @allow.skipped on all Scenarios that are using skipped Backgrounds in case you don't want them to fail. This approach is even better than using @allow.skipped on Background, because it's more clear what Scenarios aren't tested. So solution you have provided should be enough.

I will happily review your PR in case it's possible or fix this myself.

@rogertorres
Copy link
Contributor Author

@ilslv This makes total sense!

I submitted a minimal PR, which I tested locally. I was wondering if it would not be a good idea to add some tests and/or a tip on the book. This is something I could do, but I would then need more time/help to really understand how you organized things there.

tyranron pushed a commit that referenced this issue Jan 19, 2022
…#199, #198)

Co-authored-by: ilslv <ilya.solovyiov@gmail.com>
@tyranron tyranron added this to the 0.11.2 milestone Jan 19, 2022
@tyranron
Copy link
Member

@rogertorres fix has been released in 0.11.2 version.

@rogertorres
Copy link
Contributor Author

@tyranron Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants