Exit status refactoring & differentiate originator of failign exit code

CHANGELOG.md

@@ -6,9 +6,12 @@ Unreleased
 
 ## Enhancements
 * observe forked processes (enable with SimpleCov.enable_for_subprocesses)
+* SimpleCov distinguishes better that it stopped processing because of a previous error vs. SimpleCov is the originator of said error due to coverage requirements.
 
 ## Bugfixes
 * Changing the `SimpleCov.root` combined with the root filtering didn't work. Now they do! Thanks to [@deivid-rodriguez](https://github.com/deivid-rodriguez) and see [#894](https://github.com/colszowka/simplecov/pull/894)
+* in parallel test execution it could happen that the last coverage result was written when it didn't complete yet, changed to only write it once it's the final result
+* if you run parallel tests only the final process will report violations of the configured test coverage, not all previous processes
 
 0.18.5 (2020-02-25)
 ===================

features/parallel_tests.feature

@@ -59,3 +59,17 @@ Feature:
       """
     When I open the coverage report generated with `bundle exec parallel_rspec spec`
     Then I should see the branch coverage results for the parallel tests project
+
+  # Our detection doesn't work at the moment see https://github.com/grosser/parallel_tests/issues/772
+  @wip
+  Scenario: Coverage violations aren't printed until the end
+    Given I install dependencies
+    And SimpleCov for RSpec is configured with:
+      """
+      require 'simplecov'
+      SimpleCov.start do
+        minimum_coverage 89
+      end
+      """
+    When I successfully run `bundle exec parallel_rspec spec`
+    Then the output should not match /+cover.+below.+minimum/

features/rspec_failing.feature

@@ -0,0 +1,19 @@
+@rspec
+Feature:
+
+  RSpec failing in different ways results SimpleCov saying something beforehand. However it doesn't identify itself as the originator of said error.
+
+  Background:
+    Given I'm working on the project "faked_project"
+
+  Scenario:
+    When I run `bundle exec rspec bad_spec/failing_spec.rb`
+    Then the exit status should not be 0
+    And the output should match /SimpleCov.+previous.+error/
+    And the output should not match /SimpleCov.+exit.+with.+status/
+
+  Scenario:
+    When I run `bundle exec rspec bad_spec/fail_with_5.rb`
+    Then the exit status should be 5
+    And the output should match /SimpleCov.+previous.+error/
+    And the output should not match /SimpleCov.+exit.+with.+status/

lib/simplecov.rb

@@ -24,7 +24,6 @@
 module SimpleCov
   class << self
     attr_accessor :running, :pid
-    attr_reader :exit_exception
 
     # Basically, should we take care of at_exit behavior or something else?
     # Used by the minitest plugin. See lib/minitest/simplecov_plugin.rb
@@ -174,54 +173,71 @@ def clear_result
       @result = nil
     end
 
+    def at_exit_behavior
+      # If we are in a different process than called start, don't interfere.
+      return if SimpleCov.pid != Process.pid
+
+      # If SimpleCov is no longer running then don't run exit tasks
+      SimpleCov.run_exit_tasks! if SimpleCov.running
+    end
+
+    # @api private
     #
-    # Capture the current exception if it exists
-    # This will get called inside the at_exit block
+    # Called from at_exit block
     #
-    def set_exit_exception
-      @exit_exception = $ERROR_INFO
+    def run_exit_tasks!
+      error_exit_status = exit_status_from_exception
+
+      at_exit.call
+
+      exit_and_report_previous_error(error_exit_status) if previous_error?(error_exit_status)
+      process_results_and_report_error if ready_to_process_results?
     end
 
+    #
+    # @api private
     #
     # Returns the exit status from the exit exception
     #
     def exit_status_from_exception
-      return SimpleCov::ExitCodes::SUCCESS unless exit_exception
+      # Capture the current exception if it exists
+      @exit_exception = $ERROR_INFO
+      return nil unless @exit_exception
 
-      if exit_exception.is_a?(SystemExit)
-        exit_exception.status
+      if @exit_exception.is_a?(SystemExit)
+        @exit_exception.status
       else
         SimpleCov::ExitCodes::EXCEPTION
       end
     end
 
-    def at_exit_behavior
-      # If we are in a different process than called start, don't interfere.
-      return if SimpleCov.pid != Process.pid
-
-      # If SimpleCov is no longer running then don't run exit tasks
-      SimpleCov.run_exit_tasks! if SimpleCov.running
+    # @api private
+    def previous_error?(error_exit_status)
+      # Normally it'd be enough to check for previous error but when running test_unit
+      # status is 0
+      error_exit_status && error_exit_status != SimpleCov::ExitCodes::SUCCESS
     end
 
-    # @api private
     #
-    # Called from at_exit block
+    # @api private
     #
-    def run_exit_tasks!
-      set_exit_exception
-
-      exit_status = SimpleCov.exit_status_from_exception
+    # Thinking: Move this behavior earlier so if there was an error we do nothing?
+    def exit_and_report_previous_error(exit_status)
+      warn("Stopped processing SimpleCov as a previous error not related to SimpleCov has been detected") if print_error_status
+      Kernel.exit(exit_status)
+    end
 
-      SimpleCov.at_exit.call
+    # @api private
+    def ready_to_process_results?
+      final_result_process? && result?
+    end
 
-      # Don't modify the exit status unless the result has already been
-      # computed
-      exit_status = SimpleCov.process_result(SimpleCov.result, exit_status) if SimpleCov.result?
+    def process_results_and_report_error
+      exit_status = process_result(SimpleCov.result)
 
       # Force exit with stored status (see github issue #5)
-      # unless it's nil or 0 (see github issue #281)
-      if exit_status&.positive?
-        $stderr.printf("SimpleCov failed with exit %<exit_status>d\n", exit_status: exit_status) if print_error_status
+      if exit_status.positive?
+        warn("SimpleCov failed with exit #{exit_status} due to a coverage related error") if print_error_status
         Kernel.exit exit_status
       end
     end
@@ -231,48 +247,22 @@ def run_exit_tasks!
     # Usage:
     #   exit_status = SimpleCov.process_result(SimpleCov.result, exit_status)
     #
-    def process_result(result, exit_status)
-      return exit_status if exit_status != SimpleCov::ExitCodes::SUCCESS # Existing errors
-
-      covered_percent = result.covered_percent.floor(2)
-      result_exit_status = result_exit_status(result, covered_percent)
-      write_last_run(covered_percent) if result_exit_status == SimpleCov::ExitCodes::SUCCESS # No result errors
-      final_result_process? ? result_exit_status : SimpleCov::ExitCodes::SUCCESS
+    def process_result(result)
+      result_exit_status = result_exit_status(result)
+      write_last_run(result) if result_exit_status == SimpleCov::ExitCodes::SUCCESS
+      result_exit_status
     end
 
     # @api private
-    #
-    # rubocop:disable Metrics/MethodLength,Metrics/PerceivedComplexity
-    def result_exit_status(result, covered_percent)
-      covered_percentages = result.covered_percentages.map { |percentage| percentage.floor(2) }
-      if (minimum_violations = minimum_coverage_violated(result)).any?
-        report_minimum_violated(minimum_violations)
-        SimpleCov::ExitCodes::MINIMUM_COVERAGE
-      elsif covered_percentages.any? { |p| p < SimpleCov.minimum_coverage_by_file }
-        $stderr.printf(
-          "File (%<file>s) is only (%<least_covered_percentage>.2f%%) covered. This is below the expected minimum coverage per file of (%<min_coverage>.2f%%).\n",
-          file: result.least_covered_file,
-          least_covered_percentage: covered_percentages.min,
-          min_coverage: SimpleCov.minimum_coverage_by_file
-        )
-        SimpleCov::ExitCodes::MINIMUM_COVERAGE
-      elsif (last_run = SimpleCov::LastRun.read)
-        coverage_diff = last_run[:result][:covered_percent] - covered_percent
-        if coverage_diff > SimpleCov.maximum_coverage_drop
-          $stderr.printf(
-            "Coverage has dropped by %<drop_percent>.2f%% since the last time (maximum allowed: %<max_drop>.2f%%).\n",
-            drop_percent: coverage_diff,
-            max_drop: SimpleCov.maximum_coverage_drop
-          )
-          SimpleCov::ExitCodes::MAXIMUM_COVERAGE_DROP
-        else
-          SimpleCov::ExitCodes::SUCCESS
-        end
-      else
-        SimpleCov::ExitCodes::SUCCESS
-      end
+    CoverageLimits = Struct.new(:minimum_coverage, :minimum_coverage_by_file, :maximum_coverage_drop, keyword_init: true)
+    def result_exit_status(result)
+      coverage_limits = CoverageLimits.new(
+        minimum_coverage: minimum_coverage, minimum_coverage_by_file: minimum_coverage_by_file,
+        maximum_coverage_drop: maximum_coverage_drop
+      )
+
+      ExitCodes::ExitCodeHandling.call(result, coverage_limits: coverage_limits)
     end
-    # rubocop:enable Metrics/MethodLength,Metrics/PerceivedComplexity
 
     #
     # @api private
@@ -294,8 +284,16 @@ def wait_for_other_processes
     #
     # @api private
     #
-    def write_last_run(covered_percent)
-      SimpleCov::LastRun.write(result: {covered_percent: covered_percent})
+    def write_last_run(result)
+      SimpleCov::LastRun.write(result: {covered_percent: round_coverage(result.covered_percent)})
+    end
+
+    #
+    # @api private
+    #
+    # Rounding down to be extra strict, see #679
+    def round_coverage(coverage)
+      coverage.floor(2)
     end
 
   private
@@ -419,34 +417,10 @@ def remove_useless_results
     def result_with_not_loaded_files
       @result = SimpleCov::Result.new(add_not_loaded_files(@result))
     end
-
-    def minimum_coverage_violated(result)
-      coverage_achieved = minimum_coverage.map do |criterion, percent|
-        {
-          criterion: criterion,
-          minimum_expected: percent,
-          actual: result.coverage_statistics[criterion].percent
-        }
-      end
-
-      coverage_achieved.select do |achieved|
-        achieved.fetch(:actual) < achieved.fetch(:minimum_expected)
-      end
-    end
-
-    def report_minimum_violated(violations)
-      violations.each do |violation|
-        $stderr.printf(
-          "%<criterion>s coverage (%<covered>.2f%%) is below the expected minimum coverage (%<minimum_coverage>.2f%%).\n",
-          covered: violation.fetch(:actual).floor(2),
-          minimum_coverage: violation.fetch(:minimum_expected),
-          criterion: violation.fetch(:criterion).capitalize
-        )
-      end
-    end
   end
 end
 
+# requires are down here here for a load order reason I'm not sure what it is about
 require "set"
 require "forwardable"
 require_relative "simplecov/configuration"

lib/simplecov/exit_codes.rb

@@ -8,3 +8,8 @@ module ExitCodes
     MAXIMUM_COVERAGE_DROP = 3
   end
 end
+
+require_relative "exit_codes/exit_code_handling"
+require_relative "exit_codes/maximum_coverage_drop_check"
+require_relative "exit_codes/minimum_coverage_by_file_check"
+require_relative "exit_codes/minimum_overall_coverage_check"

lib/simplecov/exit_codes/exit_code_handling.rb

@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+module SimpleCov
+  module ExitCodes
+    module ExitCodeHandling
+    module_function
+
+      def call(result, coverage_limits:)
+        checks = coverage_checks(result, coverage_limits)
+
+        failing_check = checks.find(&:failing?)
+        if failing_check
+          failing_check.report
+          failing_check.exit_code
+        else
+          SimpleCov::ExitCodes::SUCCESS
+        end
+      end
+
+      def coverage_checks(result, coverage_limits)
+        [
+          MinimumOverallCoverageCheck.new(result, coverage_limits.minimum_coverage),
+          MinimumCoverageByFileCheck.new(result, coverage_limits.minimum_coverage_by_file),
+          MaximumCoverageDropCheck.new(result, coverage_limits.maximum_coverage_drop)
+        ]
+      end
+    end
+  end
+end

lib/simplecov/exit_codes/maximum_coverage_drop_check.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+module SimpleCov
+  module ExitCodes
+    class MaximumCoverageDropCheck
+      def initialize(result, maximum_coverage_drop)
+        @result = result
+        @maximum_coverage_drop = maximum_coverage_drop
+      end
+
+      def failing?
+        return false unless maximum_coverage_drop && last_run
+
+        coverage_diff > maximum_coverage_drop
+      end
+
+      def report
+        $stderr.printf(
+          "Coverage has dropped by %<drop_percent>.2f%% since the last time (maximum allowed: %<max_drop>.2f%%).\n",
+          drop_percent: coverage_diff,
+          max_drop: maximum_coverage_drop
+        )
+      end
+
+      def exit_code
+        SimpleCov::ExitCodes::MAXIMUM_COVERAGE_DROP
+      end
+
+    private
+
+      attr_reader :result, :maximum_coverage_drop
+
+      def last_run
+        return @last_run if defined?(@last_run)
+
+        @last_run = SimpleCov::LastRun.read
+      end
+
+      def coverage_diff
+        raise "Trying to access coverage_diff although there is no last run" unless last_run
+
+        @coverage_diff ||= last_run[:result][:covered_percent] - covered_percent
+      end
+
+      def covered_percent
+        SimpleCov.round_coverage(result.covered_percent)
+      end
+    end
+  end
+end