From 6323cd7203dbf1850e7939e81db4b1a9c6cf6d76 Mon Sep 17 00:00:00 2001
From: Leandro Camargo <leandroico@gmail.com>
Date: Mon, 5 Dec 2016 02:00:47 -0200
Subject: [PATCH] Comply to more requirements and requests made in the code
review
---
app/models/ci/build.rb | 2 +-
doc/ci/yaml/README.md | 16 +++++++++-------
lib/gitlab/ci/config/entry/coverage.rb | 2 +-
.../ci/config/entry/legacy_validation_helpers.rb | 5 ++---
spec/lib/ci/gitlab_ci_yaml_processor_spec.rb | 9 +++++++--
spec/models/ci/build_spec.rb | 11 ++++++++---
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
--
GitLab