diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 4b1299c7aeec6487e27a9040fec0b1aaad46ba0c..9babae4175112af5c47222be1e5a0c7e1fc57b5a 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -916,8 +916,20 @@ def collect_accessibility_reports!(accessibility_report)
end
def collect_coverage_reports!(coverage_report)
+ project_path, worktree_paths = if Feature.enabled?(:smart_cobertura_parser, project)
+ # If the flag is disabled, we intentionally pass nil
+ # for both project_path and worktree_paths to fallback
+ # to the non-smart behavior of the parser
+ [project.full_path, pipeline.all_worktree_paths]
+ end
+
each_report(Ci::JobArtifact::COVERAGE_REPORT_FILE_TYPES) do |file_type, blob|
- Gitlab::Ci::Parsers.fabricate!(file_type).parse!(blob, coverage_report)
+ Gitlab::Ci::Parsers.fabricate!(file_type).parse!(
+ blob,
+ coverage_report,
+ project_path: project_path,
+ worktree_paths: worktree_paths
+ )
end
coverage_report
diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index 99bc894b81290a54d6db09f25318dc96f713953e..a55f9d41df0ffc716c1a24baeddb12a58a4c9234 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -975,7 +975,7 @@ def accessibility_reports
def coverage_reports
Gitlab::Ci::Reports::CoverageReports.new.tap do |coverage_reports|
- latest_report_builds(Ci::JobArtifact.coverage_reports).each do |build|
+ latest_report_builds(Ci::JobArtifact.coverage_reports).includes(:project).find_each do |build|
build.collect_coverage_reports!(coverage_reports)
end
end
diff --git a/changelogs/unreleased/eb-cobertura-background-fix.yml b/changelogs/unreleased/eb-cobertura-background-fix.yml
new file mode 100644
index 0000000000000000000000000000000000000000..5f679d6b54fb4ad26cc70303db732a233b6fe094
--- /dev/null
+++ b/changelogs/unreleased/eb-cobertura-background-fix.yml
@@ -0,0 +1,5 @@
+---
+title: Implement smart cobertura class path correction
+merge_request: 48048
+author:
+type: changed
diff --git a/config/feature_flags/development/smart_cobertura_parser.yml b/config/feature_flags/development/smart_cobertura_parser.yml
new file mode 100644
index 0000000000000000000000000000000000000000..a3aa182e412b3c715ac3a6c1fc0952787b604dfa
--- /dev/null
+++ b/config/feature_flags/development/smart_cobertura_parser.yml
@@ -0,0 +1,8 @@
+---
+name: smart_cobertura_parser
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48048
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/284822
+milestone: '13.7'
+type: development
+group: group::testing
+default_enabled: false
diff --git a/lib/gitlab/ci/parsers/coverage/cobertura.rb b/lib/gitlab/ci/parsers/coverage/cobertura.rb
index 934c797580c35e4e8b47af12634eead563a66218..1edcbac2f25470166e789d8cdbd55a0d71d9e9bd 100644
--- a/lib/gitlab/ci/parsers/coverage/cobertura.rb
+++ b/lib/gitlab/ci/parsers/coverage/cobertura.rb
@@ -5,50 +5,113 @@ module Ci
module Parsers
module Coverage
class Cobertura
- CoberturaParserError = Class.new(Gitlab::Ci::Parsers::ParserError)
+ InvalidXMLError = Class.new(Gitlab::Ci::Parsers::ParserError)
+ InvalidLineInformationError = Class.new(Gitlab::Ci::Parsers::ParserError)
- def parse!(xml_data, coverage_report)
+ GO_SOURCE_PATTERN = '/usr/local/go/src'
+ MAX_SOURCES = 100
+
+ def parse!(xml_data, coverage_report, project_path: nil, worktree_paths: nil)
root = Hash.from_xml(xml_data)
- parse_all(root, coverage_report)
+ context = {
+ project_path: project_path,
+ paths: worktree_paths&.to_set,
+ sources: []
+ }
+
+ parse_all(root, coverage_report, context)
rescue Nokogiri::XML::SyntaxError
- raise CoberturaParserError, "XML parsing failed"
- rescue
- raise CoberturaParserError, "Cobertura parsing failed"
+ raise InvalidXMLError, "XML parsing failed"
end
private
- def parse_all(root, coverage_report)
+ def parse_all(root, coverage_report, context)
return unless root.present?
root.each do |key, value|
- parse_node(key, value, coverage_report)
+ parse_node(key, value, coverage_report, context)
end
end
- def parse_node(key, value, coverage_report)
- return if key == 'sources'
-
- if key == 'class'
+ def parse_node(key, value, coverage_report, context)
+ if key == 'sources' && value['source'].present?
+ parse_sources(value['source'], context)
+ elsif key == 'package'
Array.wrap(value).each do |item|
- parse_class(item, coverage_report)
+ parse_package(item, coverage_report, context)
+ end
+ elsif key == 'class'
+ # This means the cobertura XML does not have classes within package nodes.
+ # This is possible in some cases like in simple JS project structures
+ # running Jest.
+ Array.wrap(value).each do |item|
+ parse_class(item, coverage_report, context)
end
elsif value.is_a?(Hash)
- parse_all(value, coverage_report)
+ parse_all(value, coverage_report, context)
elsif value.is_a?(Array)
value.each do |item|
- parse_all(item, coverage_report)
+ parse_all(item, coverage_report, context)
end
end
end
- def parse_class(file, coverage_report)
+ def parse_sources(sources, context)
+ return unless context[:project_path] && context[:paths]
+
+ sources = Array.wrap(sources)
+
+ # TODO: Go cobertura has a different format with how their packages
+ # are included in the filename. So we can't rely on the sources.
+ # We'll deal with this later.
+ return if sources.include?(GO_SOURCE_PATTERN)
+
+ sources.each do |source|
+ source = build_source_path(source, context)
+ context[:sources] << source if source.present?
+ end
+ end
+
+ def build_source_path(source, context)
+ # | raw source | extracted |
+ # |-----------------------------|------------|
+ # | /builds/foo/test/SampleLib/ | SampleLib/ |
+ # | /builds/foo/test/something | something |
+ # | /builds/foo/test/ | nil |
+ # | /builds/foo/test | nil |
+ source.split("#{context[:project_path]}/", 2)[1]
+ end
+
+ def parse_package(package, coverage_report, context)
+ classes = package.dig('classes', 'class')
+ return unless classes.present?
+
+ matched_filenames = Array.wrap(classes).map do |item|
+ parse_class(item, coverage_report, context)
+ end
+
+ # Remove these filenames from the paths to avoid conflict
+ # with other packages that may contain the same class filenames
+ remove_matched_filenames(matched_filenames, context)
+ end
+
+ def remove_matched_filenames(filenames, context)
+ return unless context[:paths]
+
+ filenames.each { |f| context[:paths].delete(f) }
+ end
+
+ def parse_class(file, coverage_report, context)
return unless file["filename"].present? && file["lines"].present?
parsed_lines = parse_lines(file["lines"])
+ filename = determine_filename(file["filename"], context)
+
+ coverage_report.add_file(filename, Hash[parsed_lines]) if filename
- coverage_report.add_file(file["filename"], Hash[parsed_lines])
+ filename
end
def parse_lines(lines)
@@ -58,6 +121,27 @@ def parse_lines(lines)
# Using `Integer()` here to raise exception on invalid values
[Integer(line["number"]), Integer(line["hits"])]
end
+ rescue
+ raise InvalidLineInformationError, "Line information had invalid values"
+ end
+
+ def determine_filename(filename, context)
+ return filename unless context[:sources].any?
+
+ full_filename = nil
+
+ context[:sources].each_with_index do |source, index|
+ break if index >= MAX_SOURCES
+ break if full_filename = check_source(source, filename, context)
+ end
+
+ full_filename
+ end
+
+ def check_source(source, filename, context)
+ full_path = File.join(source, filename)
+
+ return full_path if context[:paths].include?(full_path)
end
end
end
diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb
index 0f5ad013a6462841ee0b759426246334eec5a25a..ad98e9d1f2420b7d3513f17a8382d810034a8bf6 100644
--- a/spec/factories/ci/job_artifacts.rb
+++ b/spec/factories/ci/job_artifacts.rb
@@ -229,6 +229,16 @@
end
end
+ trait :coverage_with_paths_not_relative_to_project_root do
+ file_type { :cobertura }
+ file_format { :gzip }
+
+ after(:build) do |artifact, evaluator|
+ artifact.file = fixture_file_upload(
+ Rails.root.join('spec/fixtures/cobertura/coverage_with_paths_not_relative_to_project_root.xml.gz'), 'application/x-gzip')
+ end
+ end
+
trait :coverage_with_corrupted_data do
file_type { :cobertura }
file_format { :gzip }
diff --git a/spec/fixtures/cobertura/coverage_with_paths_not_relative_to_project_root.xml.gz b/spec/fixtures/cobertura/coverage_with_paths_not_relative_to_project_root.xml.gz
new file mode 100644
index 0000000000000000000000000000000000000000..c4adc63fccef72f67f431e80a6aa9fce2f2263f9
Binary files /dev/null and b/spec/fixtures/cobertura/coverage_with_paths_not_relative_to_project_root.xml.gz differ
diff --git a/spec/lib/gitlab/ci/parsers/coverage/cobertura_spec.rb b/spec/lib/gitlab/ci/parsers/coverage/cobertura_spec.rb
index 45e87466532b755261d022bf386bb712d173cebd..2313378d1e9f0796d4b431ac38acdd4894f85db4 100644
--- a/spec/lib/gitlab/ci/parsers/coverage/cobertura_spec.rb
+++ b/spec/lib/gitlab/ci/parsers/coverage/cobertura_spec.rb
@@ -4,207 +4,690 @@
RSpec.describe Gitlab::Ci::Parsers::Coverage::Cobertura do
describe '#parse!' do
- subject { described_class.new.parse!(cobertura, coverage_report) }
+ subject(:parse_report) { described_class.new.parse!(cobertura, coverage_report, project_path: project_path, worktree_paths: paths) }
let(:coverage_report) { Gitlab::Ci::Reports::CoverageReports.new }
+ let(:project_path) { 'foo/bar' }
+ let(:paths) { ['app/user.rb'] }
+
+ let(:cobertura) do
+ <<~EOF
+ <coverage>
+ #{sources_xml}
+ #{classes_xml}
+ </coverage>
+ EOF
+ end
context 'when data is Cobertura style XML' do
- context 'when there is no <class>' do
- let(:cobertura) { '' }
+ shared_examples_for 'ignoring sources, project_path, and worktree_paths' do
+ context 'when there is no <class>' do
+ let(:classes_xml) { '' }
- it 'parses XML and returns empty coverage' do
- expect { subject }.not_to raise_error
+ it 'parses XML and returns empty coverage' do
+ expect { parse_report }.not_to raise_error
- expect(coverage_report.files).to eq({})
+ expect(coverage_report.files).to eq({})
+ end
end
- end
- context 'when there is a <sources>' do
- shared_examples_for 'ignoring sources' do
- it 'parses XML without errors' do
- expect { subject }.not_to raise_error
+ context 'when there is a single <class>' do
+ context 'with no lines' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="app.rb"></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns empty coverage' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({})
+ end
+ end
- expect(coverage_report.files).to eq({})
+ context 'with a single line' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="app.rb"><lines>
+ <line number="1" hits="2"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns a single file with coverage' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app.rb' => { 1 => 2 } })
+ end
+ end
+
+ context 'without a package parent' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages>
+ <class filename="app.rb"><lines>
+ <line number="1" hits="2"/>
+ </lines></class>
+ </packages>
+ EOF
+ end
+
+ it 'parses XML and returns a single file with coverage' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app.rb' => { 1 => 2 } })
+ end
+ end
+
+ context 'with multiple lines and methods info' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="app.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns a single file with coverage' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app.rb' => { 1 => 2, 2 => 0 } })
+ end
end
end
- context 'and has a single source' do
- let(:cobertura) do
- <<-EOF.strip_heredoc
+ context 'when there are multiple <class>' do
+ context 'without a package parent' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages>
+ <class filename="app.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ </lines></class>
+ <class filename="foo.rb"><methods/><lines>
+ <line number="6" hits="1"/>
+ </lines></class>
+ </packages>
+ EOF
+ end
+
+ it 'parses XML and returns coverage information per class' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app.rb' => { 1 => 2 }, 'foo.rb' => { 6 => 1 } })
+ end
+ end
+
+ context 'with the same filename and different lines' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="app.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ <class filename="app.rb"><methods/><lines>
+ <line number="6" hits="1"/>
+ <line number="7" hits="1"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns a single file with merged coverage' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app.rb' => { 1 => 2, 2 => 0, 6 => 1, 7 => 1 } })
+ end
+ end
+
+ context 'with the same filename and lines' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="app.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ <class filename="app.rb"><methods/><lines>
+ <line number="1" hits="1"/>
+ <line number="2" hits="1"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns a single file with summed-up coverage' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app.rb' => { 1 => 3, 2 => 1 } })
+ end
+ end
+
+ context 'with missing filename' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="app.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ <class><methods/><lines>
+ <line number="6" hits="1"/>
+ <line number="7" hits="1"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and ignores class with missing name' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app.rb' => { 1 => 2, 2 => 0 } })
+ end
+ end
+
+ context 'with invalid line information' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="app.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ <class filename="app.rb"><methods/><lines>
+ <line null="test" hits="1"/>
+ <line number="7" hits="1"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'raises an error' do
+ expect { parse_report }.to raise_error(described_class::InvalidLineInformationError)
+ end
+ end
+ end
+ end
+
+ context 'when there is no <sources>' do
+ let(:sources_xml) { '' }
+
+ it_behaves_like 'ignoring sources, project_path, and worktree_paths'
+ end
+
+ context 'when there is a <sources>' do
+ context 'and has a single source with a pattern for Go projects' do
+ let(:project_path) { 'local/go' } # Make sure we're not making false positives
+ let(:sources_xml) do
+ <<~EOF
<sources>
- <source>project/src</source>
+ <source>/usr/local/go/src</source>
</sources>
EOF
end
- it_behaves_like 'ignoring sources'
+ it_behaves_like 'ignoring sources, project_path, and worktree_paths'
end
- context 'and has multiple sources' do
- let(:cobertura) do
- <<-EOF.strip_heredoc
+ context 'and has multiple sources with a pattern for Go projects' do
+ let(:project_path) { 'local/go' } # Make sure we're not making false positives
+ let(:sources_xml) do
+ <<~EOF
<sources>
- <source>project/src/foo</source>
- <source>project/src/bar</source>
+ <source>/usr/local/go/src</source>
+ <source>/go/src</source>
</sources>
EOF
end
- it_behaves_like 'ignoring sources'
+ it_behaves_like 'ignoring sources, project_path, and worktree_paths'
end
- end
- context 'when there is a single <class>' do
- context 'with no lines' do
- let(:cobertura) do
- <<-EOF.strip_heredoc
- <classes><class filename="app.rb"></class></classes>
+ context 'and has a single source but already is at the project root path' do
+ let(:sources_xml) do
+ <<~EOF
+ <sources>
+ <source>builds/#{project_path}</source>
+ </sources>
EOF
end
- it 'parses XML and returns empty coverage' do
- expect { subject }.not_to raise_error
+ it_behaves_like 'ignoring sources, project_path, and worktree_paths'
+ end
- expect(coverage_report.files).to eq({})
+ context 'and has multiple sources but already are at the project root path' do
+ let(:sources_xml) do
+ <<~EOF
+ <sources>
+ <source>builds/#{project_path}/</source>
+ <source>builds/somewhere/#{project_path}</source>
+ </sources>
+ EOF
end
+
+ it_behaves_like 'ignoring sources, project_path, and worktree_paths'
end
- context 'with a single line' do
- let(:cobertura) do
- <<-EOF.strip_heredoc
- <classes>
- <class filename="app.rb"><lines>
- <line number="1" hits="2"/>
- </lines></class>
- </classes>
+ context 'and has a single source that is not at the project root path' do
+ let(:sources_xml) do
+ <<~EOF
+ <sources>
+ <source>builds/#{project_path}/app</source>
+ </sources>
EOF
end
- it 'parses XML and returns a single file with coverage' do
- expect { subject }.not_to raise_error
+ context 'when there is no <class>' do
+ let(:classes_xml) { '' }
- expect(coverage_report.files).to eq({ 'app.rb' => { 1 => 2 } })
- end
- end
+ it 'parses XML and returns empty coverage' do
+ expect { parse_report }.not_to raise_error
- context 'with multipe lines and methods info' do
- let(:cobertura) do
- <<-EOF.strip_heredoc
- <classes>
- <class filename="app.rb"><methods/><lines>
- <line number="1" hits="2"/>
- <line number="2" hits="0"/>
- </lines></class>
- </classes>
- EOF
+ expect(coverage_report.files).to eq({})
+ end
end
- it 'parses XML and returns a single file with coverage' do
- expect { subject }.not_to raise_error
+ context 'when there is a single <class>' do
+ context 'with no lines' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="user.rb"></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns empty coverage' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({})
+ end
+ end
+
+ context 'with a single line but the filename cannot be determined based on extracted source and worktree paths' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="member.rb"><lines>
+ <line number="1" hits="2"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns empty coverage' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({})
+ end
+ end
+
+ context 'with a single line' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="user.rb"><lines>
+ <line number="1" hits="2"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns a single file with the filename relative to project root' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app/user.rb' => { 1 => 2 } })
+ end
+ end
+
+ context 'with multiple lines and methods info' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="user.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns a single file with the filename relative to project root' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app/user.rb' => { 1 => 2, 2 => 0 } })
+ end
+ end
+ end
- expect(coverage_report.files).to eq({ 'app.rb' => { 1 => 2, 2 => 0 } })
+ context 'when there are multiple <class>' do
+ context 'with the same filename but the filename cannot be determined based on extracted source and worktree paths' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="member.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ <class filename="member.rb"><methods/><lines>
+ <line number="6" hits="1"/>
+ <line number="7" hits="1"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns empty coverage' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({})
+ end
+ end
+
+ context 'without a parent package' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages>
+ <class filename="user.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ <class filename="user.rb"><methods/><lines>
+ <line number="6" hits="1"/>
+ <line number="7" hits="1"/>
+ </lines></class>
+ </packages>
+ EOF
+ end
+
+ it 'parses XML and returns coverage information with the filename relative to project root' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app/user.rb' => { 1 => 2, 2 => 0, 6 => 1, 7 => 1 } })
+ end
+ end
+
+ context 'with the same filename and different lines' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="user.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ <class filename="user.rb"><methods/><lines>
+ <line number="6" hits="1"/>
+ <line number="7" hits="1"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns a single file with merged coverage, and with the filename relative to project root' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app/user.rb' => { 1 => 2, 2 => 0, 6 => 1, 7 => 1 } })
+ end
+ end
+
+ context 'with the same filename and lines' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="user.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ <class filename="user.rb"><methods/><lines>
+ <line number="1" hits="1"/>
+ <line number="2" hits="1"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns a single file with summed-up coverage, and with the filename relative to project root' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app/user.rb' => { 1 => 3, 2 => 1 } })
+ end
+ end
+
+ context 'with missing filename' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="user.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ <class><methods/><lines>
+ <line number="6" hits="1"/>
+ <line number="7" hits="1"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and ignores class with missing name' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app/user.rb' => { 1 => 2, 2 => 0 } })
+ end
+ end
+
+ context 'with filename that cannot be determined based on extracted source and worktree paths' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="user.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ <class filename="member.rb"><methods/><lines>
+ <line number="6" hits="1"/>
+ <line number="7" hits="1"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and ignores class with undetermined filename' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app/user.rb' => { 1 => 2, 2 => 0 } })
+ end
+ end
+
+ context 'with invalid line information' do
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="user.rb"><methods/><lines>
+ <line number="1" hits="2"/>
+ <line number="2" hits="0"/>
+ </lines></class>
+ <class filename="user.rb"><methods/><lines>
+ <line null="test" hits="1"/>
+ <line number="7" hits="1"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'raises an error' do
+ expect { parse_report }.to raise_error(described_class::InvalidLineInformationError)
+ end
+ end
end
end
- end
- context 'when there are multipe <class>' do
- context 'with the same filename and different lines' do
- let(:cobertura) do
- <<-EOF.strip_heredoc
- <classes>
- <class filename="app.rb"><methods/><lines>
- <line number="1" hits="2"/>
- <line number="2" hits="0"/>
- </lines></class>
- <class filename="app.rb"><methods/><lines>
- <line number="6" hits="1"/>
- <line number="7" hits="1"/>
- </lines></class>
- </classes>
+ context 'and has multiple sources that are not at the project root path' do
+ let(:sources_xml) do
+ <<~EOF
+ <sources>
+ <source>builds/#{project_path}/app1/</source>
+ <source>builds/#{project_path}/app2/</source>
+ </sources>
EOF
end
- it 'parses XML and returns a single file with merged coverage' do
- expect { subject }.not_to raise_error
-
- expect(coverage_report.files).to eq({ 'app.rb' => { 1 => 2, 2 => 0, 6 => 1, 7 => 1 } })
+ context 'and a class filename is available under multiple extracted sources' do
+ let(:paths) { ['app1/user.rb', 'app2/user.rb'] }
+
+ let(:classes_xml) do
+ <<~EOF
+ <package name="app1">
+ <classes>
+ <class filename="user.rb"><lines>
+ <line number="1" hits="2"/>
+ </lines></class>
+ </classes>
+ </package>
+ <package name="app2">
+ <classes>
+ <class filename="user.rb"><lines>
+ <line number="2" hits="3"/>
+ </lines></class>
+ </classes>
+ </package>
+ EOF
+ end
+
+ it 'parses XML and returns the files with the filename relative to project root' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({
+ 'app1/user.rb' => { 1 => 2 },
+ 'app2/user.rb' => { 2 => 3 }
+ })
+ end
end
- end
- context 'with the same filename and lines' do
- let(:cobertura) do
- <<-EOF.strip_heredoc
- <packages><package><classes>
- <class filename="app.rb"><methods/><lines>
- <line number="1" hits="2"/>
- <line number="2" hits="0"/>
- </lines></class>
- <class filename="app.rb"><methods/><lines>
- <line number="1" hits="1"/>
- <line number="2" hits="1"/>
- </lines></class>
- </classes></package></packages>
- EOF
+ context 'and a class filename is available under one of the extracted sources' do
+ let(:paths) { ['app1/member.rb', 'app2/user.rb', 'app2/pet.rb'] }
+
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="user.rb"><lines>
+ <line number="1" hits="2"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns a single file with the filename relative to project root using the extracted source where it is first found under' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({ 'app2/user.rb' => { 1 => 2 } })
+ end
end
- it 'parses XML and returns a single file with summed-up coverage' do
- expect { subject }.not_to raise_error
+ context 'and a class filename is not found under any of the extracted sources' do
+ let(:paths) { ['app1/member.rb', 'app2/pet.rb'] }
- expect(coverage_report.files).to eq({ 'app.rb' => { 1 => 3, 2 => 1 } })
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="user.rb"><lines>
+ <line number="1" hits="2"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ it 'parses XML and returns empty coverage' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({})
+ end
end
- end
- context 'with missing filename' do
- let(:cobertura) do
- <<-EOF.strip_heredoc
- <classes>
- <class filename="app.rb"><methods/><lines>
- <line number="1" hits="2"/>
- <line number="2" hits="0"/>
- </lines></class>
- <class><methods/><lines>
- <line number="6" hits="1"/>
- <line number="7" hits="1"/>
- </lines></class>
- </classes>
- EOF
+ context 'and a class filename is not found under any of the extracted sources within the iteratable limit' do
+ let(:paths) { ['app2/user.rb'] }
+
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="record.rb"><lines>
+ <line number="1" hits="2"/>
+ </lines></class>
+ <class filename="user.rb"><lines>
+ <line number="1" hits="2"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
+ end
+
+ before do
+ stub_const("#{described_class}::MAX_SOURCES", 1)
+ end
+
+ it 'parses XML and returns empty coverage' do
+ expect { parse_report }.not_to raise_error
+
+ expect(coverage_report.files).to eq({})
+ end
end
+ end
+ end
- it 'parses XML and ignores class with missing name' do
- expect { subject }.not_to raise_error
+ shared_examples_for 'non-smart parsing' do
+ let(:sources_xml) do
+ <<~EOF
+ <sources>
+ <source>builds/foo/bar/app</source>
+ </sources>
+ EOF
+ end
- expect(coverage_report.files).to eq({ 'app.rb' => { 1 => 2, 2 => 0 } })
- end
+ let(:classes_xml) do
+ <<~EOF
+ <packages><package name="app"><classes>
+ <class filename="user.rb"><lines>
+ <line number="1" hits="2"/>
+ </lines></class>
+ </classes></package></packages>
+ EOF
end
- context 'with invalid line information' do
- let(:cobertura) do
- <<-EOF.strip_heredoc
- <classes>
- <class filename="app.rb"><methods/><lines>
- <line number="1" hits="2"/>
- <line number="2" hits="0"/>
- </lines></class>
- <class filename="app.rb"><methods/><lines>
- <line null="test" hits="1"/>
- <line number="7" hits="1"/>
- </lines></class>
- </classes>
- EOF
- end
+ it 'parses XML and returns filenames unchanged just as how they are found in the class node' do
+ expect { parse_report }.not_to raise_error
- it 'raises an error' do
- expect { subject }.to raise_error(described_class::CoberturaParserError)
- end
+ expect(coverage_report.files).to eq({ 'user.rb' => { 1 => 2 } })
end
end
+
+ context 'when project_path is not present' do
+ let(:project_path) { nil }
+ let(:paths) { ['app/user.rb'] }
+
+ it_behaves_like 'non-smart parsing'
+ end
+
+ context 'when worktree_paths is not present' do
+ let(:project_path) { 'foo/bar' }
+ let(:paths) { nil }
+
+ it_behaves_like 'non-smart parsing'
+ end
end
context 'when data is not Cobertura style XML' do
let(:cobertura) { { coverage: '12%' }.to_json }
it 'raises an error' do
- expect { subject }.to raise_error(described_class::CoberturaParserError)
+ expect { parse_report }.to raise_error(described_class::InvalidXMLError)
end
end
end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 0efb014fdfcb138358e3f53a3220dbdc20659f13..ade78efe23984446f0aceea991093f64756a94d9 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -4059,13 +4059,40 @@ def run_job_without_exception
end
end
+ context 'when there is a Cobertura coverage report with class filename paths not relative to project root' do
+ before do
+ allow(build.project).to receive(:full_path).and_return('root/javademo')
+ allow(build.pipeline).to receive(:all_worktree_paths).and_return(['src/main/java/com/example/javademo/User.java'])
+
+ create(:ci_job_artifact, :coverage_with_paths_not_relative_to_project_root, job: build, project: build.project)
+ end
+
+ it 'parses blobs and add the results to the coverage report with corrected paths' do
+ expect { subject }.not_to raise_error
+
+ expect(coverage_report.files.keys).to match_array(['src/main/java/com/example/javademo/User.java'])
+ end
+
+ context 'and smart_cobertura_parser feature flag is disabled' do
+ before do
+ stub_feature_flags(smart_cobertura_parser: false)
+ end
+
+ it 'parses blobs and add the results to the coverage report with unmodified paths' do
+ expect { subject }.not_to raise_error
+
+ expect(coverage_report.files.keys).to match_array(['com/example/javademo/User.java'])
+ end
+ end
+ end
+
context 'when there is a corrupted Cobertura coverage report' do
before do
create(:ci_job_artifact, :coverage_with_corrupted_data, job: build, project: build.project)
end
it 'raises an error' do
- expect { subject }.to raise_error(Gitlab::Ci::Parsers::Coverage::Cobertura::CoberturaParserError)
+ expect { subject }.to raise_error(Gitlab::Ci::Parsers::Coverage::Cobertura::InvalidLineInformationError)
end
end
end
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 05387f06746bc5265ea89b6baaaa269f43fc764c..f50307ba0432011b2e382880a77bb66ae6f14c47 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -3426,6 +3426,16 @@ def create_build(name, stage_idx)
])
end
+ it 'does not execute N+1 queries' do
+ single_build_pipeline = create(:ci_empty_pipeline, status: :created, project: project)
+ single_rspec = create(:ci_build, :success, name: 'rspec', pipeline: single_build_pipeline, project: project)
+ create(:ci_job_artifact, :cobertura, job: single_rspec, project: project)
+
+ control = ActiveRecord::QueryRecorder.new { single_build_pipeline.coverage_reports }
+
+ expect { subject }.not_to exceed_query_limit(control)
+ end
+
context 'when builds are retried' do
let!(:build_rspec) { create(:ci_build, :retried, :success, name: 'rspec', pipeline: pipeline, project: project) }
let!(:build_golang) { create(:ci_build, :retried, :success, name: 'golang', pipeline: pipeline, project: project) }
diff --git a/spec/services/ci/pipelines/create_artifact_service_spec.rb b/spec/services/ci/pipelines/create_artifact_service_spec.rb
index 6f177889ed3f8b79706fb63ed6eb24c78ee8c1fd..4e9248d9d1a2837cda1ea6c767e1ea0bb936ef30 100644
--- a/spec/services/ci/pipelines/create_artifact_service_spec.rb
+++ b/spec/services/ci/pipelines/create_artifact_service_spec.rb
@@ -7,7 +7,8 @@
subject { described_class.new.execute(pipeline) }
context 'when pipeline has coverage reports' do
- let(:pipeline) { create(:ci_pipeline, :with_coverage_reports) }
+ let(:project) { create(:project, :repository) }
+ let(:pipeline) { create(:ci_pipeline, :with_coverage_reports, project: project) }
context 'when pipeline is finished' do
it 'creates a pipeline artifact' do