Commit be7106a1 authored by Leandro Camargo's avatar Leandro Camargo

Force coverage value to always be surrounded by '/'

parent f8bec0d1
...@@ -275,8 +275,10 @@ module Ci ...@@ -275,8 +275,10 @@ module Ci
end end
def update_coverage def update_coverage
return unless project regex = coverage_regex.to_s[1...-1]
return unless regex = self.coverage_regex
return if regex.blank?
coverage = extract_coverage(trace, regex) coverage = extract_coverage(trace, regex)
if coverage.is_a? Numeric if coverage.is_a? Numeric
...@@ -522,7 +524,9 @@ module Ci ...@@ -522,7 +524,9 @@ module Ci
end end
def coverage_regex def coverage_regex
super || project.build_coverage_regex super ||
project.try(:build_coverage_regex).presence &&
"/#{project.try(:build_coverage_regex)}/"
end end
def when def when
......
...@@ -286,9 +286,10 @@ build outputs. Setting this up globally will make all the jobs to use this ...@@ -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 setting for output filtering and extracting the coverage information from your
builds. builds.
Regular expressions are used by default. So using surrounding `/` is optional, Regular expressions are the only valid kind of value expected here. So, using
given it'll always be read as a regular expression. Don't forget to escape surrounding `/` is mandatory in order to consistently and explicitly represent
special characters whenever you want to match them literally. a regular expression string. You must escape special characters if you want to
match them literally.
A simple example: A simple example:
```yaml ```yaml
......
...@@ -11,14 +11,6 @@ module Gitlab ...@@ -11,14 +11,6 @@ module Gitlab
validations do validations do
validates :config, regexp: true validates :config, regexp: true
end end
def value
if @config.first == '/' && @config.last == '/'
@config[1...-1]
else
@config
end
end
end end
end end
end end
......
...@@ -9,15 +9,7 @@ module Gitlab ...@@ -9,15 +9,7 @@ module Gitlab
include Validatable include Validatable
validations do validations do
include LegacyValidationHelpers validates :config, array_of_strings_or_regexps: true
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
end end
end end
end end
......
...@@ -62,6 +62,45 @@ module Gitlab ...@@ -62,6 +62,45 @@ module Gitlab
record.errors.add(attribute, 'must be a regular expression') record.errors.add(attribute, 'must be a regular expression')
end end
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 end
class TypeValidator < ActiveModel::EachValidator class TypeValidator < ActiveModel::EachValidator
......
...@@ -13,11 +13,11 @@ module Ci ...@@ -13,11 +13,11 @@ module Ci
context 'when config has coverage set at the global scope' do context 'when config has coverage set at the global scope' do
before do before do
config_base.update(coverage: '\(\d+\.\d+\) covered') config_base.update(coverage: '/\(\d+\.\d+\) covered/')
end end
context "and 'rspec' job doesn't have coverage set" do 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 end
context "but 'rspec' job also has coverage set" do context "but 'rspec' job also has coverage set" do
...@@ -25,7 +25,7 @@ module Ci ...@@ -25,7 +25,7 @@ module Ci
config_base[:rspec][:coverage] = '/Code coverage: \d+\.\d+/' config_base[:rspec][:coverage] = '/Code coverage: \d+\.\d+/'
end 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 end
end end
......
...@@ -4,31 +4,26 @@ describe Gitlab::Ci::Config::Entry::Coverage do ...@@ -4,31 +4,26 @@ describe Gitlab::Ci::Config::Entry::Coverage do
let(:entry) { described_class.new(config) } let(:entry) { described_class.new(config) }
describe 'validations' do 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+' } let(:config) { 'Code coverage: \d+\.\d+' }
describe '#value' do
subject { entry.value }
it { is_expected.to eq(config) }
end
describe '#errors' do describe '#errors' do
subject { entry.errors } subject { entry.errors }
it { is_expected.to be_empty } it { is_expected.to include(/coverage config must be a regular expression/) }
end end
describe '#valid?' do describe '#valid?' do
subject { entry } subject { entry }
it { is_expected.to be_valid } it { is_expected.not_to be_valid }
end end
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+/' } let(:config) { '/Code coverage: \d+\.\d+/' }
describe '#value' do describe '#value' do
subject { entry.value } subject { entry.value }
it { is_expected.to eq(config[1...-1]) } it { is_expected.to eq(config) }
end end
describe '#errors' do describe '#errors' do
...@@ -42,7 +37,7 @@ describe Gitlab::Ci::Config::Entry::Coverage do ...@@ -42,7 +37,7 @@ describe Gitlab::Ci::Config::Entry::Coverage do
end end
end end
context 'when entry value is not correct' do context 'when entry value is not valid' do
let(:config) { '(malformed regexp' } let(:config) { '(malformed regexp' }
describe '#errors' do describe '#errors' do
......
...@@ -224,19 +224,20 @@ describe Ci::Build, :models do ...@@ -224,19 +224,20 @@ describe Ci::Build, :models do
describe '#coverage_regex' do describe '#coverage_regex' do
subject { build.coverage_regex } subject { build.coverage_regex }
context 'when project has build_coverage_regex set' do
let(:project_regex) { '\(\d+\.\d+\) covered' } let(:project_regex) { '\(\d+\.\d+\) covered' }
let(:build_regex) { 'Code coverage: \d+\.\d+' }
context 'when project has build_coverage_regex set' do
before do before do
project.build_coverage_regex = project_regex project.build_coverage_regex = project_regex
end end
context 'and coverage_regex attribute is not set' do context 'and coverage_regex attribute is not set' do
it { is_expected.to eq(project_regex) } it { is_expected.to eq("/#{project_regex}/") }
end end
context 'but coverage_regex attribute is also set' do context 'but coverage_regex attribute is also set' do
let(:build_regex) { '/Code coverage: \d+\.\d+/' }
before do before do
build.coverage_regex = build_regex build.coverage_regex = build_regex
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment