Merge branch '35804-table-based-testing' into 'master'

Use rspec-parameterized for table-based tests

See merge request !2655

Gemfile

@@ -336,6 +336,7 @@ group :development, :test do
   gem 'spinach-rerun-reporter', '~> 0.0.2'
   gem 'rspec_profiling', '~> 0.0.5'
   gem 'rspec-set', '~> 0.1.3'
+  gem 'rspec-parameterized'
 
   # Prevent occasions where minitest is not bundled in packaged versions of ruby (see #3826)
   gem 'minitest', '~> 5.7.0'

Gemfile.lock

@@ -2,6 +2,7 @@ GEM
   remote: https://rubygems.org/
   specs:
     RedCloth (4.3.2)
+    abstract_type (0.0.7)
     ace-rails-ap (4.1.2)
     actionmailer (4.2.8)
       actionpack (= 4.2.8)
@@ -41,6 +42,9 @@ GEM
       tzinfo (~> 1.1)
     acts-as-taggable-on (4.0.0)
       activerecord (>= 4.0)
+    adamantium (0.2.0)
+      ice_nine (~> 0.11.0)
+      memoizable (~> 0.4.0)
     addressable (2.3.8)
     after_commit_queue (1.3.0)
       activerecord (>= 3.0)
@@ -132,6 +136,9 @@ GEM
     coercible (1.0.0)
       descendants_tracker (~> 0.0.1)
     colorize (0.7.7)
+    concord (0.1.5)
+      adamantium (~> 0.2.0)
+      equalizer (~> 0.0.9)
     concurrent-ruby (1.0.5)
     concurrent-ruby-ext (1.0.5)
       concurrent-ruby (= 1.0.5)
@@ -489,6 +496,8 @@ GEM
       mime-types (>= 1.16, < 4)
     mail_room (0.9.1)
     memoist (0.15.0)
+    memoizable (0.4.2)
+      thread_safe (~> 0.3, >= 0.3.1)
     method_source (0.8.2)
     mime-types (2.99.3)
     mimemagic (0.3.0)
@@ -630,6 +639,11 @@ GEM
     premailer-rails (1.9.7)
       actionmailer (>= 3, < 6)
       premailer (~> 1.7, >= 1.7.9)
+    proc_to_ast (0.1.0)
+      coderay
+      parser
+      unparser
+    procto (0.0.3)
     prometheus-client-mmap (0.7.0.beta11)
       mmap2 (~> 2.2, >= 2.2.7)
     pry (0.10.4)
@@ -738,6 +752,10 @@ GEM
       chunky_png
     rqrcode-rails3 (0.1.7)
       rqrcode (>= 0.4.2)
+    rspec (3.6.0)
+      rspec-core (~> 3.6.0)
+      rspec-expectations (~> 3.6.0)
+      rspec-mocks (~> 3.6.0)
     rspec-core (3.6.0)
       rspec-support (~> 3.6.0)
     rspec-expectations (3.6.0)
@@ -746,6 +764,12 @@ GEM
     rspec-mocks (3.6.0)
       diff-lcs (>= 1.2.0, < 2.0)
       rspec-support (~> 3.6.0)
+    rspec-parameterized (0.4.0)
+      binding_of_caller
+      parser
+      proc_to_ast
+      rspec (>= 2.13, < 4)
+      unparser
     rspec-rails (3.6.0)
       actionpack (>= 3.0)
       activesupport (>= 3.0)
@@ -913,6 +937,14 @@ GEM
       get_process_mem (~> 0)
       unicorn (>= 4, < 6)
     uniform_notifier (1.10.0)
+    unparser (0.2.6)
+      abstract_type (~> 0.0.7)
+      adamantium (~> 0.2.0)
+      concord (~> 0.1.5)
+      diff-lcs (~> 1.3)
+      equalizer (~> 0.0.9)
+      parser (>= 2.3.1.2, < 2.5)
+      procto (~> 0.0.2)
     url_safe_base64 (0.2.2)
     validates_hostname (1.0.6)
       activerecord (>= 3.0)
@@ -1123,6 +1155,7 @@ DEPENDENCIES
   responders (~> 2.0)
   rouge (~> 2.0)
   rqrcode-rails3 (~> 0.1.7)
+  rspec-parameterized
   rspec-rails (~> 3.6.0)
   rspec-retry (~> 0.4.5)
   rspec-set (~> 0.1.3)

doc/development/testing.md

@@ -283,6 +283,43 @@ end
 - Avoid scenario titles that add no information, such as "successfully".
 - Avoid scenario titles that repeat the feature title.
 
+### Table-based / Parameterized tests
+
+This style of testing is used to exercise one piece of code with a comprehensive
+range of inputs. By specifying the test case once, alongside a table of inputs
+and the expected output for each, your tests can be made easier to read and more
+compact.
+
+We use the [rspec-parameterized](https://github.com/tomykaira/rspec-parameterized)
+gem. A short example, using the table syntax and checking Ruby equality for a
+range of inputs, might look like this:
+
+```ruby
+describe "#==" do
+  using Rspec::Parameterized::TableSyntax
+
+  let(:project1) { create(:project) }
+  let(:project2) { create(:project) }
+  where(:a, :b, :result) do
+    1         | 1        | true
+    1         | 2        | false
+    true      | true     | true
+    true      | false    | false
+    project1  | project1 | true
+    project2  | project2 | true
+    project 1 | project2 | false
+  end
+
+  with_them do
+    it { expect(a == b).to eq(result) }
+
+    it 'is isomorphic' do
+      expect(b == a).to eq(result)
+    end
+  end
+end
+```
+
 ### Matchers
 
 Custom matchers should be created to clarify the intent and/or hide the

spec/ee/spec/models/ee/issue_spec.rb

 require 'spec_helper'
 
 describe Issue do
+  using RSpec::Parameterized::TableSyntax
+
   describe '#allows_multiple_assignees?' do
     it 'does not allow multiple assignees without license' do
       stub_licensed_features(multiple_issue_assignees: false)
@@ -20,24 +22,23 @@ describe Issue do
   end
 
   describe '#weight' do
-    [
-      { license: true,  database: 5,    expected: 5 },
-      { license: true,  database: nil,  expected: nil },
-      { license: false, database: 5,    expected: nil },
-      { license: false, database: nil,  expected: nil }
-    ].each do |spec|
-      context spec.inspect do
-        let(:spec) { spec }
-        let(:issue) { build(:issue, weight: spec[:database]) }
+    where(:license_value, :database_value, :expected) do
+      true  | 5   | 5
+      true  | nil | nil
+      false | 5   | nil
+      false | nil | nil
+    end
+
+    with_them do
+      let(:issue) { build(:issue, weight: database_value) }
 
       subject { issue.weight }
 
       before do
-          stub_licensed_features(issue_weights: spec[:license])
+        stub_licensed_features(issue_weights: license_value)
       end
 
-        it { is_expected.to eq(spec[:expected]) }
-      end
+      it { is_expected.to eq(expected) }
     end
   end
 end

spec/ee/spec/models/ee/merge_request_spec.rb

 require 'spec_helper'
 
 describe MergeRequest do
+  using RSpec::Parameterized::TableSyntax
+
   let(:project) { create(:project, :repository) }
 
   subject(:merge_request) { create(:merge_request, source_project: project, target_project: project) }
@@ -135,24 +137,23 @@ describe MergeRequest do
   end
 
   describe '#approvals_before_merge' do
-    [
-      { license: true,  database: 5,   expected: 5 },
-      { license: true,  database: nil, expected: nil },
-      { license: false, database: 5,   expected: nil },
-      { license: false, database: nil, expected: nil }
-    ].each do |spec|
-      context spec.inspect do
-        let(:spec) { spec }
-        let(:merge_request) { build(:merge_request, approvals_before_merge: spec[:database]) }
+    where(:license_value, :db_value, :expected) do
+      true  | 5   | 5
+      true  | nil | nil
+      false | 5   | nil
+      false | nil | nil
+    end
+
+    with_them do
+      let(:merge_request) { build(:merge_request, approvals_before_merge: db_value) }
 
       subject { merge_request.approvals_before_merge }
 
       before do
-          stub_licensed_features(merge_request_approvers: spec[:license])
+        stub_licensed_features(merge_request_approvers: license_value)
       end
 
-        it { is_expected.to eq(spec[:expected]) }
-      end
+      it { is_expected.to eq(expected) }
     end
   end
 end

spec/ee/spec/models/ee/project_import_data_spec.rb

 require 'spec_helper'
 
 describe ProjectImportData do
+  using RSpec::Parameterized::TableSyntax
+
   let(:import_url) { 'ssh://example.com' }
   let(:import_data_attrs) { { auth_method: 'ssh_public_key' } }
   let(:project) { build(:project, :mirror, import_url: import_url, import_data_attributes: import_data_attrs) }
@@ -12,20 +14,19 @@ describe ProjectImportData do
   end
 
   describe '#ssh_key_auth?' do
-    subject { import_data.ssh_key_auth? }
+    where(:import_url, :auth_method, :expected) do
+      'ssh://example.com'  | 'ssh_public_key' | true
+      'ssh://example.com'  | 'password'       | false
+      'http://example.com' | 'ssh_public_key' | false
+      'http://example.com' | 'password'       | false
+    end
 
-    [
-      { import_url: 'ssh://example.com',  auth_method: 'ssh_public_key', expected: true  },
-      { import_url: 'ssh://example.com',  auth_method: 'password',       expected: false },
-      { import_url: 'http://example.com', auth_method: 'ssh_public_key', expected: false },
-      { import_url: 'http://example.com', auth_method: 'password',       expected: false }
-    ].each do |spec|
-      context spec.inspect do
-        let(:import_url) { spec[:import_url] }
-        let(:import_data_attrs) { { auth_method: spec[:auth_method] } }
+    with_them do
+      let(:import_data_attrs) { { auth_method: auth_method } }
 
-        it { is_expected.to spec[:expected] ? be_truthy : be_falsy }
-      end
+      subject { import_data.ssh_key_auth? }
+
+      it { is_expected.to eq(expected) }
     end
   end
 
@@ -62,20 +63,18 @@ describe ProjectImportData do
   end
 
   describe '#ssh_import?' do
-    subject { import_data.ssh_import? }
+    where(:import_url, :expected) do
+      'ssh://example.com'   | true
+      'git://example.com'   | false
+      'http://example.com'  | false
+      'https://example.com' | false
+      nil                   | nil
+    end
 
-    [
-      { import_url: nil,                   expected: false },
-      { import_url: 'ssh://example.com',   expected: true  },
-      { import_url: 'git://example.com',   expected: false },
-      { import_url: 'http://example.com',  expected: false },
-      { import_url: 'https://example.com', expected: false }
-    ].each do |spec|
-      context spec.inspect do
-        let(:import_url) { spec[:import_url] }
+    with_them do
+      subject { import_data.ssh_import? }
 
-        it { is_expected.to spec[:expected] ? be_truthy : be_falsy }
-      end
+      it { is_expected.to eq(expected) }
     end
   end
 

spec/ee/spec/models/ee/project_spec.rb

 require 'spec_helper'
 
 describe Project do
+  using RSpec::Parameterized::TableSyntax
+
   describe 'associations' do
     it { is_expected.to delegate_method(:shared_runners_minutes).to(:statistics) }
     it { is_expected.to delegate_method(:shared_runners_seconds).to(:statistics) }
@@ -639,102 +641,98 @@ describe Project do
   end
 
   describe '#approvals_before_merge' do
-    [
-      { license: true,  database: 5,  expected: 5 },
-      { license: true,  database: 0,  expected: 0 },
-      { license: false, database: 5,  expected: 0 },
-      { license: false, database: 0,  expected: 0 }
-    ].each do |spec|
-      context spec.inspect do
-        let(:spec) { spec }
-        let(:project) { build(:project, approvals_before_merge: spec[:database]) }
+    where(:license_value, :db_value, :expected) do
+      true  | 5 | 5
+      true  | 0 | 0
+      false | 5 | 0
+      false | 0 | 0
+    end
+
+    with_them do
+      let(:project) { build(:project, approvals_before_merge: db_value) }
 
       subject { project.approvals_before_merge }
 
       before do
-          stub_licensed_features(merge_request_approvers: spec[:license])
+        stub_licensed_features(merge_request_approvers: license_value)
       end
 
-        it { is_expected.to eq(spec[:expected]) }
-      end
+      it { is_expected.to eq(expected) }
     end
   end
 
   describe "#reset_approvals_on_push?" do
-    [
-      { license: true,  database: true,  expected: true },
-      { license: true,  database: false, expected: false },
-      { license: false, database: true,  expected: false },
-      { license: false, database: false, expected: false }
-    ].each do |spec|
-      context spec.inspect do
-        let(:spec) { spec }
-        let(:project) { build(:project, reset_approvals_on_push: spec[:database]) }
+    where(:license_value, :db_value, :expected) do
+      true  | true  | true
+      true  | false | false
+      false | true  | false
+      false | false | false
+    end
+
+    with_them do
+      let(:project) { build(:project, reset_approvals_on_push: db_value) }
 
       subject { project.reset_approvals_on_push? }
 
       before do
-          stub_licensed_features(merge_request_approvers: spec[:license])
+        stub_licensed_features(merge_request_approvers: license_value)
       end
 
-        it { is_expected.to eq(spec[:expected]) }
-      end
+      it { is_expected.to eq(expected) }
     end
   end
 
   describe '#approvals_before_merge' do
-    [
-      { license: true,  database: 5,  expected: 5 },
-      { license: true,  database: 0,  expected: 0 },
-      { license: false, database: 5,  expected: 0 },
-      { license: false, database: 0,  expected: 0 }
-    ].each do |spec|
-      context spec.inspect do
-        let(:spec) { spec }
-        let(:project) { build(:project, approvals_before_merge: spec[:database]) }
+    where(:license_value, :db_value, :expected) do
+      true  | 5 | 5
+      true  | 0 | 0
+      false | 5 | 0
+      false | 0 | 0
+    end
+
+    with_them do
+      let(:project) { build(:project, approvals_before_merge: db_value) }
 
       subject { project.approvals_before_merge }
 
       before do
-          stub_licensed_features(merge_request_approvers: spec[:license])
+        stub_licensed_features(merge_request_approvers: license_value)
       end
 
-        it { is_expected.to eq(spec[:expected]) }
-      end
+      it { is_expected.to eq(expected) }
     end
   end
 
   describe '#merge_method' do
-    [
-      { ff: true,  rebase: true,  ff_licensed: true,  rebase_licensed: true,  method: :ff },
-      { ff: true,  rebase: true,  ff_licensed: true,  rebase_licensed: false, method: :ff },
-      { ff: true,  rebase: true,  ff_licensed: false, rebase_licensed: true,  method: :rebase_merge },
-      { ff: true,  rebase: true,  ff_licensed: false, rebase_licensed: false, method: :merge },
-      { ff: true,  rebase: false, ff_licensed: true,  rebase_licensed: true,  method: :ff },
-      { ff: true,  rebase: false, ff_licensed: true,  rebase_licensed: false, method: :ff },
-      { ff: true,  rebase: false, ff_licensed: false, rebase_licensed: true,  method: :merge },
-      { ff: true,  rebase: false, ff_licensed: false, rebase_licensed: false, method: :merge },
-      { ff: false, rebase: true,  ff_licensed: true,  rebase_licensed: true,  method: :rebase_merge },
-      { ff: false, rebase: true,  ff_licensed: true,  rebase_licensed: false, method: :merge },
-      { ff: false, rebase: true,  ff_licensed: false, rebase_licensed: true,  method: :rebase_merge },
-      { ff: false, rebase: true,  ff_licensed: false, rebase_licensed: false, method: :merge },
-      { ff: false, rebase: false, ff_licensed: true,  rebase_licensed: true,  method: :merge },
-      { ff: false, rebase: false, ff_licensed: true,  rebase_licensed: false, method: :merge },
-      { ff: false, rebase: false, ff_licensed: false, rebase_licensed: true,  method: :merge },
-      { ff: false, rebase: false, ff_licensed: false, rebase_licensed: false, method: :merge }
-    ].each do |spec|
-      context spec.inspect do
-        let(:project) { build(:project, merge_requests_rebase_enabled: spec[:rebase], merge_requests_ff_only_enabled: spec[:ff]) }
-        let(:spec) { spec }
+    where(:ff, :rebase, :ff_licensed, :rebase_licensed, :method) do
+      true  | true  | true  | true  | :ff
+      true  | true  | true  | false | :ff
+      true  | true  | false | true  | :rebase_merge
+      true  | true  | false | false | :merge
+      true  | false | true  | true  | :ff
+      true  | false | true  | false | :ff
+      true  | false | false | true  | :merge
+      true  | false | false | false | :merge
+      false | true  | true  | true  | :rebase_merge
+      false | true  | true  | false | :merge
+      false | true  | false | true  | :rebase_merge
+      false | true  | false | false | :merge
+      false | false | true  | true  | :merge
+      false | false | true  | false | :merge
+      false | false | false | true  | :merge
+      false | false | false | false | :merge
+    end
+
+    with_them do
+      let(:project) { build(:project, merge_requests_rebase_enabled: rebase, merge_requests_ff_only_enabled: ff) }
 
       subject { project.merge_method }
 
       before do
-          stub_licensed_features(merge_request_rebase: spec[:rebase_licensed], fast_forward_merge: spec[:ff_licensed])
+        stub_licensed_features(merge_request_rebase: rebase_licensed, fast_forward_merge: ff_licensed)
       end
 
-        it { is_expected.to eq(spec[:method]) }
-      end
+      it { is_expected.to eq(method) }
     end
   end
 

spec/ee/spec/models/ssh_host_key_spec.rb

 require 'spec_helper'
 
 describe SshHostKey do
+  using RSpec::Parameterized::TableSyntax
   include ReactiveCachingHelpers
 
-  keys = [
-    SSHKeygen.generate,
-    SSHKeygen.generate
-  ]
+  let(:key1) do
+    'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQC3UpyF2iLqy1d63M6k3jH1vuEnq/NWtE+o' \
+    'rJe1Xn7JoRbduKd6zpsJ0JhBGWgcQK0ph0aGW5PcudzzBSc+SlYfCc4GTaxDtmj41hW0o72m' \
+    'NiuDW3oKXXShOiVRde2ZOquH8Z865jGiZIC8BI/bXZD29IGUih0hPu7Rjp70VYiE+35QRf/p' \
+    'sD0Ddrz8QUIG3A/2dMzLI5F5ZORk3BIX2F3mJwJOvZxRhR/SqyphDMZ5eZ0EzqbFBCDE6HAB' \
+    'Woz9ck8RBGLvCIggmDHj3FmMLcQGMDiy6wKp7QdnBtxjCP6vtE6YPUM223AqsWt+9NTtCfB8' \
+    'YdNAH7YcHHOR1FgtSk1x'
+  end
+
+  let(:key2) do
+    'ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDLIp+4ciR2YO9f9rpldc7InNQw/TBUtcNb' \
+    'J2XR0rr15/5ytz7YM16xXG0Qjx576PNSmqs4gbTrvTuFZak+v1Jx/9deHRq/yqp9f+tv33+i' \
+    'aJGCQCX/+OVY7aWgV2R9YsS7XQ4mnv4XlOTEssib/rGAIT+ATd/GcdYSEOO+dh4O09/6O/jI' \
+    'MGSeP+NNetgn1nPCnLOjrXFZUnUtNDi6EEKeIlrliJjSb7Jr4f7gjvZnv4RskWHHFo8FgAAq' \
+    't0gOMT6EmKrnypBe2vLGSAXbtkXr01q6/DNPH+n9VA1LTV6v1KN/W5CN5tQV11wRSKiM8g5O' \
+    'Ebi86VjJRi2sOuYoXQU1'
+  end
 
   # Purposefully ordered so that `sort` will make changes
-  known_hosts = <<~EOF
-    example.com #{keys[0]} git@localhost
-    @revoked other.example.com #{keys[1]} git@localhost
+  let(:known_hosts) do
+    <<~EOF
+      example.com #{key1} git@localhost
+      @revoked other.example.com #{key2} git@localhost
     EOF
+  end
+
+  let(:extra) { known_hosts + "foo\nbar\n" }
+  let(:reversed) { known_hosts.lines.reverse.join }
 
   def stub_ssh_keyscan(args, status: true, stdout: "", stderr: "")
     stdin = StringIO.new
@@ -33,7 +52,7 @@ describe SshHostKey do
     it 'returns an array of indexed fingerprints when the cache is filled' do
       stub_reactive_cache(ssh_host_key, known_hosts: known_hosts)
 
-      expected = keys
+      expected = [key1, key2]
         .map { |data| Gitlab::KeyFingerprint.new(data) }
         .each_with_index
         .map { |key, i| { bits: key.bits, fingerprint: key.fingerprint, type: key.type, index: i } } 
@@ -48,16 +67,12 @@ describe SshHostKey do
 
   describe '#fingerprints', use_clean_rails_memory_store_caching: true do
     it 'returns an array of indexed fingerprints when the cache is filled' do
-      key1 = SSHKeygen.generate
-      key2 = SSHKeygen.generate
-
-      known_hosts = "example.com #{key1} git@localhost\n\n\n@revoked other.example.com #{key2} git@localhost\n"
       stub_reactive_cache(ssh_host_key, known_hosts: known_hosts)
 
       expect(ssh_host_key.fingerprints.as_json).to eq(
         [
           { bits: 2048, fingerprint: Gitlab::KeyFingerprint.new(key1).fingerprint, type: 'RSA', index: 0 },
-          { bits: 2048, fingerprint: Gitlab::KeyFingerprint.new(key2).fingerprint, type: 'RSA', index: 3 }
+          { bits: 2048, fingerprint: Gitlab::KeyFingerprint.new(key2).fingerprint, type: 'RSA', index: 1 }
         ]
       )
     end
@@ -68,36 +83,35 @@ describe SshHostKey do
   end
 
   describe '#changes_project_import_data?' do
+    where(:a, :b, :result) do
+      known_hosts | extra       | true
+      known_hosts | "foo\n"     | true
+      known_hosts | ''          | true
+      known_hosts | nil         | true
+      known_hosts | known_hosts | false
+      reversed    | known_hosts | false
+      extra       | "foo\n"     | true
+      ''          | ''          | false
+      nil         | nil         | false
+      ''          | nil         | false
+    end
+
+    with_them do
       subject { ssh_host_key.changes_project_import_data? }
 
-    reversed = known_hosts.lines.reverse.join
-    extra = known_hosts + "foo\nbar\n"
+      it "(normal)" do
+        expect(ssh_host_key).to receive(:known_hosts).and_return(a)
+        project.import_data.ssh_known_hosts = b
 
-    [
-      { a: known_hosts, b: extra,       result: true  },
-      { a: known_hosts, b: "foo\n",     result: true  },
-      { a: known_hosts, b: '',          result: true  },
-      { a: known_hosts, b: nil,         result: true  },
-      { a: known_hosts, b: known_hosts, result: false },
-      { a: reversed,    b: known_hosts, result: false },
-      { a: extra,       b: "foo\n",     result: true  },
-      { a: '',          b: '',          result: false },
-      { a: nil,         b: nil,         result: false },
-      { a: '',          b: nil,         result: false }
-    ].each_with_index do |spec, index|
-      it "is #{spec[:result]} for test case #{index}" do
-        expect(ssh_host_key).to receive(:known_hosts).and_return(spec[:a])
-        project.import_data.ssh_known_hosts = spec[:b]
-
-        is_expected.to eq(spec[:result])
+        is_expected.to eq(result)
       end
 
       # Comparisons should be symmetrical, so test the reverse too
-      it "is #{spec[:result]} for test case #{index} (reversed)" do
-        expect(ssh_host_key).to receive(:known_hosts).and_return(spec[:b])
-        project.import_data.ssh_known_hosts = spec[:a]
+      it "(reversed)" do
+        expect(ssh_host_key).to receive(:known_hosts).and_return(b)
+        project.import_data.ssh_known_hosts = a
 
-        is_expected.to eq(spec[:result])
+        is_expected.to eq(result)
       end
     end
   end

spec/spec_helper.rb

@@ -8,6 +8,7 @@ require File.expand_path("../../config/environment", __FILE__)
 require 'rspec/rails'
 require 'shoulda/matchers'
 require 'rspec/retry'
+require 'rspec-parameterized'
 
 rspec_profiling_is_configured =
   ENV['RSPEC_PROFILING_POSTGRES_URL'].present? ||