Add minor refactoring

app/models/ci/build.rb

@@ -509,10 +509,8 @@ module Ci
     end
 
     def steps
-      [
-          Gitlab::Ci::Build::Step.from_commands(self),
-          Gitlab::Ci::Build::Step.from_after_script(self)
-      ].compact
+      [Gitlab::Ci::Build::Step.from_commands(self),
+       Gitlab::Ci::Build::Step.from_after_script(self)].compact
     end
 
     def image

lib/api/runner.rb

@@ -115,7 +115,7 @@ module API
         end
       end
 
-      desc 'Appends a patch to the job.trace' do
+      desc 'Appends a patch to the job trace' do
         http_codes [[202, 'Trace was patched'],
                     [400, 'Missing Content-Range header'],
                     [403, 'Forbidden'],

lib/gitlab/ci/build/image.rb

@@ -21,8 +21,7 @@ module Gitlab
         end
 
         def initialize(image)
-          type = image.class
-          @name = image if type == String
+          @name = image
         end
 
         def valid?

lib/gitlab/ci/build/step.rb

@@ -6,7 +6,9 @@ module Gitlab
         WHEN_ON_SUCCESS = 'on_success'.freeze
         WHEN_ALWAYS = 'always'.freeze
 
-        attr_reader :name, :script, :timeout, :when, :allow_failure
+        attr_reader :name
+        attr_writer :script
+        attr_accessor :timeout, :when, :allow_failure
 
         class << self
           def from_commands(job)
@@ -25,7 +27,7 @@ module Gitlab
               step.script = after_script
               step.timeout = job.timeout
               step.when = WHEN_ALWAYS
-              step.allow_failure_on
+              step.allow_failure = true
             end
           end
         end
@@ -35,20 +37,8 @@ module Gitlab
           @allow_failure = false
         end
 
-        def script=(script)
-          @script = script.split("\n")
-        end
-
-        def timeout=(timeout)
-          @timeout = timeout
-        end
-
-        def when=(when_condition)
-          @when = when_condition
-        end
-
-        def allow_failure_on
-          @allow_failure = true
+        def script
+          @script.split("\n")
         end
       end
     end

spec/lib/gitlab/ci/build/image_spec.rb

@@ -10,18 +10,27 @@ describe Gitlab::Ci::Build::Image do
       let(:image_name) { 'ruby:2.1' }
       let(:job) { create(:ci_build, options: { image: image_name } ) }
 
-      it { is_expected.to be_kind_of(described_class) }
-      it { expect(subject.name).to eq(image_name) }
+      it 'fabricates an object of the proper class' do
+        is_expected.to be_kind_of(described_class)
+      end
+
+      it 'populates fabricated object with the proper name attribute' do
+        expect(subject.name).to eq(image_name)
+      end
 
       context 'when image name is empty' do
         let(:image_name) { '' }
 
-        it { is_expected.to eq(nil) }
+        it 'does not fabricate an object' do
+          is_expected.to be_nil
+        end
       end
     end
 
     context 'when image is not defined in job' do
-      it { is_expected.to eq(nil) }
+      it 'does not fabricate an object' do
+        is_expected.to be_nil
+      end
     end
   end
 
@@ -32,21 +41,27 @@ describe Gitlab::Ci::Build::Image do
       let(:service_image_name) { 'postgres' }
       let(:job) { create(:ci_build, options: { services: [service_image_name] }) }
 
-      it { is_expected.to be_kind_of(Array) }
-      it { is_expected.not_to be_empty }
-      it { expect(subject[0].name).to eq(service_image_name) }
+      it 'fabricates an non-empty array of objects' do
+        is_expected.to be_kind_of(Array)
+        is_expected.not_to be_empty
+        expect(subject.first.name).to eq(service_image_name)
+      end
 
       context 'when service image name is empty' do
         let(:service_image_name) { '' }
 
-        it { is_expected.to be_kind_of(Array) }
-        it { is_expected.to be_empty }
+        it 'fabricates an empty array' do
+          is_expected.to be_kind_of(Array)
+          is_expected.to be_empty
+        end
       end
     end
 
     context 'when services are not defined in job' do
-      it { is_expected.to be_kind_of(Array) }
-      it { is_expected.to be_empty }
+      it 'fabricates an empty array' do
+        is_expected.to be_kind_of(Array)
+        is_expected.to be_empty
+      end
     end
   end
 end

spec/lib/gitlab/ci/build/step_spec.rb

@@ -6,28 +6,34 @@ describe Gitlab::Ci::Build::Step do
   describe '#from_commands' do
     subject { described_class.from_commands(job) }
 
-    it { expect(subject.name).to eq(:script) }
-    it { expect(subject.script).to eq(['ls -la', 'date']) }
-    it { expect(subject.timeout).to eq(job.timeout) }
-    it { expect(subject.when).to eq('on_success') }
-    it { expect(subject.allow_failure).to be_falsey }
+    it 'fabricates an object' do
+      expect(subject.name).to eq(:script)
+      expect(subject.script).to eq(['ls -la', 'date'])
+      expect(subject.timeout).to eq(job.timeout)
+      expect(subject.when).to eq('on_success')
+      expect(subject.allow_failure).to be_falsey
+    end
   end
 
   describe '#from_after_script' do
     subject { described_class.from_after_script(job) }
 
     context 'when after_script is empty' do
-      it { is_expected.to be(nil) }
+      it 'doesn not fabricate an object' do
+        is_expected.to be_nil
+      end
     end
 
     context 'when after_script is not empty' do
       let(:job) { create(:ci_build, options: { after_script: "ls -la\ndate" }) }
 
-      it { expect(subject.name).to eq(:after_script) }
-      it { expect(subject.script).to eq(['ls -la', 'date']) }
-      it { expect(subject.timeout).to eq(job.timeout) }
-      it { expect(subject.when).to eq('always') }
-      it { expect(subject.allow_failure).to be_truthy }
+      it 'fabricates an object' do
+        expect(subject.name).to eq(:after_script)
+        expect(subject.script).to eq(['ls -la', 'date'])
+        expect(subject.timeout).to eq(job.timeout)
+        expect(subject.when).to eq('always')
+        expect(subject.allow_failure).to be_truthy
+      end
     end
   end
 end

spec/requests/api/runner_spec.rb

@@ -155,7 +155,10 @@ describe API::Runner do
     let(:project) { create(:empty_project, shared_runners_enabled: false) }
     let(:pipeline) { create(:ci_pipeline_without_jobs, project: project, ref: 'master') }
     let(:runner) { create(:ci_runner) }
-    let!(:job) { create(:ci_build, :artifacts, :extended_options, pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0, commands: "ls\ndate") }
+    let!(:job) do
+      create(:ci_build, :artifacts, :extended_options,
+             pipeline: pipeline, name: 'spinach', stage: 'test', stage_idx: 0, commands: "ls\ndate")
+    end
 
     before { project.runners << runner }
 
@@ -283,6 +286,7 @@ describe API::Runner do
               'project_id' => job.project.id,
               'project_name' => job.project.name }
           end
+
           let(:expected_git_info) do
             { 'repo_url' => job.repo_url,
               'ref' => job.ref,
@@ -290,6 +294,7 @@ describe API::Runner do
               'before_sha' => job.before_sha,
               'ref_type' => 'branch' }
           end
+
           let(:expected_steps) do
             [{ 'name' => 'script',
                'script' => %w(ls date),
@@ -302,11 +307,13 @@ describe API::Runner do
                'when' => 'always',
                'allow_failure' => true }]
           end
+
           let(:expected_variables) do
             [{ 'key' => 'CI_BUILD_NAME', 'value' => 'spinach', 'public' => true },
              { 'key' => 'CI_BUILD_STAGE', 'value' => 'test', 'public' => true },
              { 'key' => 'DB_NAME', 'value' => 'postgres', 'public' => true }]
           end
+
           let(:expected_artifacts) do
             [{ 'name' => 'artifacts_file',
                'untracked' => false,
@@ -314,13 +321,14 @@ describe API::Runner do
                'when' => 'always',
                'expire_in' => '7d' }]
           end
+
           let(:expected_cache) do
             [{ 'key' => 'cache_key',
                'untracked' => false,
                'paths' => ['vendor/*'] }]
           end
 
-          it 'starts a job' do
+          it 'picks a job' do
             request_job info: { platform: :darwin }
 
             expect(response).to have_http_status(201)