cabbage-ex / cabbage

Story BDD tool for executing elixir in ExUnit
MIT License
138 stars 33 forks source link

Implemented Backgrounds for Features/Scenarios #62

Closed mononym closed 5 years ago

mononym commented 5 years ago

The addition of 'IO.ANSI.white()' to the end of the log lines keeps the configured color from bleeding over into the beginning of the next log line.

Version was bumped because there was new functionality added which can change the behavior of tests.

Test 0 makes no sense, so tests now begin at 1.

It might be worth calling out in the docs somewhere that any tags that are run before the tests are run before the background steps are run. Background steps have access to any context set by an explicit 'setup_all' callback. Note that due to the preservation of order for 'setup' callbacks by ExUnit, any 'setup' callback defined explicitly in the test module will be run after the setup block automatically generated by cabbage which contains the tag and background steps.

revati commented 5 years ago

Note that due to the preservation of order for 'setup' callbacks by ExUnit, any 'setup' callback defined explicitly in the test module will be run after the setup block automatically generated by cabbage which contains the tag and background steps

I'm not sure if that () would be a feasible solution. Maybe I'm not representing the majority in my opinion, but I thought that background steps also should happen after all test setups are done. How I was implementing them (code isn't still done/merged) was to simply append background steps to all scenarios in the feature. So they would become regular steps, and always would be executed after all setups have finished.

Questions regarded this behavior is more like, what would be developers implicit expectation in what sequence initializations are executed

setup_all -> setup -> tags -> bacgroud -> steps setup_all -> bacgrkoud -> setup -> tags -> step

On another hand, what would be cases to use setup over setup_all at all?

mononym commented 5 years ago

I don't think the setup callback should be used explicitly with Background implemented, honestly. That is the whole point of Background steps for this in my mind. Using both setup and Background steps feels like you are beginning to mix-and-match behaviors as well as obfuscating critical information for a Feature/Rule.

I think it defeats the purpose/ability to look at a Feature file and know what's going on at a glance if you then have to go check the test implementation to make sure there isn't any hidden state being established for the test that you aren't aware of because it wasn't called out in the feature file.

Using setup_all is a bit more up in the air in my mind, but given that what's happening is code replacement based on a description in a feature file I kinda feel like if you're going to use Cabbage it should be more of an all-or-nothing sort of deal. Especially since steps can/should be shared between modules. Any setup that would be done in a setup_all callback, I feel, should probably be implemented as one or more steps which are explicitly referenced from said feature file. This is especially true once I've made some tweaks to the way I'm implementing Rules, so I'll bring it up here as it informs the conversation.

My plan for implementing the Rule keyword (Gherkin v6) is to effectively have a Feature level Background block act as a setup_all callback. I initially implemented it differently but I'm rethinking it. To illustrate let's take the following feature file (Please note the Gherkin docs aren't explicit about having a Background section outside of a Rule section when Rules are implemented):

Feature: Rule feature works
  Tests that multiple Rules work with backgrounds of all kinds.

  Background:
    Given I provide Background

  Rule: First rule has no additional background

    Scenario: Background provides default state
      Then I provided Background

  Rule: Second rule provides additional background

    Background:
      Given I provide additional Background

    Scenario: Background provides default state again
      Then I provided Background
      And I provided additional Background

My initial implementation was to treat the Feature level Background and the Rule level background as two lists of steps which I simply concat together and then run in the setup block after tags. After doing that I'm not entirely happy with it, because it doesn't allow for setting up of "global" test state without hiding said logic in the test file and away from the Feature file. Which is, again, I think something that should be avoided and defeats the whole purpose of doing this in the first place.

My new plan is to tweak my implementation so that the Feature level Background steps are inserted into a setup_all callback. This would allow for all setup to be explicitly called out in the feature files while allowing a developer to maintain the very useful split between setup and setup_all callbacks. The only thing, I think, that I feel like might get a bit hairy with that is needing some more advanced usage of the on_exit callback in the setup_all block. Then again, you could always make that a part of one of the steps that is called from the Feature level Background block...so I'n not sure how much of an issue that really is.

mononym commented 5 years ago

And to answer your question about order, which I forgot about, per the Gherkin docs the @Before/@After tags should come before/after any of the steps for the scenarios.

The way I read that, the tags are basically the ExUnit equivalent of the setup and on_exit callbacks, which should come before the Background steps. So it would be: setup_all -> tags -> background -> steps -> tags (via on_exit callback defined in tag)

With a feature file containing Rules it would look like: Feature background steps (as setup_all which applies to all Rules/Scenarios) -> tags (per scenario as setup) -> Rule background steps (per scenario as setup) -> steps -> tags (via on_exit callback defined in tag)

revati commented 5 years ago

I accidentally merged this pull request, though it depends on pull requests on https://github.com/cabbage-ex/gherkin/pull/8 Which fail tests because of

** (Mix) You're trying to run :gherkin on Elixir v1.2.6 but it has declared in its mix.exs file it supports only Elixir ~> 1.3

Is this elixir version bump intentional?

mononym commented 5 years ago

I didn't bump the Elixir version in that PR. None of my commits change the Elixir version.

That change was done over a year ago.

mononym commented 5 years ago

To your point about the setup callback though. While I do still think setup shouldn't be explicitly used when using Background steps, I can see how there might be some additional confusion because of setup ordering.

I am happy to make a quick change to prepend the background steps to the scenario steps.

revati commented 5 years ago

Previously I accidentally merged this PR, had to make revert.

Now tho, dependency on gherkin have been merged. But trying to revert previously made revert throws conflicts, can you please look into those and create new PR? Thanks and sorry.

mononym commented 5 years ago

Sure! Sorry I didn't respond previously. Have been busy with work and family. I'll try to put something together today/tonight.

I'll also make sure to have the background steps prepended to the scenario steps so that any explicit 'setup' callbacks are run first.