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
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 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.
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