Comply to more requirements and requests made in the code review

6 files changed, 28 insertions, 17 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 46a6b4c724a..951818ad561 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -522,7 +522,7 @@ module Ci
     end
 
     def coverage_regex
-      read_attribute(:coverage_regex) || project.build_coverage_regex
+      super || project.build_coverage_regex
     end
 
     def when
diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index 0a264c0e228..5e2d9788f33 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -286,11 +286,13 @@ build outputs. Setting this up globally will make all the jobs to use this
 setting for output filtering and extracting the coverage information from your
 builds.
 
-Regular expressions are used by default. So using surrounding `/` is optional, given it'll always be read as a regular expression. Don't forget to escape special characters whenever you want to match them in the regular expression.
+Regular expressions are used by default. So using surrounding `/` is optional,
+given it'll always be read as a regular expression. Don't forget to escape
+special characters whenever you want to match them literally.
 
 A simple example:
 ```yaml
-coverage: \(\d+\.\d+\) covered\.
+coverage: /\(\d+\.\d+\) covered\./
 ```
 
 ## Jobs
@@ -1014,19 +1016,19 @@ job:
 This entry is pretty much the same as described in the global context in
 [`coverage`](#coverage). The only difference is that, by setting it inside
 the job level, whatever is set in there will take precedence over what has
-been defined in the global level. A quick example of one overwritting the
+been defined in the global level. A quick example of one overriding the
 other would be:
 
 ```yaml
-coverage: \(\d+\.\d+\) covered\.
+coverage: /\(\d+\.\d+\) covered\./
 
 job1:
-  coverage: Code coverage: \d+\.\d+
+  coverage: /Code coverage: \d+\.\d+/
 ```
 
 In the example above, considering the context of the job `job1`, the coverage
-regex that would be used is `Code coverage: \d+\.\d+` instead of
-`\(\d+\.\d+\) covered\.`.
+regex that would be used is `/Code coverage: \d+\.\d+/` instead of
+`/\(\d+\.\d+\) covered\./`.
 
 ## Git Strategy
 
diff --git a/lib/gitlab/ci/config/entry/coverage.rb b/lib/gitlab/ci/config/entry/coverage.rb
index aa738fcfd11..706bfc882de 100644
--- a/lib/gitlab/ci/config/entry/coverage.rb
+++ b/lib/gitlab/ci/config/entry/coverage.rb
@@ -13,7 +13,7 @@ module Gitlab
           end
 
           def value
-            if @config.start_with?('/') && @config.end_with?('/')
+            if @config.first == '/' && @config.last == '/'
               @config[1...-1]
             else
               @config
diff --git a/lib/gitlab/ci/config/entry/legacy_validation_helpers.rb b/lib/gitlab/ci/config/entry/legacy_validation_helpers.rb
index d8e74b15712..9b9a0a8125a 100644
--- a/lib/gitlab/ci/config/entry/legacy_validation_helpers.rb
+++ b/lib/gitlab/ci/config/entry/legacy_validation_helpers.rb
@@ -29,8 +29,7 @@ module Gitlab
           end
 
           def validate_regexp(value)
-            Regexp.new(value)
-            true
+            !value.nil? && Regexp.new(value.to_s) && true
           rescue RegexpError, TypeError
             false
           end
@@ -39,7 +38,7 @@ module Gitlab
             return true if value.is_a?(Symbol)
             return false unless value.is_a?(String)
 
-            if value.start_with?('/') && value.end_with?('/')
+            if value.first == '/' && value.last == '/'
               validate_regexp(value[1...-1])
             else
               true
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index ac706216d5a..3ffcfaa1f29 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -12,14 +12,19 @@ module Ci
         let(:config) { YAML.dump(config_base) }
 
         context 'when config has coverage set at the global scope' do
-          before { config_base.update(coverage: '\(\d+\.\d+\) covered') }
+          before do
+            config_base.update(coverage: '\(\d+\.\d+\) covered')
+          end
 
           context "and 'rspec' job doesn't have coverage set" do
             it { is_expected.to include(coverage_regex: '\(\d+\.\d+\) covered') }
           end
 
           context 'but \'rspec\' job also has coverage set' do
-            before { config_base[:rspec][:coverage] = '/Code coverage: \d+\.\d+/' }
+            before do
+              config_base[:rspec][:coverage] = '/Code coverage: \d+\.\d+/'
+            end
+
             it { is_expected.to include(coverage_regex: 'Code coverage: \d+\.\d+') }
           end
         end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 7c054dd95f5..7baaef9c85e 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -228,14 +228,19 @@ describe Ci::Build, :models do
     let(:build_regex) { 'Code coverage: \d+\.\d+' }
 
     context 'when project has build_coverage_regex set' do
-      before { project.build_coverage_regex = project_regex }
+      before do
+        project.build_coverage_regex = project_regex
+      end
 
       context 'and coverage_regex attribute is not set' do
         it { is_expected.to eq(project_regex) }
       end
 
       context 'but coverage_regex attribute is also set' do
-        before { build.coverage_regex = build_regex }
+        before do
+          build.coverage_regex = build_regex
+        end
+
         it { is_expected.to eq(build_regex) }
       end
     end
@@ -250,7 +255,7 @@ describe Ci::Build, :models do
       build.coverage_regex = '\(\d+.\d+\%\) covered'
       allow(build).to receive(:trace) { 'Coverage 1033 / 1051 LOC (98.29%) covered' }
       allow(build).to receive(:coverage_regex).and_call_original
-      allow(build).to receive(:update_attributes).with(coverage: 98.29) { true }
+      expect(build).to receive(:update_attributes).with(coverage: 98.29) { true }
       expect(build.update_coverage).to be true
     end
   end