Fix next round of tests

app/controllers/ci/commits_controller.rb

@@ -13,7 +13,7 @@ module Ci
     end
 
     def status
-      commit = Ci::Project.find(params[:project_id]).commits.find_by_sha!(params[:id], params[:ref_id])
+      commit = Ci::Project.find(params[:project_id]).commits.find_by_sha!(params[:id])
       render json: commit.to_json(only: [:id, :sha], methods: [:status, :coverage])
     rescue ActiveRecord::RecordNotFound
       render json: { status: "not_found" }

app/helpers/builds_helper.rb

@@ -3,10 +3,6 @@ module BuildsHelper
     gitlab_ref_link build.project, build.ref
   end
 
-  def build_compare_link build
-    gitlab_compare_link build.project, build.commit.short_before_sha, build.short_sha
-  end
-
   def build_commit_link build
     gitlab_commit_link build.project, build.short_sha
   end

app/models/project_services/ci/hip_chat_message.rb

@@ -11,14 +11,7 @@ module Ci
     def to_s
       lines = Array.new
       lines.push("<a href=\"#{ci_project_url(project)}\">#{project.name}</a> - ")
-      
+      lines.push("<a href=\"#{ci_project_commits_url(project, commit.sha)}\">Commit ##{commit.id}</a></br>")
-      if commit.matrix?
-        lines.push("<a href=\"#{ci_project_commits_url(project, commit.sha)}\">Commit ##{commit.id}</a></br>")
-      else
-        first_build = commit.builds_without_retry.first
-        lines.push("<a href=\"#{ci_project_build_url(project, first_build)}\">Build '#{first_build.name}' ##{first_build.id}</a></br>")
-      end
-      
       lines.push("#{commit.short_sha} #{commit.git_author_name} - #{commit.git_commit_message}</br>")
       lines.push("#{humanized_status(commit_status)} in #{commit.duration} second(s).")
       lines.join('')

app/models/project_services/ci/slack_message.rb

@@ -23,15 +23,13 @@ module Ci
     def attachments
       fields = []
 
-      if commit.matrix?
+      commit.builds_without_retry.each do |build|
-        commit.builds_without_retry.each do |build|
+        next if build.allow_failure?
-          next if build.allow_failure?
+        next unless build.failed?
-          next unless build.failed?
+        fields << {
-          fields << {
+          title: build.name,
-            title: build.name,
+          value: "Build <#{ci_project_build_url(project, build)}|\##{build.id}> failed in #{build.duration.to_i} second(s)."
-            value: "Build <#{ci_project_build_url(project, build)}|\##{build.id}> failed in #{build.duration.to_i} second(s)."
+        }
-          }
-        end
       end
 
       [{
@@ -47,12 +45,7 @@ module Ci
 
     def attachment_message
       out = "<#{ci_project_url(project)}|#{project_name}>: "
-      if commit.matrix?
+      out << "Commit <#{ci_project_commits_url(project, commit.sha)}|\##{commit.id}> "
-        out << "Commit <#{ci_project_commits_url(project, commit.sha)}|\##{commit.id}> "
-      else
-        build = commit.builds_without_retry.first
-        out << "Build <#{ci_project_build_url(project, build)}|\##{build.id}> "
-      end
       out << "(<#{commit_sha_link}|#{commit.short_sha}>) "
       out << "of <#{commit_ref_link}|#{commit.ref}> "
       out << "by #{commit.git_author_name} " if commit.git_author_name

app/models/project_services/gitlab_ci_service.rb

@@ -52,7 +52,7 @@ class GitlabCiService < CiService
 
     ci_project = Ci::Project.find_by(gitlab_id: project.id)
     if ci_project
-      Ci::CreateCommitService.new.execute(ci_project, data, current_user)
+      Ci::CreateCommitService.new.execute(ci_project, current_user, data)
     end
   end
 

app/services/ci/create_builds_service.rb

@@ -4,20 +4,23 @@ module Ci
       builds_attrs = commit.config_processor.builds_for_stage_and_ref(stage, ref, tag)
 
       builds_attrs.map do |build_attrs|
-        build_attrs.slice!(:name,
+        # don't create the same build twice
-                           :commands,
+        unless commit.builds.find_by(ref: ref, tag: tag, trigger_request: trigger_request, name: build_attrs[:name])
-                           :tag_list,
+          build_attrs.slice!(:name,
-                           :options,
+                             :commands,
-                           :allow_failure,
+                             :tag_list,
-                           :stage,
+                             :options,
-                           :stage_idx)
+                             :allow_failure,
+                             :stage,
+                             :stage_idx)
 
-        build_attrs.merge!(ref: ref,
+          build_attrs.merge!(ref: ref,
-                           tag: tag,
+                             tag: tag,
-                           trigger_request: trigger_request,
+                             trigger_request: trigger_request,
-                           user: user)
+                             user: user)
 
-        commit.builds.create!(build_attrs)
+          commit.builds.create!(build_attrs)
+        end
       end
     end
   end

app/services/ci/create_commit_service.rb

@@ -1,7 +1,6 @@
 module Ci
   class CreateCommitService
-    def execute(project, params, user)
+    def execute(project, user, params)
-      before_sha = params[:before]
       sha = params[:checkout_sha] || params[:after]
       origin_ref = params[:ref]
       

app/views/ci/commits/show.html.haml

@@ -9,9 +9,9 @@
   .gray-content-block.second-block
     .row
       .col-sm-6
-       %p
+      %p
-          %span.attr-name Commit:
+        %span.attr-name Commit:
-          #{gitlab_commit_link(@project, @commit.sha)}
+        #{gitlab_commit_link(@project, @commit.sha)}
       .col-sm-6
         - if @commit.git_author_name || @commit.git_author_email
           %p

lib/ci/api/commits.rb

@@ -51,7 +51,7 @@ module Ci
           required_attributes! [:project_id, :data, :project_token]
           project = Ci::Project.find(params[:project_id])
           authenticate_project_token!(project)
-          commit = Ci::CreateCommitService.new.execute(project, params[:data], current_user)
+          commit = Ci::CreateCommitService.new.execute(project, current_user, params[:data])
 
           if commit.persisted?
             present commit, with: Entities::CommitWithBuilds

spec/factories/ci/commits.rb

@@ -17,7 +17,7 @@
 
 # Read about factories at https://github.com/thoughtbot/factory_girl
 FactoryGirl.define do
-  factory :ci_commit, class: Ci::Commit do
+  factory :ci_empty_commit, class: Ci::Commit do
     sha '97de212e80737a608d939f648d959671fb0a0142'
 
     gl_project factory: :empty_project
@@ -40,7 +40,7 @@ FactoryGirl.define do
       end
     end
 
-    factory :ci_commit_yaml_stub do
+    factory :ci_commit do
       after(:build) do |commit|
         allow(commit).to receive(:ci_yaml_file) { File.read(Rails.root.join('spec/support/gitlab_stubs/gitlab_ci.yml')) }
       end

spec/features/ci/commits_spec.rb

@@ -38,10 +38,9 @@ describe "Commits" do
       end
 
       it "shows warning" do
-        @commit.push_data[:ci_yaml_file] = nil
+        @commit_no_yaml = FactoryGirl.create :ci_empty_commit
-        @commit.save
 
-        visit ci_commit_path(@commit)
+        visit ci_commit_path(@commit_no_yaml)
 
         expect(page).to have_content ".gitlab-ci.yml not found in this commit"
       end

spec/lib/ci/gitlab_ci_yaml_processor_spec.rb

@@ -20,8 +20,8 @@ module Ci
           except: nil,
           name: :rspec,
           only: nil,
-          script: "pwd\nrspec",
+          commands: "pwd\nrspec",
-          tags: [],
+          tag_list: [],
           options: {},
           allow_failure: false
         })
@@ -117,8 +117,8 @@ module Ci
           stage: "test",
           name: :rspec,
           only: nil,
-          script: "pwd\nrspec",
+          commands: "pwd\nrspec",
-          tags: [],
+          tag_list: [],
           options: {
             image: "ruby:2.1",
             services: ["mysql"]
@@ -143,8 +143,8 @@ module Ci
           stage: "test",
           name: :rspec,
           only: nil,
-          script: "pwd\nrspec",
+          commands: "pwd\nrspec",
-          tags: [],
+          tag_list: [],
           options: {
             image: "ruby:2.5",
             services: ["postgresql"]

spec/models/ci/build_spec.rb

@@ -28,7 +28,7 @@ require 'spec_helper'
 describe Ci::Build do
   let(:project) { FactoryGirl.create :ci_project }
   let(:gl_project) { FactoryGirl.create :empty_project, gitlab_ci_project: project }
-  let(:commit) { FactoryGirl.create :ci_commit_yaml_stub, gl_project: gl_project }
+  let(:commit) { FactoryGirl.create :ci_commit, gl_project: gl_project }
   let(:build) { FactoryGirl.create :ci_build, commit: commit }
   subject { build }
 
@@ -352,8 +352,8 @@ describe Ci::Build do
   end
 
   describe :project_recipients do
-    let (:pusher_email) { 'pusher@gitlab.test' }
+    let(:pusher_email) { 'pusher@gitlab.test' }
-    let (:user) { User.new(notification_email: pusher_email) }
+    let(:user) { User.new(notification_email: pusher_email) }
     subject { build.project_recipients }
 
     before do

spec/models/ci/commit_spec.rb

@@ -65,15 +65,6 @@ describe Ci::Commit do
     end
   end
 
-  describe :short_sha do
-    subject { commit.short_before_sha }
-
-    it 'has 8 items' do
-      expect(subject.size).to eq(8)
-    end
-    it { expect(commit.before_sha).to start_with(subject) }
-  end
-
   describe :short_sha do
     subject { commit.short_sha }
 
@@ -87,7 +78,7 @@ describe Ci::Commit do
   end
 
   describe :create_builds do
-    let(:commit) { FactoryGirl.create :ci_commit_yaml_stub, gl_project: gl_project }
+    let(:commit) { FactoryGirl.create :ci_commit, gl_project: gl_project }
 
     def create_builds(trigger_request = nil)
       commit.create_builds('master', false, nil, trigger_request)

spec/models/ci/project_services/hip_chat_message_spec.rb

@@ -7,7 +7,7 @@ describe Ci::HipChatMessage do
     let(:commit) { FactoryGirl.create(:ci_commit_with_one_job) }
 
     let(:build) do
-      commit.create_builds
+      commit.create_builds('master', false, nil)
       commit.builds.first
     end
 
@@ -43,7 +43,7 @@ describe Ci::HipChatMessage do
 
     context 'when all matrix builds succeed' do
       it 'returns a successful message' do
-        commit.create_builds
+        commit.create_builds('master', false, nil)
         commit.builds.update_all(status: "success")
         commit.reload
 
@@ -56,7 +56,7 @@ describe Ci::HipChatMessage do
 
     context 'when at least one matrix build fails' do
       it 'returns a failure message' do
-        commit.create_builds
+        commit.create_builds('master', false, nil)
         first_build = commit.builds.first
         second_build = commit.builds.last
         first_build.update(status: "success")

spec/models/ci/project_services/mail_service_spec.rb

@@ -29,12 +29,13 @@ describe Ci::MailService do
 
   describe 'Sends email for' do
     let(:mail)   { Ci::MailService.new }
+    let(:user)   { User.new(notification_email: 'git@example.com')}
 
     describe 'failed build' do
       let(:project) { FactoryGirl.create(:ci_project, email_add_pusher: true) }
       let(:gl_project) { FactoryGirl.create(:empty_project, gitlab_ci_project: project) }
       let(:commit) { FactoryGirl.create(:ci_commit, gl_project: gl_project) }
-      let(:build) { FactoryGirl.create(:ci_build, status: :failed, commit: commit) }
+      let(:build) { FactoryGirl.create(:ci_build, status: :failed, commit: commit, user: user) }
 
       before do
         allow(mail).to receive_messages(
@@ -57,7 +58,7 @@ describe Ci::MailService do
       let(:project) { FactoryGirl.create(:ci_project, email_add_pusher: true, email_only_broken_builds: false) }
       let(:gl_project) { FactoryGirl.create(:empty_project, gitlab_ci_project: project) }
       let(:commit) { FactoryGirl.create(:ci_commit, gl_project: gl_project) }
-      let(:build) { FactoryGirl.create(:ci_build, status: :success, commit: commit) }
+      let(:build) { FactoryGirl.create(:ci_build, status: :success, commit: commit, user: user) }
 
       before do
         allow(mail).to receive_messages(
@@ -85,7 +86,7 @@ describe Ci::MailService do
       end
       let(:gl_project) { FactoryGirl.create(:empty_project, gitlab_ci_project: project) }
       let(:commit) { FactoryGirl.create(:ci_commit, gl_project: gl_project) }
-      let(:build) { FactoryGirl.create(:ci_build, status: :success, commit: commit) }
+      let(:build) { FactoryGirl.create(:ci_build, status: :success, commit: commit, user: user) }
 
       before do
         allow(mail).to receive_messages(
@@ -114,7 +115,7 @@ describe Ci::MailService do
       end
       let(:gl_project) { FactoryGirl.create(:empty_project, gitlab_ci_project: project) }
       let(:commit) { FactoryGirl.create(:ci_commit, gl_project: gl_project) }
-      let(:build) { FactoryGirl.create(:ci_build, status: :success, commit: commit) }
+      let(:build) { FactoryGirl.create(:ci_build, status: :success, commit: commit, user: user) }
 
       before do
         allow(mail).to receive_messages(
@@ -143,7 +144,7 @@ describe Ci::MailService do
       end
       let(:gl_project) { FactoryGirl.create(:empty_project, gitlab_ci_project: project) }
       let(:commit) { FactoryGirl.create(:ci_commit, gl_project: gl_project) }
-      let(:build) { FactoryGirl.create(:ci_build, status: :success, commit: commit) }
+      let(:build) { FactoryGirl.create(:ci_build, status: :success, commit: commit, user: user) }
 
       before do
         allow(mail).to receive_messages(
@@ -166,7 +167,7 @@ describe Ci::MailService do
       end
       let(:gl_project) { FactoryGirl.create(:empty_project, gitlab_ci_project: project) }
       let(:commit) { FactoryGirl.create(:ci_commit, gl_project: gl_project) }
-      let(:build) { FactoryGirl.create(:ci_build, status: :failed, commit: commit) }
+      let(:build) { FactoryGirl.create(:ci_build, status: :failed, commit: commit, user: user) }
 
       before do
         allow(mail).to receive_messages(

spec/models/ci/project_services/slack_message_spec.rb

@@ -7,7 +7,7 @@ describe Ci::SlackMessage do
     let(:commit) { FactoryGirl.create(:ci_commit_with_one_job) }
 
     let(:build) do
-      commit.create_builds
+      commit.create_builds('master', false, nil)
       commit.builds.first
     end
 
@@ -47,7 +47,7 @@ describe Ci::SlackMessage do
       let(:color) { 'good' }
 
       it 'returns a message with success' do
-        commit.create_builds
+        commit.create_builds('master', false, nil)
         commit.builds.update_all(status: "success")
         commit.reload
 
@@ -63,7 +63,7 @@ describe Ci::SlackMessage do
       let(:color) { 'danger' }
 
       it 'returns a message with information about failed build' do
-        commit.create_builds
+        commit.create_builds('master', false, nil)
         first_build = commit.builds.first
         second_build = commit.builds.last
         first_build.update(status: "success")

spec/requests/ci/api/builds_spec.rb

@@ -19,7 +19,7 @@ describe Ci::API::API do
     describe "POST /builds/register" do
       it "should start a build" do
         commit = FactoryGirl.create(:ci_commit, gl_project: gl_project)
-        commit.create_builds
+        commit.create_builds('master', false, nil)
         build = commit.builds.first
 
         post ci_api("/builds/register"), token: runner.token, info: { platform: :darwin }
@@ -55,7 +55,7 @@ describe Ci::API::API do
 
       it "returns options" do
         commit = FactoryGirl.create(:ci_commit, gl_project: gl_project)
-        commit.create_builds
+        commit.create_builds('master', false, nil)
 
         post ci_api("/builds/register"), token: runner.token, info: { platform: :darwin }
 
@@ -65,7 +65,7 @@ describe Ci::API::API do
 
       it "returns variables" do
         commit = FactoryGirl.create(:ci_commit, gl_project: gl_project)
-        commit.create_builds
+        commit.create_builds('master', false, nil)
         project.variables << Ci::Variable.new(key: "SECRET_KEY", value: "secret_value")
 
         post ci_api("/builds/register"), token: runner.token, info: { platform: :darwin }
@@ -82,7 +82,7 @@ describe Ci::API::API do
         commit = FactoryGirl.create(:ci_commit, gl_project: gl_project)
 
         trigger_request = FactoryGirl.create(:ci_trigger_request_with_variables, commit: commit, trigger: trigger)
-        commit.create_builds(trigger_request)
+        commit.create_builds('master', false, nil, trigger_request)
         project.variables << Ci::Variable.new(key: "SECRET_KEY", value: "secret_value")
 
         post ci_api("/builds/register"), token: runner.token, info: { platform: :darwin }

spec/services/ci/create_commit_service_spec.rb

@@ -4,11 +4,16 @@ module Ci
   describe CreateCommitService do
     let(:service) { CreateCommitService.new }
     let(:project) { FactoryGirl.create(:ci_project) }
+    let(:user) { nil }
+
+    before do
+      stub_ci_commit_to_return_yaml_file
+    end
 
     describe :execute do
       context 'valid params' do
         let(:commit) do
-          service.execute(project,
+          service.execute(project, user,
             ref: 'refs/heads/master',
             before: '00000000',
             after: '31das312',
@@ -26,7 +31,7 @@ module Ci
 
       context "skip tag if there is no build for it" do
         it "creates commit if there is appropriate job" do
-          result = service.execute(project,
+          result = service.execute(project, user,
             ref: 'refs/tags/0_1',
             before: '00000000',
             after: '31das312',
@@ -38,8 +43,9 @@ module Ci
 
         it "creates commit if there is no appropriate job but deploy job has right ref setting" do
           config = YAML.dump({ deploy: { deploy: "ls", only: ["0_1"] } })
+          stub_ci_commit_yaml_file(config)
 
-          result = service.execute(project,
+          result = service.execute(project, user,
             ref: 'refs/heads/0_1',
             before: '00000000',
             after: '31das312',
@@ -51,11 +57,12 @@ module Ci
       end
 
       it 'fails commits without .gitlab-ci.yml' do
-        result = service.execute(project,
+        stub_ci_commit_yaml_file(nil)
+        result = service.execute(project, user,
                                  ref: 'refs/heads/0_1',
                                  before: '00000000',
                                  after: '31das312',
-                                 ci_yaml_file: config,
+                                 ci_yaml_file: nil,
                                  commits: [ { message: 'Message' } ]
         )
         expect(result).to be_persisted
@@ -64,9 +71,15 @@ module Ci
       end
 
       describe :ci_skip? do
+        let (:message) { "some message[ci skip]" }
+
+        before do
+          allow_any_instance_of(Ci::Commit).to receive(:git_commit_message) { message }
+        end
+
         it "skips builds creation if there is [ci skip] tag in commit message" do
-          commits = [{ message: "some message[ci skip]" }]
+          commits = [{ message: message }]
-          commit = service.execute(project,
+          commit = service.execute(project, user,
             ref: 'refs/tags/0_1',
             before: '00000000',
             after: '31das312',
@@ -78,9 +91,10 @@ module Ci
         end
 
         it "does not skips builds creation if there is no [ci skip] tag in commit message" do
-          commits = [{ message: "some message" }]
+          allow_any_instance_of(Ci::Commit).to receive(:git_commit_message) { "some message" }
 
-          commit = service.execute(project,
+          commits = [{ message: "some message" }]
+          commit = service.execute(project, user,
             ref: 'refs/tags/0_1',
             before: '00000000',
             after: '31das312',
@@ -92,8 +106,9 @@ module Ci
         end
 
         it "skips builds creation if there is [ci skip] tag in commit message and yaml is invalid" do
-          commits = [{ message: "some message[ci skip]" }]
+          stub_ci_commit_yaml_file('invalid: file')
-          commit = service.execute(project,
+          commits = [{ message: message }]
+          commit = service.execute(project, user,
                                    ref: 'refs/tags/0_1',
                                    before: '00000000',
                                    after: '31das312',
@@ -106,8 +121,10 @@ module Ci
       end
 
       it "skips build creation if there are already builds" do
+        allow_any_instance_of(Ci::Commit).to receive(:ci_yaml_file) { gitlab_ci_yaml }
+
         commits = [{ message: "message" }]
-        commit = service.execute(project,
+        commit = service.execute(project, user,
           ref: 'refs/heads/master',
           before: '00000000',
           after: '31das312',
@@ -116,7 +133,7 @@ module Ci
         )
         expect(commit.builds.count(:all)).to  eq(2)
 
-        commit = service.execute(project,
+        commit = service.execute(project, user,
           ref: 'refs/heads/master',
           before: '00000000',
           after: '31das312',
@@ -127,9 +144,11 @@ module Ci
       end
 
       it "creates commit with failed status if yaml is invalid" do
+        stub_ci_commit_yaml_file('invalid: file')
+
         commits = [{ message: "some message" }]
 
-        commit = service.execute(project,
+        commit = service.execute(project, user,
                                  ref: 'refs/tags/0_1',
                                  before: '00000000',
                                  after: '31das312',

spec/support/stub_gitlab_calls.rb

@@ -13,6 +13,14 @@ module StubGitlabCalls
     allow_any_instance_of(Network).to receive(:projects) { project_hash_array }
   end
 
+  def stub_ci_commit_to_return_yaml_file
+    stub_ci_commit_yaml_file(gitlab_ci_yaml)
+  end
+
+  def stub_ci_commit_yaml_file(ci_yaml)
+    allow_any_instance_of(Ci::Commit).to receive(:ci_yaml_file) { ci_yaml }
+  end
+
   private
 
   def gitlab_url