Let's stop adding new Spinach features

I've been thinking that we should put a freeze on new Spinach feature tests and instead focus on RSpec exclusively, for a couple reasons.

"Natural Language" isn't a benefit

One of the main points of Gherkin/Cucumber/Spinach is that you can write these feature definitions in plain English that non-technical project managers could approve/refine, and then technical people could make them work by implementing the steps.

I don't think we benefit from this since our feature tests are read almost entirely by other developers and we don't really have project managers or clients that need these plain-English definitions.

Then consider that the majority of our team members aren't native English speakers, and this quickly stops being a selling point.

It's a pain to develop and debug

I hate having the feature definition as one file and the steps defined in another file (often multiple other files, since there's a lot of shared steps.)

If there's a broken feature, the test output only shows the step that failed. So to figure out what's going on and which test I may want to add a @focus tag to, I have to go to the step that failed, do a grep for the full step string, and hope it was only used in one place. If it's in multiple places I have to scroll through the way-too-verbose-but-not-at-all-useful test output and find the Scenario.

The steps are often not grouped with their previous or subsequent steps either because they're a shared definition, or the original author just didn't know or care where to place the new step definition in relation to the other steps.

Look at the output of a failed Spinach run:

Error summary:
  Errors (1)
    Project Forked Merge Requests :: I see the users in the target project for a new merge request :: And I fill out a "Merge Request On Forked Project" merge request

Ok... why did this fail? Where does this file even exist? Let's scroll up hundreds of lines:

  Scenario: I see the users in the target project for a new merge request
    ✔  Given I sign in as a user                                              # features/steps/shared/authentication.rb:7
    ✔  And I am a member of project "Shop"                                    # features/steps/project/forked_merge_requests.rb:8
    ✔  And I have a project forked off of "Shop" called "Forked Shop"         # features/steps/project/forked_merge_requests.rb:14
    ✔  Given I logout                                                         # features/steps/shared/authentication.rb:27
    ✔  And I sign in as an admin                                              # features/steps/shared/authentication.rb:11
    ✔  And I have a project forked off of "Shop" called "Forked Shop"         # features/steps/project/forked_merge_requests.rb:14

        Unable to find field "merge_request_title"
        /builds/gitlab-org/gitlab-ce/vendor/ruby/2.1.0/gems/capybara-2.4.4/lib/capybara/node/finders.rb:41:in `block in find'
    ✔  Then I visit project "Forked Shop" merge requests page                 # features/steps/shared/paths.rb:294
    ✔  And I click link "New Merge Request"                                   # features/steps/project/forked_merge_requests.rb:18
    !  And I fill out a "Merge Request On Forked Project" merge request       # features/steps/project/forked_merge_requests.rb:36
    ~  When I click "Assign to" dropdown"
    ~  Then I should see the target project ID in the input selector
    ~  And I should see the users from the target project ID

Ignoring for a second that the error message confusingly comes before two more successfully-executed steps, all we get is the location of the step definition. If I want to re-run just this failing test, I have to grep for "I fill out a "Merge Request On --" forget it, I'll just hit Retry.

Now compare that to RSpec:

  1) Projects::ImportExport::WikiRepoBundler bundle bundles the repo successfully
     Failure/Error: expect(wiki_bundler.bundle).to be true

       expected true
            got false
     # ./spec/services/projects/import_export/wiki_repo_bundler_spec.rb:22:in `block (3 levels) in <top (required)>'

You see exactly what failed, what it expected, what it got, and, critically, the exact file and line number where it occurred so you can re-run a focused execution.

It induces bad test design

Passing values from step to step via instance variables, looking up critical database records by their String-based names, vague rules about scope. Ugh.

Solutions?

At a minimum I think we should put a permanent moratorium on adding new Spinach tests. If adding a test requires adding more than one new step definition, we should move the entire scenario to an RSpec feature test.

Eventually I'd like to convert everything entirely, but that's a potentially large time investment that's hard to justify.

Related to the gripe session earlier today: https://gitlab.slack.com/archives/development/p1457456218000227

cc @DouweM @yorickpeterse @dzaporozhets