Force coverage value to always be surrounded by '/'

8 files changed, 65 insertions, 41 deletions

diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 951818ad561..e3753869b67 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -275,8 +275,10 @@ module Ci
     end
 
     def update_coverage
-      return unless project
-      return unless regex = self.coverage_regex
+      regex = coverage_regex.to_s[1...-1]
+
+      return if regex.blank?
+
       coverage = extract_coverage(trace, regex)
 
       if coverage.is_a? Numeric
@@ -522,7 +524,9 @@ module Ci
     end
 
     def coverage_regex
-      super || project.build_coverage_regex
+      super ||
+        project.try(:build_coverage_regex).presence &&
+        "/#{project.try(:build_coverage_regex)}/"
     end
 
     def when
diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md
index 5e2d9788f33..85b2c75cee8 100644
--- a/doc/ci/yaml/README.md
+++ b/doc/ci/yaml/README.md
@@ -286,9 +286,10 @@ 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 literally.
+Regular expressions are the only valid kind of value expected here. So, using
+surrounding `/` is mandatory in order to consistently and explicitly represent
+a regular expression string. You must escape special characters if you want to
+match them literally.
 
 A simple example:
 ```yaml
diff --git a/lib/gitlab/ci/config/entry/coverage.rb b/lib/gitlab/ci/config/entry/coverage.rb
index 706bfc882de..25546f363fb 100644
--- a/lib/gitlab/ci/config/entry/coverage.rb
+++ b/lib/gitlab/ci/config/entry/coverage.rb
@@ -11,14 +11,6 @@ module Gitlab
           validations do
             validates :config, regexp: true
           end
-
-          def value
-            if @config.first == '/' && @config.last == '/'
-              @config[1...-1]
-            else
-              @config
-            end
-          end
         end
       end
     end
diff --git a/lib/gitlab/ci/config/entry/trigger.rb b/lib/gitlab/ci/config/entry/trigger.rb
index 28b0a9ffe01..16b234e6c59 100644
--- a/lib/gitlab/ci/config/entry/trigger.rb
+++ b/lib/gitlab/ci/config/entry/trigger.rb
@@ -9,15 +9,7 @@ module Gitlab
           include Validatable
 
           validations do
-            include LegacyValidationHelpers
-
-            validate :array_of_strings_or_regexps
-
-            def array_of_strings_or_regexps
-              unless validate_array_of_strings_or_regexps(config)
-                errors.add(:config, 'should be an array of strings or regexps')
-              end
-            end
+            validates :config, array_of_strings_or_regexps: true
           end
         end
       end
diff --git a/lib/gitlab/ci/config/entry/validators.rb b/lib/gitlab/ci/config/entry/validators.rb
index 03a8205b081..5f50b80af6c 100644
--- a/lib/gitlab/ci/config/entry/validators.rb
+++ b/lib/gitlab/ci/config/entry/validators.rb
@@ -62,6 +62,45 @@ module Gitlab
                 record.errors.add(attribute, 'must be a regular expression')
               end
             end
+
+            private
+
+            def look_like_regexp?(value)
+              value =~ %r{\A/.*/\z}
+            end
+
+            def validate_regexp(value)
+              look_like_regexp?(value) &&
+                Regexp.new(value.to_s[1...-1]) &&
+                true
+            rescue RegexpError
+              false
+            end
+          end
+
+          class ArrayOfStringsOrRegexps < RegexpValidator
+            def validate_each(record, attribute, value)
+              unless validate_array_of_strings_or_regexps(value)
+                record.errors.add(attribute, 'should be an array of strings or regexps')
+              end
+            end
+
+            private
+
+            def validate_array_of_strings_or_regexps(values)
+              values.is_a?(Array) && values.all?(&method(:validate_string_or_regexp))
+            end
+
+            def validate_string_or_regexp(value)
+              return true if value.is_a?(Symbol)
+              return false unless value.is_a?(String)
+
+              if look_like_regexp?(value)
+                validate_regexp(value)
+              else
+                true
+              end
+            end
           end
 
           class TypeValidator < ActiveModel::EachValidator
diff --git a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
index b1e09350847..e2302f5968a 100644
--- a/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
+++ b/spec/lib/ci/gitlab_ci_yaml_processor_spec.rb
@@ -13,11 +13,11 @@ module Ci
 
         context 'when config has coverage set at the global scope' do
           before do
-            config_base.update(coverage: '\(\d+\.\d+\) covered')
+            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') }
+            it { is_expected.to include(coverage_regex: '/\(\d+\.\d+\) covered/') }
           end
 
           context "but 'rspec' job also has coverage set" do
@@ -25,7 +25,7 @@ module Ci
               config_base[:rspec][:coverage] = '/Code coverage: \d+\.\d+/'
             end
 
-            it { is_expected.to include(coverage_regex: 'Code coverage: \d+\.\d+') }
+            it { is_expected.to include(coverage_regex: '/Code coverage: \d+\.\d+/') }
           end
         end
       end
diff --git a/spec/lib/gitlab/ci/config/entry/coverage_spec.rb b/spec/lib/gitlab/ci/config/entry/coverage_spec.rb
index 8f989ebd732..eb04075f1be 100644
--- a/spec/lib/gitlab/ci/config/entry/coverage_spec.rb
+++ b/spec/lib/gitlab/ci/config/entry/coverage_spec.rb
@@ -4,31 +4,26 @@ describe Gitlab::Ci::Config::Entry::Coverage do
   let(:entry) { described_class.new(config) }
 
   describe 'validations' do
-    context "when entry config value is correct without surrounding '/'" do
+    context "when entry config value doesn't have the surrounding '/'" do
       let(:config) { 'Code coverage: \d+\.\d+' }
 
-      describe '#value' do
-        subject { entry.value }
-        it { is_expected.to eq(config) }
-      end
-
       describe '#errors' do
         subject { entry.errors }
-        it { is_expected.to be_empty }
+        it { is_expected.to include(/coverage config must be a regular expression/) }
       end
 
       describe '#valid?' do
         subject { entry }
-        it { is_expected.to be_valid }
+        it { is_expected.not_to be_valid }
       end
     end
 
-    context "when entry config value is correct with surrounding '/'" do
+    context "when entry config value has the surrounding '/'" do
       let(:config) { '/Code coverage: \d+\.\d+/' }
 
       describe '#value' do
         subject { entry.value }
-        it { is_expected.to eq(config[1...-1]) }
+        it { is_expected.to eq(config) }
       end
 
       describe '#errors' do
@@ -42,7 +37,7 @@ describe Gitlab::Ci::Config::Entry::Coverage do
       end
     end
 
-    context 'when entry value is not correct' do
+    context 'when entry value is not valid' do
       let(:config) { '(malformed regexp' }
 
       describe '#errors' do
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 52cc45f07b2..f23155a5d13 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -224,19 +224,20 @@ describe Ci::Build, :models do
   describe '#coverage_regex' do
     subject { build.coverage_regex }
 
-    let(:project_regex) { '\(\d+\.\d+\) covered' }
-    let(:build_regex) { 'Code coverage: \d+\.\d+' }
-
     context 'when project has build_coverage_regex set' do
+      let(:project_regex) { '\(\d+\.\d+\) covered' }
+
       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) }
+        it { is_expected.to eq("/#{project_regex}/") }
       end
 
       context 'but coverage_regex attribute is also set' do
+        let(:build_regex) { '/Code coverage: \d+\.\d+/' }
+
         before do
           build.coverage_regex = build_regex
         end