Merge pull request #390 from danmayer/improved_config

cleanup configuration options, ensure all env options can be set via …
danmayer · Aug 24, 2020 · 4b2b8a4 · 4b2b8a4
2 parents 0d82c5c + d1a10ef
commit 4b2b8a4
Show file tree

Hide file tree

Showing 10 changed files with 119 additions and 61 deletions.
diff --git a/.standard.yml b/.standard.yml
@@ -13,6 +13,7 @@ ignore: # default: []
       - Style/RedundantRegexpEscape # fix later, enforcement changed
       - Layout/ArrayAlignment # WTF all of master broken from a few changes in rubo
       - Performance/RegexpMatch # Rubocop / standardrb have this WRONG for Ruby 2.3/2.4 not compatiable
+      - Style/GlobalStdStream # Rubocop / standardrb have this WRONG for Ruby 2.3/2.4 not compatiable
   - "vendor/**/*"
   - "pkg/**/*"
   - "test/**/*":

diff --git a/changes.md b/changes.md
@@ -61,7 +61,11 @@ Will be the fully modern release that drops maintenance legacy support in favor
   - drops static report support
   - drops gem support
   - only loaded web reporter files when required
-- improved load order allowing more time for ENV vars (better dotenv, figaro, rails secrets support)
+- configuration improvements
+	- improved load order allowing more time for ENV vars (better dotenv, figaro, rails secrets support)
+	- all config options can be set via coverband config, not requiring ENV var support
+	- deprecation notices on soon to be removed config options
+	- config exceptions on invalid configuration combinations
 - improved resque patching pattern
 - improved default ignores
 - additional adapters

diff --git a/coverband.gemspec b/coverband.gemspec
@@ -30,6 +30,7 @@ Gem::Specification.new do |spec|
   spec.add_development_dependency "rack-test"
   spec.add_development_dependency "rake"
   spec.add_development_dependency "resque"
+  spec.add_development_dependency "standard", "= 0.2.5"
   spec.add_development_dependency "standardrb"
 
   spec.add_development_dependency "coveralls"

diff --git a/lib/coverband/adapters/web_service_store.rb b/lib/coverband/adapters/web_service_store.rb
@@ -3,8 +3,7 @@
 module Coverband
   module Adapters
     ###
-    # WebServiceStore store a merged coverage file to local disk
-    # TODO: add webmock tests
+    # WebServiceStore: store a checkpoint of coverage to a remote service
     ###
     class WebServiceStore < Base
       attr_reader :coverband_url, :process_type, :runtime_env, :hostname, :pid
@@ -56,8 +55,7 @@ def coverage(local_type = nil, opts = {})
         res = Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == "https") do |http|
           http.request(req)
         end
-        coverage_data = JSON.parse(res.body)
-        coverage_data
+        JSON.parse(res.body)
       rescue => e
         logger&.error "Coverband: Error while retrieving coverage #{e}" if Coverband.configuration.verbose || Coverband.configuration.service_dev_mode
       end

diff --git a/lib/coverband/configuration.rb b/lib/coverband/configuration.rb
@@ -3,15 +3,15 @@
 module Coverband
   class Configuration
     attr_accessor :root_paths, :root,
-      :additional_files, :verbose,
+      :verbose,
       :reporter, :redis_namespace, :redis_ttl,
       :background_reporting_enabled,
       :test_env, :web_enable_clear, :gem_details, :web_debug, :report_on_exit,
       :simulate_oneshot_lines_coverage,
       :view_tracker
     attr_writer :logger, :s3_region, :s3_bucket, :s3_access_key_id,
       :s3_secret_access_key, :password, :api_key, :service_url, :coverband_timeout, :service_dev_mode,
-      :service_test_mode, :proces_type, :report_period, :track_views,
+      :service_test_mode, :process_type, :track_views, :redis_url,
       :background_reporting_sleep_seconds, :reporting_wiggle
 
     attr_reader :track_gems, :ignore, :use_oneshot_lines_coverage
@@ -20,14 +20,14 @@ class Configuration
     # TODO: This is is brittle and not a great solution to avoid deploy time
     # actions polluting the 'runtime' metrics
     #
-    # * should we skip /bin/rails webpacker:compile ?
     # * Perhaps detect heroku deployment ENV var opposed to tasks?
     #####
     IGNORE_TASKS = ["coverband:clear",
                     "coverband:coverage",
                     "coverband:coverage_server",
                     "coverband:migrate",
                     "assets:precompile",
+                    "webpacker:compile",
                     "db:version",
                     "db:create",
                     "db:drop",
@@ -56,7 +56,6 @@ def reset
       @root_paths = []
       @ignore = IGNORE_DEFAULTS.dup
       @search_paths = TRACKED_DEFAULT_PATHS.dup
-      @additional_files = []
       @verbose = false
       @reporter = "scov"
       @logger = nil
@@ -65,8 +64,6 @@ def reset
       @background_reporting_sleep_seconds = nil
       @test_env = nil
       @web_enable_clear = false
-      @track_gems = false
-      @gem_details = false
       @track_views = true
       @view_tracker = nil
       @web_debug = false
@@ -85,17 +82,19 @@ def reset
       @service_dev_mode = nil
       @service_test_mode = nil
       @proces_type = nil
-      @report_period = nil
+
+      @redis_url = nil
+      @redis_namespace = nil
+      @redis_ttl = 2_592_000 # in seconds. Default is 30 days.
+      @reporting_wiggle = nil
 
       # TODO: these are deprecated
       @s3_region = nil
       @s3_bucket = nil
       @s3_access_key_id = nil
       @s3_secret_access_key = nil
-
-      @redis_namespace = nil
-      @redis_ttl = 2_592_000 # in seconds. Default is 30 days.
-      @reporting_wiggle = nil
+      @track_gems = false
+      @gem_details = false
     end
 
     def logger
@@ -110,25 +109,16 @@ def password
       @password || ENV["COVERBAND_PASSWORD"]
     end
 
-    def s3_bucket
-      puts "deprecated, s3 is no longer support"
-    end
-
-    def s3_region
-      puts "deprecated, s3 is no longer support"
-    end
-
-    def s3_access_key_id
-      puts "deprecated, s3 is no longer support"
-    end
-
-    def s3_secret_access_key
-      puts "deprecated, s3 is no longer support"
-    end
-
+    # The adjustments here either protect the redis or service from being overloaded
+    # the tradeoff being the delay in when reporting data is available
+    # if running your own redis increasing this number reduces load on the redis CPU
     def background_reporting_sleep_seconds
-      @background_reporting_sleep_seconds ||= if Coverband.coverband_service?
-        Coverband.configuration.coverband_env == "production" ? Coverband.configuration.report_period : 60
+      @background_reporting_sleep_seconds ||= if service?
+        # default to 10m for service
+        Coverband.configuration.coverband_env == "production" ? 600 : 60
+      elsif store.is_a?(Coverband::Adapters::HashRedisStore)
+        # Default to 5 minutes if using the hash redis store
+        300
       else
         60
       end
@@ -139,29 +129,27 @@ def reporting_wiggle
     end
 
     def store
-      @store ||= if Coverband.coverband_service?
+      @store ||= if service?
+        raise "invalid configuration: unclear default store coverband expects either api_key or redis_url" if redis_url
         require "coverband/adapters/web_service_store"
         Coverband::Adapters::WebServiceStore.new(service_url)
       else
         Coverband::Adapters::RedisStore.new(Redis.new(url: redis_url), redis_store_options)
       end
     end
 
-    def track_views
-      @track_views ||= service_disabled_dev_test_env? ? false : true
-    end
-
     def store=(store)
       raise "Pass in an instance of Coverband::Adapters" unless store.is_a?(Coverband::Adapters::Base)
-
-      # Default to 5 minutes if using the hash redis store
-      # This is a safer default for the high server volumes that need the hash store
-      # it should avoid overloading the redis with lots of load
-      @background_reporting_sleep_seconds = 300 if store.is_a?(Coverband::Adapters::HashRedisStore)
+      raise "invalid configuration: only coverband service expects an API Key" if api_key && !store.is_a?(Coverband::Adapters::WebServiceStore)
+      raise "invalid configuration: coverband service shouldn't have redis url set" if redis_url && store.is_a?(Coverband::Adapters::WebServiceStore)
 
       @store = store
     end
 
+    def track_views
+      @track_views ||= service_disabled_dev_test_env? ? false : true
+    end
+
     ###
     # Search Paths
     ###
@@ -183,10 +171,6 @@ def ignore=(ignored_array)
       @ignore = (@ignore + ignored_array).uniq
     end
 
-    def track_gems=(_value)
-      puts "gem tracking is deprecated, setting this will be ignored"
-    end
-
     def current_root
       @current_root ||= File.expand_path(Coverband.configuration.root).freeze
     end
@@ -203,7 +187,7 @@ def all_root_patterns
       @all_root_patterns ||= all_root_paths.map { |path| /^#{path}/ }.freeze
     end
 
-    SKIPPED_SETTINGS = %w[@s3_secret_access_key @store]
+    SKIPPED_SETTINGS = %w[@s3_secret_access_key @store @api_key @password]
     def to_h
       instance_variables
         .each_with_object({}) do |var, hash|
@@ -221,6 +205,10 @@ def one_shot_coverage_implemented_in_ruby_version?
       Gem::Version.new(RUBY_VERSION) >= Gem::Version.new("2.6.0")
     end
 
+    def redis_url
+      @redis_url ||= ENV["COVERBAND_REDIS_URL"] || ENV["REDIS_URL"]
+    end
+
     def api_key
       @api_key ||= ENV["COVERBAND_API_KEY"]
     end
@@ -245,25 +233,41 @@ def service_test_mode
       @service_dev_mode ||= ENV["COVERBAND_ENABLE_TEST_MODE"] || false
     end
 
-    def proces_type
+    def process_type
       @process_type ||= ENV["PROCESS_TYPE"] || "unknown"
     end
 
-    def report_period
-      @process_type ||= (ENV["COVERBAND_REPORT_PERIOD"] || 600).to_i
+    def service?
+      Coverband.coverband_service? || !api_key.nil?
     end
 
     def service_disabled_dev_test_env?
       (coverband_env == "test" && !Coverband.configuration.service_test_mode) ||
         (coverband_env == "development" && !Coverband.configuration.service_dev_mode)
     end
 
-    private
+    def s3_bucket
+      puts "deprecated, s3 is no longer support"
+    end
 
-    def redis_url
-      ENV["COVERBAND_REDIS_URL"] || ENV["REDIS_URL"]
+    def s3_region
+      puts "deprecated, s3 is no longer support"
+    end
+
+    def s3_access_key_id
+      puts "deprecated, s3 is no longer support"
+    end
+
+    def s3_secret_access_key
+      puts "deprecated, s3 is no longer support"
+    end
+
+    def track_gems=(_value)
+      puts "gem tracking is deprecated, setting this will be ignored"
     end
 
+    private
+
     def redis_store_options
       {ttl: Coverband.configuration.redis_ttl,
        redis_namespace: Coverband.configuration.redis_namespace}

diff --git a/lib/coverband/reporters/base.rb b/lib/coverband/reporters/base.rb
@@ -101,8 +101,7 @@ def get_current_scov_data_imp(store, roots)
             end
           end
 
-          scov_style_report = fix_file_names(scov_style_report, roots)
-          scov_style_report
+          fix_file_names(scov_style_report, roots)
         end
       end
     end

diff --git a/test/coverband/configuration_test.rb b/test/coverband/configuration_test.rb
@@ -4,6 +4,7 @@
 
 class BaseTest < Minitest::Test
   def setup
+    Coverband.configuration.reset
     super
     Coverband.configuration.reset
     Coverband.configure do |config|
@@ -52,7 +53,7 @@ def setup
     assert_equal current_paths, Coverband.configuration.root_paths
   end
 
-  test "store raises issues" do
+  test "store raises when not set to supported adapter" do
     Coverband::Collectors::Coverage.instance.reset_instance
     assert_raises RuntimeError do
       Coverband.configure do |config|
@@ -61,6 +62,44 @@ def setup
     end
   end
 
+  test "store defaults to redis store" do
+    Coverband::Collectors::Coverage.instance.reset_instance
+    assert_equal Coverband.configuration.store.class, Coverband::Adapters::RedisStore
+  end
+
+  test "store is a service store when api_key is set" do
+    Coverband::Collectors::Coverage.instance.reset_instance
+    Coverband.configuration.reset
+    Coverband.configure do |config|
+      config.redis_url = nil
+      config.api_key = "test-key"
+    end
+    assert_equal Coverband.configuration.store.class.to_s, "Coverband::Adapters::WebServiceStore"
+  end
+
+  test "store raises when api key set but not set to service" do
+    Coverband::Collectors::Coverage.instance.reset_instance
+    Coverband.configuration.reset
+    assert_raises RuntimeError do
+      Coverband.configure do |config|
+        config.api_key = "test-key"
+        config.redis_url = "redis://localhost:3333"
+        config.store = Coverband::Adapters::RedisStore.new(Coverband::Test.redis, redis_namespace: "coverband_test")
+      end
+    end
+  end
+
+  test "store raises when api key and redis_url" do
+    Coverband::Collectors::Coverage.instance.reset_instance
+    Coverband.configuration.reset
+    assert_raises RuntimeError do
+      Coverband.configure do |config|
+        config.api_key = "test-key"
+        config.redis_url = "redis://localhost:3333"
+      end
+    end
+  end
+
   test "use_oneshot_lines_coverage" do
     refute Coverband.configuration.use_oneshot_lines_coverage
 

diff --git a/test/coverband/integrations/background_test.rb b/test/coverband/integrations/background_test.rb
@@ -12,18 +12,29 @@ def alive?
     end
   end
 
+  def setup
+    Coverband.configuration.reset
+    super
+    Coverband.configure do |config|
+      config.background_reporting_sleep_seconds = 60
+      Coverband.configuration.reporting_wiggle = 0
+    end
+  end
+
   def test_start
+    sleep_seconds = Coverband.configuration.background_reporting_sleep_seconds.to_i
     Thread.expects(:new).yields.returns(ThreadDouble.new(true))
     Coverband::Background.expects(:loop).yields
-    Coverband::Background.expects(:sleep).with(60)
+    Coverband::Background.expects(:sleep).with(sleep_seconds)
     Coverband::Collectors::Coverage.instance.expects(:report_coverage).once
     2.times { Coverband::Background.start }
   end
 
   def test_start_with_wiggle
+    sleep_seconds = Coverband.configuration.background_reporting_sleep_seconds.to_i
     Thread.expects(:new).yields.returns(ThreadDouble.new(true))
     Coverband::Background.expects(:loop).yields
-    Coverband::Background.expects(:sleep).with(65)
+    Coverband::Background.expects(:sleep).with(sleep_seconds + 5)
     Coverband::Background.expects(:rand).with(10).returns(5)
     Coverband.configuration.reporting_wiggle = 10
     Coverband::Collectors::Coverage.instance.expects(:report_coverage).once

diff --git a/test/jruby_check.rb b/test/jruby_check.rb
@@ -2,7 +2,7 @@
 
 Coverage.start
 
-require "./test/dog.rb"
+require "./test/dog"
 
 puts Coverage.peek_result
 

diff --git a/test/test_helper.rb b/test/test_helper.rb
@@ -45,6 +45,7 @@ def self.redis
     end
 
     def self.reset
+      Coverband.configuration.reset
       Coverband.configuration.redis_namespace = "coverband_test"
       Coverband.configuration.store.instance_variable_set(:@redis_namespace, "coverband_test")
       Coverband.configuration.store.class.class_variable_set(:@@path_cache, {})