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