From 8d5f1d4d1425ac02b0925646ee3020b170c481cb Mon Sep 17 00:00:00 2001 From: Jeanine Soterwood Date: Mon, 20 May 2024 09:44:13 -0700 Subject: [PATCH] Format forcing does not bypass valid file check (#125) When forcing a format, the system still collects all possible coverage files across all file types, and passes them directly to the specified parser, bypassing the parser's matches? method that checks to make sure that the file is of a valid format. This is causing issues when a user is forcing the cobertura format, but has a .coverage file without the coverage executable installed. These changes ensure that if a user specifies a format, then only the files that match the glob-pattern for the specified parser will be passed to the parser. In addition, the matches? method is no longer bypassed, which ensures that even if a file matches the correct naming pattern, there will be no attempt to parse it if the actual file format is incorrect. If a user specifies the python format for a .coverage file, but does not have the coverage executable installed, then an error will be raised. If no format is specified, and the user has a .coverage file but does not have the coverage executable installed, then a debug warning message only will be displayed. --- spec/coverage_reporter/parser_spec.cr | 66 +++++++++++++++++-- .../parsers/coveragepy_parser_spec.cr | 33 +++++++--- .../parsers/lcov_parser_spec.cr | 2 +- ...{for-base-path-lcov => for-base-path.lcov} | 0 src/coverage_reporter/parser.cr | 28 ++++---- src/coverage_reporter/parsers/base_parser.cr | 2 +- .../parsers/cobertura_parser.cr | 5 -- .../parsers/coveragepy_parser.cr | 33 ++++++---- .../parsers/coveralls_parser.cr | 3 - src/coverage_reporter/parsers/lcov_parser.cr | 4 -- 10 files changed, 118 insertions(+), 58 deletions(-) rename spec/fixtures/lcov/{for-base-path-lcov => for-base-path.lcov} (100%) diff --git a/spec/coverage_reporter/parser_spec.cr b/spec/coverage_reporter/parser_spec.cr index b5760156..88a1bcc2 100644 --- a/spec/coverage_reporter/parser_spec.cr +++ b/spec/coverage_reporter/parser_spec.cr @@ -46,7 +46,7 @@ Spectator.describe CoverageReporter::Parser do end context "when a file is specified" do - let(coverage_files) { ["spec/fixtures/lcov/for-base-path-lcov"] } + let(coverage_files) { ["spec/fixtures/lcov/for-base-path.lcov"] } let(base_path) { "spec/fixtures/lcov" } it "returns report only for specified file" do @@ -83,9 +83,9 @@ Spectator.describe CoverageReporter::Parser do context "when a file is specified and coverage is not installed" do let(coverage_files) { ["spec/fixtures/python/.coverage"] } - let(base_path) { "spec/fixtures/lcov" } + let(base_path) { "spec/fixtures/python" } - it "returns report only for specified file" do + it "raises error" do path = ENV["PATH"] ENV.delete("PATH") @@ -107,10 +107,64 @@ Spectator.describe CoverageReporter::Parser do end context "for all files" do - it "returns reports for all files" do - reports = subject.parse + context "when coveragepy is installed" do + it "returns reports for all files" do + reports = subject.parse + + expect(reports.size).to be > 2 + end + end + + context "when coveragepy is not installed" do + it "returns reports for all files (no error is raised)" do + path = ENV["PATH"] + ENV.delete("PATH") + + reports = subject.parse + + expect(reports.size).to be > 2 + + ENV["PATH"] = path + end + end + end + end + + describe "#files" do + context "when no coverage_format specified" do + it "returns all possible files across all formats" do + files = subject.files + + expect(files).to match_array [ + "spec/fixtures/lcov/coverage/test.lcov", + "spec/fixtures/lcov/test.lcov", + "spec/fixtures/lcov/test-current-folder.lcov", + "spec/fixtures/lcov/empty.lcov", + "spec/fixtures/lcov/for-base-path.lcov", + "spec/fixtures/simplecov/.resultset.json", + "spec/fixtures/clover/clover.xml", + "spec/fixtures/cobertura/cobertura.xml", + "spec/fixtures/cobertura/cobertura-coverage.xml", + "spec/fixtures/jacoco/jacoco-oneline-report.xml", + "spec/fixtures/jacoco/jacoco-report-multiple-packages.xml", + "spec/fixtures/jacoco/jacoco-report.xml", + "spec/fixtures/gcov/main.c.gcov", + "spec/fixtures/python/.coverage", + "spec/fixtures/coveralls/coveralls.json", + ] + end + end + + context "when coverage_format specified" do + let(coverage_format) { "cobertura" } + + it "only returns possible files for the specified format" do + files = subject.files - expect(reports.size).to be > 2 + expect(files).to match_array [ + "spec/fixtures/cobertura/cobertura.xml", + "spec/fixtures/cobertura/cobertura-coverage.xml", + ] end end end diff --git a/spec/coverage_reporter/parsers/coveragepy_parser_spec.cr b/spec/coverage_reporter/parsers/coveragepy_parser_spec.cr index 1cd52764..0e4a9474 100644 --- a/spec/coverage_reporter/parsers/coveragepy_parser_spec.cr +++ b/spec/coverage_reporter/parsers/coveragepy_parser_spec.cr @@ -4,19 +4,34 @@ Spectator.describe CoverageReporter::CoveragepyParser do subject { described_class.new(nil) } describe "#matches?" do - it "matches only SQLite3 db file" do - expect(subject.matches?("spec/fixtures/python/.coverage")).to eq true - expect(subject.matches?("spec/fixtures/python/.coverage-non-existing")).to eq false - expect(subject.matches?("spec/fixtures/golang/coverage.out")).to eq false + context "when format is not forced" do + it "matches only SQLite3 db file" do + expect(subject.matches?("spec/fixtures/python/.coverage")).to eq true + expect(subject.matches?("spec/fixtures/python/.coverage-non-existing")).to eq false + expect(subject.matches?("spec/fixtures/golang/coverage.out")).to eq false + end + + it "does not match if coverage program is not installed" do + path = ENV["PATH"] + ENV.delete("PATH") + + expect(subject.matches?("spec/fixtures/python/.coverage")).to be_falsey + + ENV["PATH"] = path + end end - it "does not match if coverage program is not installed" do - path = ENV["PATH"] - ENV.delete("PATH") + context "when format is forced" do + subject { described_class.new(nil, true) } + + it "raises error if coverage program is not installed" do + path = ENV["PATH"] + ENV.delete("PATH") - expect(subject.matches?("spec/fixtures/python/.coverage")).to be_falsey + expect { subject.matches?("spec/fixtures/python/.coverage") }.to raise_error(CoverageReporter::CoveragepyParser::ParserError) - ENV["PATH"] = path + ENV["PATH"] = path + end end end diff --git a/spec/coverage_reporter/parsers/lcov_parser_spec.cr b/spec/coverage_reporter/parsers/lcov_parser_spec.cr index 881b7803..b89b47e3 100644 --- a/spec/coverage_reporter/parsers/lcov_parser_spec.cr +++ b/spec/coverage_reporter/parsers/lcov_parser_spec.cr @@ -61,7 +61,7 @@ Spectator.describe CoverageReporter::LcovParser do end context "with base path" do - let(filename) { "spec/fixtures/lcov/for-base-path-lcov" } + let(filename) { "spec/fixtures/lcov/for-base-path.lcov" } let(base_path) { "spec/fixtures/lcov" } it "parses correctly" do diff --git a/spec/fixtures/lcov/for-base-path-lcov b/spec/fixtures/lcov/for-base-path.lcov similarity index 100% rename from spec/fixtures/lcov/for-base-path-lcov rename to spec/fixtures/lcov/for-base-path.lcov diff --git a/src/coverage_reporter/parser.cr b/src/coverage_reporter/parser.cr index 6956603c..8d2ea58d 100644 --- a/src/coverage_reporter/parser.cr +++ b/src/coverage_reporter/parser.cr @@ -49,7 +49,17 @@ module CoverageReporter end def initialize(@coverage_files : Array(String) | Nil, @coverage_format : String?, @base_path : String?) - @parsers = PARSERS.map(&.new(@base_path)).to_a + @parsers = if coverage_format + Log.info("✏️ Forced coverage format: #{coverage_format}") + parser_class = PARSERS.find { |klass| klass.name == coverage_format } + if parser_class + [parser_class.new(base_path, true)] + else + raise InvalidCoverageFormat.new(coverage_format) + end + else + PARSERS.map(&.new(base_path)).to_a + end end # Returns coverage report files that can be parsed by utility. @@ -79,22 +89,6 @@ module CoverageReporter end def parse : SourceFiles - if coverage_format - Log.info("✏️ Forced coverage format: #{coverage_format}") - parser_class = PARSERS.find { |klass| klass.name == coverage_format } - if parser_class - parser = parser_class.new(base_path) - source_files = SourceFiles.new - files.each do |filename| - source_files.add(parser.parse(filename), filename) - end - - return source_files - else - raise InvalidCoverageFormat.new(coverage_format) - end - end - source_files = SourceFiles.new files.each do |filename| source_files.add(parse_file(filename), filename) diff --git a/src/coverage_reporter/parsers/base_parser.cr b/src/coverage_reporter/parsers/base_parser.cr index f1c552b7..43399879 100644 --- a/src/coverage_reporter/parsers/base_parser.cr +++ b/src/coverage_reporter/parsers/base_parser.cr @@ -45,7 +45,7 @@ module CoverageReporter # # *base_path* can be used to join with all paths in coverage report in order # to properly reference a file. - def initialize(@base_path : String? = nil) + def initialize(@base_path : String? = nil, @forced : Bool = false) end # Creates a `FileReport` instance. Use this method instead of explicit diff --git a/src/coverage_reporter/parsers/cobertura_parser.cr b/src/coverage_reporter/parsers/cobertura_parser.cr index 537a96a4..999c95f6 100644 --- a/src/coverage_reporter/parsers/cobertura_parser.cr +++ b/src/coverage_reporter/parsers/cobertura_parser.cr @@ -7,11 +7,6 @@ module CoverageReporter coverage : Hash(Line, Hits?), branches : Hash(Line, Array(Hits)) - # NOTE: Provide the base path for the sources. You can check "filename" in - # coverage report and see what part is missing to get a valid source path. - def initialize(@base_path : String?) - end - def globs : Array(String) [ "**/*/cobertura.xml", diff --git a/src/coverage_reporter/parsers/coveragepy_parser.cr b/src/coverage_reporter/parsers/coveragepy_parser.cr index fa06d84e..7383a25b 100644 --- a/src/coverage_reporter/parsers/coveragepy_parser.cr +++ b/src/coverage_reporter/parsers/coveragepy_parser.cr @@ -25,6 +25,8 @@ module CoverageReporter end valid_file_exists && check_for_coverage_executable + rescue error : ParserError + raise error rescue Exception false end @@ -41,17 +43,8 @@ module CoverageReporter parser = CoberturaParser.new(@base_path) parser.parse(tmpfile.path) else - error_message = - %Q|There was an error processing #{filename}: #{error} + error_message = "There was an error processing #{filename}: #{error}\n\n#{missing_coverage_executable_message}" -To use the #{self.class.name} format, do one of the following: -1. Make sure that the coverage executable is available in the - runner environment, or -2. Convert the .coverage file to a coverage.xml file by running - `coverage xml`. Then pass the input option `format: cobertura` - (for Coveralls GitHub Action or orb), or pass `--format=cobertura` - if using the coverage reporter alone. -| raise ParserError.new(error_message) end ensure @@ -72,9 +65,25 @@ To use the #{self.class.name} format, do one of the following: if process_status.success? true else - Log.debug("⚠️ Detected coverage format: #{self.class.name}, but error with coverage executable: #{error}") - false + if @forced + raise ParserError.new(missing_coverage_executable_message) + else + Log.debug("⚠️ Detected coverage format: #{self.class.name}, but error with coverage executable: #{error}") + Log.debug(missing_coverage_executable_message) + false + end end end + + private def missing_coverage_executable_message + %Q|To use the #{self.class.name} format, do one of the following: +1. Make sure that the coverage executable is available in the + runner environment, or +2. Convert the .coverage file to a coverage.xml file by running + `coverage xml`. Then pass the input option `format: cobertura` + (for Coveralls GitHub Action or orb), or pass `--format=cobertura` + if using the coverage reporter alone. +| + end end end diff --git a/src/coverage_reporter/parsers/coveralls_parser.cr b/src/coverage_reporter/parsers/coveralls_parser.cr index 1d9fa4a8..866d885b 100644 --- a/src/coverage_reporter/parsers/coveralls_parser.cr +++ b/src/coverage_reporter/parsers/coveralls_parser.cr @@ -17,9 +17,6 @@ module CoverageReporter property source_files : Array(SourceFiles) end - def initialize(@base_path : String?) - end - def globs : Array(String) [ "coveralls.json", diff --git a/src/coverage_reporter/parsers/lcov_parser.cr b/src/coverage_reporter/parsers/lcov_parser.cr index 2e5d46db..c04aa40f 100644 --- a/src/coverage_reporter/parsers/lcov_parser.cr +++ b/src/coverage_reporter/parsers/lcov_parser.cr @@ -13,10 +13,6 @@ module CoverageReporter BRANCH_COVERAGE_RE = Regex.new("\\ABRDA:(\\d+),(\\d+),(\\d+),(\\d+|-)", Regex::CompileOptions::MATCH_INVALID_UTF) END_RE = Regex.new("\\Aend_of_record", Regex::CompileOptions::MATCH_INVALID_UTF) - # Use *base_path* to join with paths found in reports. - def initialize(@base_path : String?) - end - def globs : Array(String) [ "*.lcov",