Merge remote-tracking branch 'origin/master' into ci-commit-as-pipelineci-commit-as-pipeline

# Conflicts:
#	db/schema.rb

8 files changed, 394 insertions, 64 deletions

diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index dcb8a3451bd..643acf0303c 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -286,6 +286,81 @@ module Ci
       end
 
     end
+    
+    describe "Scripts handling" do
+      let(:config_data) { YAML.dump(config) }
+      let(:config_processor) { GitlabCiYamlProcessor.new(config_data, path) }
+      
+      subject { config_processor.builds_for_stage_and_ref("test", "master").first }
+      
+      describe "before_script" do
+        context "in global context" do
+          let(:config) do
+            {
+              before_script: ["global script"],
+              test: { script: ["script"] }
+            }
+          end
+          
+          it "return commands with scripts concencaced" do
+            expect(subject[:commands]).to eq("global script\nscript")
+          end
+        end
+ 
+        context "overwritten in local context" do
+          let(:config) do
+            {
+              before_script: ["global script"],
+              test: { before_script: ["local script"], script: ["script"] }
+            }
+          end
+
+          it "return commands with scripts concencaced" do
+            expect(subject[:commands]).to eq("local script\nscript")
+          end
+        end
+      end
+
+      describe "script" do
+        let(:config) do
+          {
+            test: { script: ["script"] }
+          }
+        end
+
+        it "return commands with scripts concencaced" do
+          expect(subject[:commands]).to eq("script")
+        end
+      end
+
+      describe "after_script" do
+        context "in global context" do
+          let(:config) do
+            {
+              after_script: ["after_script"],
+              test: { script: ["script"] }
+            }
+          end
+
+          it "return after_script in options" do
+            expect(subject[:options][:after_script]).to eq(["after_script"])
+          end
+        end
+
+        context "overwritten in local context" do
+          let(:config) do
+            {
+              after_script: ["local after_script"],
+              test: { after_script: ["local after_script"], script: ["script"] }
+            }
+          end
+
+          it "return after_script in options" do
+            expect(subject[:options][:after_script]).to eq(["local after_script"])
+          end
+        end
+      end
+    end
 
     describe "Image and service handling" do
       it "returns image and service when defined" do
@@ -345,20 +420,76 @@ module Ci
       end
     end
 
-    describe "Variables" do
-      it "returns variables when defined" do
-        variables = {
-          var1: "value1",
-          var2: "value2",
-        }
-        config = YAML.dump({
-                             variables: variables,
-                             before_script: ["pwd"],
-                             rspec: { script: "rspec" }
-                           })
+    describe 'Variables' do
+      context 'when global variables are defined' do
+        it 'returns global variables' do
+          variables = {
+            VAR1: 'value1',
+            VAR2: 'value2',
+          }
 
-        config_processor = GitlabCiYamlProcessor.new(config, path)
-        expect(config_processor.variables).to eq(variables)
+          config = YAML.dump({
+            variables: variables,
+            before_script: ['pwd'],
+            rspec: { script: 'rspec' }
+          })
+
+          config_processor = GitlabCiYamlProcessor.new(config, path)
+
+          expect(config_processor.global_variables).to eq(variables)
+        end
+      end
+
+      context 'when job variables are defined' do
+        context 'when syntax is correct' do
+          it 'returns job variables' do
+            variables =  {
+              KEY1: 'value1',
+              SOME_KEY_2: 'value2'
+            }
+
+            config =  YAML.dump(
+              { before_script: ['pwd'],
+                rspec: {
+                  variables: variables,
+                  script: 'rspec' }
+              })
+
+            config_processor = GitlabCiYamlProcessor.new(config, path)
+
+            expect(config_processor.job_variables(:rspec)).to eq variables
+          end
+        end
+
+        context 'when syntax is incorrect' do
+          it 'raises error' do
+            variables = [:KEY1, 'value1', :KEY2, 'value2']
+
+            config =  YAML.dump(
+              { before_script: ['pwd'],
+                rspec: {
+                  variables: variables,
+                  script: 'rspec' }
+              })
+
+            expect { GitlabCiYamlProcessor.new(config, path) }
+              .to raise_error(GitlabCiYamlProcessor::ValidationError,
+                               /job: variables should be a map/)
+          end
+        end
+      end
+
+      context 'when job variables are not defined' do
+        it 'returns empty array' do
+          config = YAML.dump({
+            before_script: ['pwd'],
+            rspec: { script: 'rspec' }
+          })
+
+          config_processor = GitlabCiYamlProcessor.new(config, path)
+
+          expect(config_processor.job_variables(:rspec)).to eq []
+        end
       end
     end
 
@@ -536,7 +667,7 @@ module Ci
           stage_idx: 1,
           name: :normal_job,
           only: nil,
-          commands: "\ntest",
+          commands: "test",
           tag_list: [],
           options: {},
           when: "on_success",
@@ -563,7 +694,7 @@ EOT
           stage_idx: 1,
           name: :job1,
           only: nil,
-          commands: "\nexecute-script-for-job",
+          commands: "execute-script-for-job",
           tag_list: [],
           options: {},
           when: "on_success",
@@ -575,7 +706,7 @@ EOT
           stage_idx: 1,
           name: :job2,
           only: nil,
-          commands: "\nexecute-script-for-job",
+          commands: "execute-script-for-job",
           tag_list: [],
           options: {},
           when: "on_success",
@@ -607,6 +738,27 @@ EOT
         end.to raise_error(GitlabCiYamlProcessor::ValidationError, "before_script should be an array of strings")
       end
 
+      it "returns errors if job before_script parameter is not an array of strings" do
+        config = YAML.dump({ rspec: { script: "test", before_script: [10, "test"] } })
+        expect do
+          GitlabCiYamlProcessor.new(config, path)
+        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: before_script should be an array of strings")
+      end
+
+      it "returns errors if after_script parameter is invalid" do
+        config = YAML.dump({ after_script: "bundle update", rspec: { script: "test" } })
+        expect do
+          GitlabCiYamlProcessor.new(config, path)
+        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "after_script should be an array of strings")
+      end
+
+      it "returns errors if job after_script parameter is not an array of strings" do
+        config = YAML.dump({ rspec: { script: "test", after_script: [10, "test"] } })
+        expect do
+          GitlabCiYamlProcessor.new(config, path)
+        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "rspec job: after_script should be an array of strings")
+      end
+
       it "returns errors if image parameter is invalid" do
         config = YAML.dump({ image: ["test"], rspec: { script: "test" } })
         expect do
@@ -730,14 +882,14 @@ EOT
         config = YAML.dump({ variables: "test", rspec: { script: "test" } })
         expect do
           GitlabCiYamlProcessor.new(config, path)
-        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "variables should be a map of key-valued strings")
+        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "variables should be a map of key-value strings")
       end
 
-      it "returns errors if variables is not a map of key-valued strings" do
+      it "returns errors if variables is not a map of key-value strings" do
         config = YAML.dump({ variables: { test: false }, rspec: { script: "test" } })
         expect do
           GitlabCiYamlProcessor.new(config, path)
-        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "variables should be a map of key-valued strings")
+        end.to raise_error(GitlabCiYamlProcessor::ValidationError, "variables should be a map of key-value strings")
       end
 
       it "returns errors if job when is not on_success, on_failure or always" do
diff --git a/spec/lib/gitlab/github_import/issue_formatter_spec.rb b/spec/lib/gitlab/github_import/issue_formatter_spec.rb
index fd05428b322..0e7ffbe9b8e 100644
--- a/spec/lib/gitlab/github_import/issue_formatter_spec.rb
+++ b/spec/lib/gitlab/github_import/issue_formatter_spec.rb
@@ -2,13 +2,14 @@ require 'spec_helper'
 
 describe Gitlab::GithubImport::IssueFormatter, lib: true do
   let!(:project) { create(:project, namespace: create(:namespace, path: 'octocat')) }
-  let(:octocat) { OpenStruct.new(id: 123456, login: 'octocat') }
+  let(:octocat) { double(id: 123456, login: 'octocat') }
   let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') }
   let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') }
 
   let(:base_data) do
     {
       number: 1347,
+      milestone: nil,
       state: 'open',
       title: 'Found a bug',
       body: "I'm having a problem with this.",
@@ -26,11 +27,13 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
 
   describe '#attributes' do
     context 'when issue is open' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(state: 'open')) }
+      let(:raw_data) { double(base_data.merge(state: 'open')) }
 
       it 'returns formatted attributes' do
         expected = {
+          iid: 1347,
           project: project,
+          milestone: nil,
           title: 'Found a bug',
           description: "*Created by: octocat*\n\nI'm having a problem with this.",
           state: 'opened',
@@ -46,11 +49,13 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
 
     context 'when issue is closed' do
       let(:closed_at) { DateTime.strptime('2011-01-28T19:01:12Z') }
-      let(:raw_data) { OpenStruct.new(base_data.merge(state: 'closed', closed_at: closed_at)) }
+      let(:raw_data) { double(base_data.merge(state: 'closed', closed_at: closed_at)) }
 
       it 'returns formatted attributes' do
         expected = {
+          iid: 1347,
           project: project,
+          milestone: nil,
           title: 'Found a bug',
           description: "*Created by: octocat*\n\nI'm having a problem with this.",
           state: 'closed',
@@ -65,7 +70,7 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
     end
 
     context 'when it is assigned to someone' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(assignee: octocat)) }
+      let(:raw_data) { double(base_data.merge(assignee: octocat)) }
 
       it 'returns nil as assignee_id when is not a GitLab user' do
         expect(issue.attributes.fetch(:assignee_id)).to be_nil
@@ -78,8 +83,23 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
       end
     end
 
+    context 'when it has a milestone' do
+      let(:milestone) { double(number: 45) }
+      let(:raw_data) { double(base_data.merge(milestone: milestone)) }
+
+      it 'returns nil when milestone does not exist' do
+        expect(issue.attributes.fetch(:milestone)).to be_nil
+      end
+
+      it 'returns milestone when it exists' do
+        milestone = create(:milestone, project: project, iid: 45)
+
+        expect(issue.attributes.fetch(:milestone)).to eq milestone
+      end
+    end
+
     context 'when author is a GitLab user' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(user: octocat)) }
+      let(:raw_data) { double(base_data.merge(user: octocat)) }
 
       it 'returns project#creator_id as author_id when is not a GitLab user' do
         expect(issue.attributes.fetch(:author_id)).to eq project.creator_id
@@ -95,7 +115,7 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
 
   describe '#has_comments?' do
     context 'when number of comments is greater than zero' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(comments: 1)) }
+      let(:raw_data) { double(base_data.merge(comments: 1)) }
 
       it 'returns true' do
         expect(issue.has_comments?).to eq true
@@ -103,7 +123,7 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
     end
 
     context 'when number of comments is equal to zero' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(comments: 0)) }
+      let(:raw_data) { double(base_data.merge(comments: 0)) }
 
       it 'returns false' do
         expect(issue.has_comments?).to eq false
@@ -112,7 +132,7 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
   end
 
   describe '#number' do
-    let(:raw_data) { OpenStruct.new(base_data.merge(number: 1347)) }
+    let(:raw_data) { double(base_data.merge(number: 1347)) }
 
     it 'returns pull request number' do
       expect(issue.number).to eq 1347
@@ -121,7 +141,7 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
 
   describe '#valid?' do
     context 'when mention a pull request' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(pull_request: OpenStruct.new)) }
+      let(:raw_data) { double(base_data.merge(pull_request: double)) }
 
       it 'returns false' do
         expect(issue.valid?).to eq false
@@ -129,7 +149,7 @@ describe Gitlab::GithubImport::IssueFormatter, lib: true do
     end
 
     context 'when does not mention a pull request' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(pull_request: nil)) }
+      let(:raw_data) { double(base_data.merge(pull_request: nil)) }
 
       it 'returns true' do
         expect(issue.valid?).to eq true
diff --git a/spec/lib/gitlab/github_import/label_formatter_spec.rb b/spec/lib/gitlab/github_import/label_formatter_spec.rb
new file mode 100644
index 00000000000..e94440a7fb0
--- /dev/null
+++ b/spec/lib/gitlab/github_import/label_formatter_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe Gitlab::GithubImport::LabelFormatter, lib: true do
+
+  describe '#attributes' do
+    it 'returns formatted attributes' do
+      project = create(:project)
+      raw = double(name: 'improvements', color: 'e6e6e6')
+
+      formatter = described_class.new(project, raw)
+
+      expect(formatter.attributes).to eq({
+        project: project,
+        title: 'improvements',
+        color: '#e6e6e6'
+      })
+    end
+  end
+end
diff --git a/spec/lib/gitlab/github_import/milestone_formatter_spec.rb b/spec/lib/gitlab/github_import/milestone_formatter_spec.rb
new file mode 100644
index 00000000000..5a421e50581
--- /dev/null
+++ b/spec/lib/gitlab/github_import/milestone_formatter_spec.rb
@@ -0,0 +1,82 @@
+require 'spec_helper'
+
+describe Gitlab::GithubImport::MilestoneFormatter, lib: true do
+  let(:project) { create(:empty_project) }
+  let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') }
+  let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') }
+  let(:base_data) do
+    {
+      number: 1347,
+      state: 'open',
+      title: '1.0',
+      description: 'Version 1.0',
+      due_on: nil,
+      created_at: created_at,
+      updated_at: updated_at,
+      closed_at: nil
+    }
+  end
+
+  subject(:formatter) { described_class.new(project, raw_data)}
+
+  describe '#attributes' do
+    context 'when milestone is open' do
+      let(:raw_data) { double(base_data.merge(state: 'open')) }
+
+      it 'returns formatted attributes' do
+        expected = {
+          iid: 1347,
+          project: project,
+          title: '1.0',
+          description: 'Version 1.0',
+          state: 'active',
+          due_date: nil,
+          created_at: created_at,
+          updated_at: updated_at
+        }
+
+        expect(formatter.attributes).to eq(expected)
+      end
+    end
+
+    context 'when milestone is closed' do
+      let(:closed_at) { DateTime.strptime('2011-01-28T19:01:12Z') }
+      let(:raw_data) { double(base_data.merge(state: 'closed', closed_at: closed_at)) }
+
+      it 'returns formatted attributes' do
+        expected = {
+          iid: 1347,
+          project: project,
+          title: '1.0',
+          description: 'Version 1.0',
+          state: 'closed',
+          due_date: nil,
+          created_at: created_at,
+          updated_at: closed_at
+        }
+
+        expect(formatter.attributes).to eq(expected)
+      end
+    end
+
+    context 'when milestone has a due date' do
+      let(:due_date) { DateTime.strptime('2011-01-28T19:01:12Z') }
+      let(:raw_data) { double(base_data.merge(due_on: due_date)) }
+
+      it 'returns formatted attributes' do
+        expected = {
+          iid: 1347,
+          project: project,
+          title: '1.0',
+          description: 'Version 1.0',
+          state: 'active',
+          due_date: due_date,
+          created_at: created_at,
+          updated_at: updated_at
+        }
+
+        expect(formatter.attributes).to eq(expected)
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb
index e49dcb42342..e59c0ca110e 100644
--- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb
+++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb
@@ -2,17 +2,18 @@ require 'spec_helper'
 
 describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
   let(:project) { create(:project) }
-  let(:repository) { OpenStruct.new(id: 1, fork: false) }
+  let(:repository) { double(id: 1, fork: false) }
   let(:source_repo) { repository }
-  let(:source_branch) { OpenStruct.new(ref: 'feature', repo: source_repo) }
+  let(:source_branch) { double(ref: 'feature', repo: source_repo) }
   let(:target_repo) { repository }
-  let(:target_branch) { OpenStruct.new(ref: 'master', repo: target_repo) }
-  let(:octocat) { OpenStruct.new(id: 123456, login: 'octocat') }
+  let(:target_branch) { double(ref: 'master', repo: target_repo) }
+  let(:octocat) { double(id: 123456, login: 'octocat') }
   let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') }
   let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') }
   let(:base_data) do
     {
       number: 1347,
+      milestone: nil,
       state: 'open',
       title: 'New feature',
       body: 'Please pull these awesome changes',
@@ -31,10 +32,11 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
 
   describe '#attributes' do
     context 'when pull request is open' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(state: 'open')) }
+      let(:raw_data) { double(base_data.merge(state: 'open')) }
 
       it 'returns formatted attributes' do
         expected = {
+          iid: 1347,
           title: 'New feature',
           description: "*Created by: octocat*\n\nPlease pull these awesome changes",
           source_project: project,
@@ -42,6 +44,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
           target_project: project,
           target_branch: 'master',
           state: 'opened',
+          milestone: nil,
           author_id: project.creator_id,
           assignee_id: nil,
           created_at: created_at,
@@ -54,10 +57,11 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
 
     context 'when pull request is closed' do
       let(:closed_at) { DateTime.strptime('2011-01-28T19:01:12Z') }
-      let(:raw_data) { OpenStruct.new(base_data.merge(state: 'closed', closed_at: closed_at)) }
+      let(:raw_data) { double(base_data.merge(state: 'closed', closed_at: closed_at)) }
 
       it 'returns formatted attributes' do
         expected = {
+          iid: 1347,
           title: 'New feature',
           description: "*Created by: octocat*\n\nPlease pull these awesome changes",
           source_project: project,
@@ -65,6 +69,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
           target_project: project,
           target_branch: 'master',
           state: 'closed',
+          milestone: nil,
           author_id: project.creator_id,
           assignee_id: nil,
           created_at: created_at,
@@ -77,10 +82,11 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
 
     context 'when pull request is merged' do
       let(:merged_at) { DateTime.strptime('2011-01-28T13:01:12Z') }
-      let(:raw_data) { OpenStruct.new(base_data.merge(state: 'closed', merged_at: merged_at)) }
+      let(:raw_data) { double(base_data.merge(state: 'closed', merged_at: merged_at)) }
 
       it 'returns formatted attributes' do
         expected = {
+          iid: 1347,
           title: 'New feature',
           description: "*Created by: octocat*\n\nPlease pull these awesome changes",
           source_project: project,
@@ -88,6 +94,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
           target_project: project,
           target_branch: 'master',
           state: 'merged',
+          milestone: nil,
           author_id: project.creator_id,
           assignee_id: nil,
           created_at: created_at,
@@ -99,7 +106,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
     end
 
     context 'when it is assigned to someone' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(assignee: octocat)) }
+      let(:raw_data) { double(base_data.merge(assignee: octocat)) }
 
       it 'returns nil as assignee_id when is not a GitLab user' do
         expect(pull_request.attributes.fetch(:assignee_id)).to be_nil
@@ -113,7 +120,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
     end
 
     context 'when author is a GitLab user' do
-      let(:raw_data) { OpenStruct.new(base_data.merge(user: octocat)) }
+      let(:raw_data) { double(base_data.merge(user: octocat)) }
 
       it 'returns project#creator_id as author_id when is not a GitLab user' do
         expect(pull_request.attributes.fetch(:author_id)).to eq project.creator_id
@@ -125,10 +132,25 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
         expect(pull_request.attributes.fetch(:author_id)).to eq gl_user.id
       end
     end
+
+    context 'when it has a milestone' do
+      let(:milestone) { double(number: 45) }
+      let(:raw_data) { double(base_data.merge(milestone: milestone)) }
+
+      it 'returns nil when milestone does not exists' do
+        expect(pull_request.attributes.fetch(:milestone)).to be_nil
+      end
+
+      it 'returns milestone when is exists' do
+        milestone = create(:milestone, project: project, iid: 45)
+
+        expect(pull_request.attributes.fetch(:milestone)).to eq milestone
+      end
+    end
   end
 
   describe '#number' do
-    let(:raw_data) { OpenStruct.new(base_data.merge(number: 1347)) }
+    let(:raw_data) { double(base_data.merge(number: 1347)) }
 
     it 'returns pull request number' do
       expect(pull_request.number).to eq 1347
@@ -136,11 +158,11 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
   end
 
   describe '#valid?' do
-    let(:invalid_branch) { OpenStruct.new(ref: 'invalid-branch') }
+    let(:invalid_branch) { double(ref: 'invalid-branch').as_null_object }
 
     context 'when source, and target repositories are the same' do
       context 'and source and target branches exists' do
-        let(:raw_data) { OpenStruct.new(base_data.merge(head: source_branch, base: target_branch)) }
+        let(:raw_data) { double(base_data.merge(head: source_branch, base: target_branch)) }
 
         it 'returns true' do
           expect(pull_request.valid?).to eq true
@@ -148,7 +170,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
       end
 
       context 'and source branch doesn not exists' do
-        let(:raw_data) { OpenStruct.new(base_data.merge(head: invalid_branch, base: target_branch)) }
+        let(:raw_data) { double(base_data.merge(head: invalid_branch, base: target_branch)) }
 
         it 'returns false' do
           expect(pull_request.valid?).to eq false
@@ -156,7 +178,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
       end
 
       context 'and target branch doesn not exists' do
-        let(:raw_data) { OpenStruct.new(base_data.merge(head: source_branch, base: invalid_branch)) }
+        let(:raw_data) { double(base_data.merge(head: source_branch, base: invalid_branch)) }
 
         it 'returns false' do
           expect(pull_request.valid?).to eq false
@@ -165,8 +187,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
     end
 
     context 'when source repo is a fork' do
-      let(:source_repo) { OpenStruct.new(id: 2, fork: true) }
-      let(:raw_data) { OpenStruct.new(base_data) }
+      let(:source_repo) { double(id: 2, fork: true) }
+      let(:raw_data) { double(base_data) }
 
       it 'returns false' do
         expect(pull_request.valid?).to eq false
@@ -174,8 +196,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
     end
 
     context 'when target repo is a fork' do
-      let(:target_repo) { OpenStruct.new(id: 2, fork: true) }
-      let(:raw_data) { OpenStruct.new(base_data) }
+      let(:target_repo) { double(id: 2, fork: true) }
+      let(:raw_data) { double(base_data) }
 
       it 'returns false' do
         expect(pull_request.valid?).to eq false
diff --git a/spec/lib/gitlab/metrics/instrumentation_spec.rb b/spec/lib/gitlab/metrics/instrumentation_spec.rb
index ad4290c43bb..5c885a7a982 100644
--- a/spec/lib/gitlab/metrics/instrumentation_spec.rb
+++ b/spec/lib/gitlab/metrics/instrumentation_spec.rb
@@ -33,8 +33,16 @@ describe Gitlab::Metrics::Instrumentation do
         described_class.instrument_method(@dummy, :foo)
       end
 
-      it 'renames the original method' do
-        expect(@dummy).to respond_to(:_original_foo)
+      it 'instruments the Class' do
+        target = @dummy.singleton_class
+
+        expect(described_class.instrumented?(target)).to eq(true)
+      end
+
+      it 'defines a proxy method' do
+        mod = described_class.proxy_module(@dummy.singleton_class)
+
+        expect(mod.method_defined?(:foo)).to eq(true)
       end
 
       it 'calls the instrumented method with the correct arguments' do
@@ -76,6 +84,14 @@ describe Gitlab::Metrics::Instrumentation do
 
         expect(dummy.method(:test).arity).to eq(0)
       end
+
+      describe 'when a module is instrumented multiple times' do
+        it 'calls the instrumented method with the correct arguments' do
+          described_class.instrument_method(@dummy, :foo)
+
+          expect(@dummy.foo).to eq('foo')
+        end
+      end
     end
 
     describe 'with metrics disabled' do
@@ -86,7 +102,9 @@ describe Gitlab::Metrics::Instrumentation do
       it 'does not instrument the method' do
         described_class.instrument_method(@dummy, :foo)
 
-        expect(@dummy).to_not respond_to(:_original_foo)
+        target = @dummy.singleton_class
+
+        expect(described_class.instrumented?(target)).to eq(false)
       end
     end
   end
@@ -100,8 +118,14 @@ describe Gitlab::Metrics::Instrumentation do
           instrument_instance_method(@dummy, :bar)
       end
 
-      it 'renames the original method' do
-        expect(@dummy.method_defined?(:_original_bar)).to eq(true)
+      it 'instruments instances of the Class' do
+        expect(described_class.instrumented?(@dummy)).to eq(true)
+      end
+
+      it 'defines a proxy method' do
+        mod = described_class.proxy_module(@dummy)
+
+        expect(mod.method_defined?(:bar)).to eq(true)
       end
 
       it 'calls the instrumented method with the correct arguments' do
@@ -144,7 +168,7 @@ describe Gitlab::Metrics::Instrumentation do
         described_class.
           instrument_instance_method(@dummy, :bar)
 
-        expect(@dummy.method_defined?(:_original_bar)).to eq(false)
+        expect(described_class.instrumented?(@dummy)).to eq(false)
       end
     end
   end
@@ -167,18 +191,17 @@ describe Gitlab::Metrics::Instrumentation do
     it 'recursively instruments a class hierarchy' do
       described_class.instrument_class_hierarchy(@dummy)
 
-      expect(@child1).to respond_to(:_original_child1_foo)
-      expect(@child2).to respond_to(:_original_child2_foo)
+      expect(described_class.instrumented?(@child1.singleton_class)).to eq(true)
+      expect(described_class.instrumented?(@child2.singleton_class)).to eq(true)
 
-      expect(@child1.method_defined?(:_original_child1_bar)).to eq(true)
-      expect(@child2.method_defined?(:_original_child2_bar)).to eq(true)
+      expect(described_class.instrumented?(@child1)).to eq(true)
+      expect(described_class.instrumented?(@child2)).to eq(true)
     end
 
     it 'does not instrument the root module' do
       described_class.instrument_class_hierarchy(@dummy)
 
-      expect(@dummy).to_not respond_to(:_original_foo)
-      expect(@dummy.method_defined?(:_original_bar)).to eq(false)
+      expect(described_class.instrumented?(@dummy)).to eq(false)
     end
   end
 
@@ -190,7 +213,7 @@ describe Gitlab::Metrics::Instrumentation do
     it 'instruments all public class methods' do
       described_class.instrument_methods(@dummy)
 
-      expect(@dummy).to respond_to(:_original_foo)
+      expect(described_class.instrumented?(@dummy.singleton_class)).to eq(true)
     end
 
     it 'only instruments methods directly defined in the module' do
@@ -223,7 +246,7 @@ describe Gitlab::Metrics::Instrumentation do
     it 'instruments all public instance methods' do
       described_class.instrument_instance_methods(@dummy)
 
-      expect(@dummy.method_defined?(:_original_bar)).to eq(true)
+      expect(described_class.instrumented?(@dummy)).to eq(true)
     end
 
     it 'only instruments methods directly defined in the module' do
diff --git a/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb b/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb
index 7bc070a4d09..e3293a01207 100644
--- a/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb
+++ b/spec/lib/gitlab/metrics/subscribers/active_record_spec.rb
@@ -28,6 +28,9 @@ describe Gitlab::Metrics::Subscribers::ActiveRecord do
         expect(transaction).to receive(:increment).
           with(:sql_duration, 0.2)
 
+        expect(transaction).to receive(:increment).
+          with(:sql_count, 1)
+
         subscriber.sql(event)
       end
     end
diff --git a/spec/lib/gitlab/push_data_builder_spec.rb b/spec/lib/gitlab/push_data_builder_spec.rb
index 961022b9d12..7fc34139eff 100644
--- a/spec/lib/gitlab/push_data_builder_spec.rb
+++ b/spec/lib/gitlab/push_data_builder_spec.rb
@@ -14,11 +14,11 @@ describe Gitlab::PushDataBuilder, lib: true do
     it { expect(data[:ref]).to eq('refs/heads/master') }
     it { expect(data[:commits].size).to eq(3) }
     it { expect(data[:total_commits_count]).to eq(3) }
-    it { expect(data[:commits].first[:added]).to eq(["gitlab-grack"]) }
-    it { expect(data[:commits].first[:modified]).to eq([".gitmodules"]) }
+    it { expect(data[:commits].first[:added]).to eq(['gitlab-grack']) }
+    it { expect(data[:commits].first[:modified]).to eq(['.gitmodules']) }
     it { expect(data[:commits].first[:removed]).to eq([]) }
 
-    include_examples 'project hook data'
+    include_examples 'project hook data with deprecateds'
     include_examples 'deprecated repository hook data'
   end
 
@@ -34,9 +34,18 @@ describe Gitlab::PushDataBuilder, lib: true do
     it { expect(data[:checkout_sha]).to eq('5937ac0a7beb003549fc5fd26fc247adbce4a52e') }
     it { expect(data[:after]).to eq('8a2a6eb295bb170b34c24c76c49ed0e9b2eaf34b') }
     it { expect(data[:ref]).to eq('refs/tags/v1.1.0') }
+    it { expect(data[:user_id]).to eq(user.id) }
+    it { expect(data[:user_name]).to eq(user.name) }
+    it { expect(data[:user_email]).to eq(user.email) }
+    it { expect(data[:user_avatar]).to eq(user.avatar_url) }
+    it { expect(data[:project_id]).to eq(project.id) }
+    it { expect(data[:project]).to be_a(Hash) }
     it { expect(data[:commits]).to be_empty }
     it { expect(data[:total_commits_count]).to be_zero }
 
+    include_examples 'project hook data with deprecateds'
+    include_examples 'deprecated repository hook data'
+
     it 'does not raise an error when given nil commits' do
       expect { described_class.build(spy, spy, spy, spy, spy, nil) }.
         not_to raise_error