From dc953ea254254bd286ca07115f0b763647bf29a0 Mon Sep 17 00:00:00 2001 From: Dan Mayer Date: Fri, 3 Jul 2020 15:51:27 -0600 Subject: [PATCH] a production usable file adapter following cookpads style I took some ideas from our previous logger, and from cookpads logger https://github.com/riseshia/oneshot_coverage/blob/master/lib/oneshot_coverage/logger/file_logger.rb I resolved issues like multiprocess by running in coverband_demo and tested both regular and one-shot. I think we could further improve the perf of one-shot for this adapter, but this is a fully functional versions that we could perhaps have them try to use. --- lib/coverband.rb | 1 + lib/coverband/adapters/base.rb | 1 - lib/coverband/adapters/file_store.rb | 44 +++++++++++++++++++--- lib/coverband/adapters/stdout_store.rb | 41 ++++++++++++++++++++ lib/coverband/collectors/delta.rb | 10 +++++ lib/coverband/utils/tasks.rb | 1 + test/coverband/adapters/file_store_test.rb | 12 +++--- test/coverband/collectors/delta_test.rb | 1 + 8 files changed, 98 insertions(+), 13 deletions(-) create mode 100644 lib/coverband/adapters/stdout_store.rb diff --git a/lib/coverband.rb b/lib/coverband.rb index 22c596f8..d449271d 100644 --- a/lib/coverband.rb +++ b/lib/coverband.rb @@ -12,6 +12,7 @@ require "coverband/adapters/redis_store" require "coverband/adapters/hash_redis_store" require "coverband/adapters/file_store" +require "coverband/adapters/stdout_store" require "coverband/utils/file_hasher" require "coverband/utils/html_formatter" require "coverband/utils/result" diff --git a/lib/coverband/adapters/base.rb b/lib/coverband/adapters/base.rb index 58ad6702..1705fb74 100644 --- a/lib/coverband/adapters/base.rb +++ b/lib/coverband/adapters/base.rb @@ -112,7 +112,6 @@ def merge_reports(new_report, old_report, options = {}) # transparently update from RUNTIME_TYPE = nil to RUNTIME_TYPE = :runtime # transparent update for format coveband_3_2 old_report = coverage(nil, override_type: nil) if old_report.nil? && type == Coverband::RUNTIME_TYPE - new_report = expand_report(new_report) unless options[:skip_expansion] keys = (new_report.keys + old_report.keys).uniq keys.each do |file| diff --git a/lib/coverband/adapters/file_store.rb b/lib/coverband/adapters/file_store.rb index 0eaf8101..c3030e25 100644 --- a/lib/coverband/adapters/file_store.rb +++ b/lib/coverband/adapters/file_store.rb @@ -3,14 +3,38 @@ module Coverband module Adapters ### - # FilesStore store a merged coverage file to local disk - # Generally this is for testing and development - # Not recommended for production deployment, as it doesn't handle concurrency + # FileStore store a merged coverage file to local disk + # + # Notes: Concurrency + # * threadsafe as the caller to save_report uses @semaphore.synchronize + # * file access process safe as each file written per process PID + # + # Usage: + # config.store = Coverband::Adapters::FileStore.new('log/coverage.log') + # + # View Reports: + # Using this assumes you are syncing the coverage files + # to some shared storage that is accessable outside of the production server + # download files to a system where you want to view the reports.. + # When viewing coverage from the filestore adapter it merges all coverage + # files matching the path pattern, in this case `log/coverage.log.*` + # + # run: `bundle exec rake coverband:coverage_server` + # open http://localhost:1022/ + # + # one could also build a report via code, the output is suitable to feed into SimpleCov + # + # ``` + # coverband.configuration.store.merge_mode = true + # coverband.configuration.store.coverage + # ``` ### class FileStore < Base + attr_accessor :merge_mode def initialize(path, _opts = {}) super() - @path = path + @path = "#{path}.#{::Process.pid}" + @merge_mode = false config_dir = File.dirname(@path) Dir.mkdir config_dir unless File.exist?(config_dir) @@ -29,17 +53,25 @@ def migrate! end def coverage(_local_type = nil) - if File.exist?(path) + if merge_mode + data = {} + Dir[path.sub(/\.\d+/, ".*")].each do |path| + data = merge_reports(data, JSON.parse(File.read(path)), skip_expansion: true) + end + data + elsif File.exist?(path) JSON.parse(File.read(path)) else {} end + rescue Errno::ENOENT + {} end def save_report(report) data = report.dup data = merge_reports(data, coverage) - File.open(path, "w") { |f| f.write(data.to_json) } + File.write(path, JSON.dump(data)) end def raw_store diff --git a/lib/coverband/adapters/stdout_store.rb b/lib/coverband/adapters/stdout_store.rb new file mode 100644 index 00000000..3f4d9d68 --- /dev/null +++ b/lib/coverband/adapters/stdout_store.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Coverband + module Adapters + ### + # StdoutStore is for testing and development + # + # Usage: + # config.store = Coverband::Adapters::StdoutStore.new + ### + class StdoutStore < Base + def initialize(_opts = {}) + super() + end + + def clear! + # NOOP + end + + def size + 0 + end + + def migrate! + raise NotImplementedError, "StdoutStore doesn't support migrations" + end + + def coverage(_local_type = nil) + {} + end + + def save_report(report) + $stdout.puts(report.to_json) + end + + def raw_store + raise NotImplementedError, "StdoutStore doesn't support raw_store" + end + end + end +end diff --git a/lib/coverband/collectors/delta.rb b/lib/coverband/collectors/delta.rb index 566f229d..0b957b55 100644 --- a/lib/coverband/collectors/delta.rb +++ b/lib/coverband/collectors/delta.rb @@ -77,6 +77,16 @@ def array_diff(latest, original) def transform_oneshot_lines_results(results) results.each_with_object({}) do |(file, coverage), new_results| + ### + # Eager filter: + # Normally I would break this out into additional methods + # and improve the readability but this is in a tight loop + # on the critical performance path, and any refactoring I come up with + # would slow down the performance. + ### + next unless @@ignore_patterns.none? { |pattern| file.match(pattern) } && + file.start_with?(@@project_directory) + @@stubs[file] ||= ::Coverage.line_stub(file) transformed_line_counts = coverage[:oneshot_lines].each_with_object(@@stubs[file].dup) { |line_number, line_counts| line_counts[line_number - 1] = 1 diff --git a/lib/coverband/utils/tasks.rb b/lib/coverband/utils/tasks.rb index 19a072f7..6c72f9a0 100644 --- a/lib/coverband/utils/tasks.rb +++ b/lib/coverband/utils/tasks.rb @@ -12,6 +12,7 @@ desc "report runtime Coverband code coverage" task :coverage_server do Rake.application["environment"].invoke if Rake::Task.task_defined?("environment") + Coverband.configuration.store.merge_mode = true if Coverband.configuration.store.is_a?(Coverband::Adapters::FileStore) Rack::Server.start app: Coverband::Reporters::Web.new, Port: ENV.fetch("COVERBAND_COVERAGE_PORT", 1022).to_i end diff --git a/test/coverband/adapters/file_store_test.rb b/test/coverband/adapters/file_store_test.rb index 6f03c709..764b3359 100644 --- a/test/coverband/adapters/file_store_test.rb +++ b/test/coverband/adapters/file_store_test.rb @@ -13,14 +13,13 @@ def test_covered_lines_when_no_file def setup super @test_file_path = "/tmp/coverband_filestore_test_path.json" - File.open(@test_file_path, "w") { |f| f.write(test_data.to_json) } + previous_file_path = "#{@test_file_path}.#{::Process.pid}" + `rm #{@test_file_path}` if File.exist?(@test_file_path) + `rm #{previous_file_path}` if File.exist?(previous_file_path) + File.open(previous_file_path, "w") { |f| f.write(test_data.to_json) } @store = Coverband::Adapters::FileStore.new(@test_file_path) end - def test_size - assert @store.size > 1 - end - def test_coverage assert_equal @store.coverage["dog.rb"]["data"][0], 1 assert_equal @store.coverage["dog.rb"]["data"][1], 2 @@ -31,7 +30,7 @@ def test_covered_lines_when_null end def test_covered_files - assert_equal @store.covered_files, ["dog.rb"] + assert @store.covered_files.include?("dog.rb") end def test_clear @@ -43,6 +42,7 @@ def test_save_report mock_file_hash @store.send(:save_report, "cat.rb" => [0, 1]) assert_equal @store.coverage["cat.rb"]["data"][1], 1 + assert @store.size > 1 end def test_data diff --git a/test/coverband/collectors/delta_test.rb b/test/coverband/collectors/delta_test.rb index 2d76c6ed..e83d43f3 100644 --- a/test/coverband/collectors/delta_test.rb +++ b/test/coverband/collectors/delta_test.rb @@ -114,6 +114,7 @@ def setup oneshot_lines: [2, 3] } } + Coverband::Collectors::Delta.class_variable_set(:@@project_directory, "dealership.rb") ::Coverage.expects(:line_stub).with("dealership.rb").returns([nil, 0, 0, nil]) results = Coverband::Collectors::Delta.results(mock_coverage(current_coverage)) expected = {