Stop rake parents from clobbering subprocess reports

Three layers of defense end a long-standing failure mode where a parent
process — typically a Rakefile or a Rails app whose `Bundler.require`
pulls in simplecov — auto-loaded `.simplecov`, started `Coverage`, then
shelled out to the test runner. The subprocess wrote a correct report
on its way out; the parent's `at_exit` hook then formatted its own
empty result on top, wiping the data.

First, `.simplecov` is now a pure configuration file. The autoloader
wraps the load in `SimpleCov.with_dot_simplecov_autoload`, and any
`SimpleCov.start` call inside the file is intercepted: its profile and
block are applied via `initial_setup` (configuration only), and a
one-time deprecation warning points users at moving the call into
`spec_helper.rb` / `test_helper.rb`. No `Coverage.start`, no at_exit
hook, no empty parent report.

Second, `ResultMerger.store_result` now merges incoming entries with
same-`command_name` entries whose timestamp is `>=` our
`process_start_time`. Those entries can only have been written by a
sibling test runner that finished while we were running, so combining
coverage data preserves the subprocess's contribution even when an
empty parent result is stored over it.

Third, `SimpleCov.at_exit_behavior` defers entirely when our merged
result has zero files and `coverage/.last_run.json` is fresher than
this process. With the first two layers in place this rarely fires,
but it catches degenerate cases (merging disabled, custom formatters
that bypass the resultset) where the first two can't help.

Resolves #581.

Author: sferik

.rubocop.yml

@@ -55,7 +55,7 @@ Metrics/BlockNesting:
   Max: 2
 
 Metrics/ClassLength:
-  Max: 300
+  Max: 320
 
 Metrics/ModuleLength:
   Description: Avoid modules longer than 100 lines of code.

CHANGELOG.md

@@ -12,6 +12,7 @@ Unreleased
 * HTML and JSON formatters now write the "Coverage report generated for X to Y" status line (and the per-criterion totals beneath it) to stderr instead of stdout. The message is a diagnostic, not the program's output, and routing it to stdout polluted pipelines like `rspec -f json`. Suppress it entirely with `silent: true` on the formatter; redirect with `2>&1` if you want the old behavior. See #1060.
 
 ## Deprecations
+* Calling `SimpleCov.start` from `.simplecov` is deprecated. Coverage tracking still begins for backward compatibility, but a one-time deprecation warning fires pointing the user at moving the call into `spec_helper.rb` / `test_helper.rb`; a future release will require the explicit `SimpleCov.start` from a test helper. The migration goes hand-in-hand with the bugfix below: once `SimpleCov.start` lives in the test helper, the parent process that auto-loads `.simplecov` never starts tracking and the empty-report-overwrite scenario can't arise. See #581.
 * `# :nocov:` toggle comments (and the configurable `SimpleCov.nocov_token` / `SimpleCov.skip_token`) are deprecated in favor of the new `# simplecov:disable` / `# simplecov:enable` directives. Each file that still uses `# :nocov:` emits a one-time deprecation warning to stderr at load time pointing at the recommended replacement, and any call to `SimpleCov.nocov_token` or `SimpleCov.skip_token` (getter or setter) likewise warns. The directive will be removed in a future release.
 
 ## Enhancements
@@ -35,6 +36,7 @@ Unreleased
 * `CommandGuesser` now appends the framework name to parallel test data (e.g. `"RSpec (1/2)"` instead of `"(1/2)"`)
 
 ## Bugfixes
+* Fix the parent-process / subprocess race where a Rakefile (or Rails `Bundler.require`) caused `.simplecov` to auto-load `SimpleCov.start` in the rake parent, which then shelled out to a test runner subprocess; the subprocess wrote a correct report, then the parent's `at_exit` would clobber it with an empty 0% report. Three layers of defense now apply: (1) `.simplecov` is treated as configuration only and no longer starts tracking from the parent (see Deprecations); (2) `ResultMerger.store_result` merges incoming entries with same-`command_name` entries that were written after our `process_start_time` instead of overwriting them; (3) `SimpleCov.at_exit_behavior` defers entirely when our merged result is empty and `coverage/.last_run.json` is fresher than this process. See #581.
 * Don't report misleading 100% branch/method coverage for files added via `track_files` that were never loaded. See #902
 * Fix HTML formatter tab bar layout: dark mode toggle no longer wraps onto two lines, and tabs connect seamlessly with the content panel
 * Allow `SimpleCov.root('/')` so files outside the conventional project root can be tracked (e.g. Docker layouts where code and tests are siblings at `/`). The root-prefix regex no longer doubles the separator (`//`), and `project_name` no longer crashes when `root` has no parent segment. The user-facing `:root_filter` profile and the unconditional `UselessResultsRemover` now share a single regex source instead of computing it independently. See #860.

README.md

@@ -260,29 +260,34 @@ Please check out the [Configuration] API documentation to find out what you can
 
 ## Using .simplecov for centralized config
 
-If you use SimpleCov to merge multiple test suite results (e.g. Test/Unit and Cucumber) into a single report, you'd
-normally have to set up all your config options twice, once in `test_helper.rb` and once in `env.rb`.
-
-To avoid this, you can place a file called `.simplecov` in your project root. You can then just leave the
-`require 'simplecov'` in each test setup helper (**at the top**) and move the `SimpleCov.start` code with all your
-custom config options into `.simplecov`:
+If you use SimpleCov to merge multiple test suite results (e.g. RSpec and Cucumber) into a single report, you'd
+normally have to repeat your filters / groups / profile in every test helper. To avoid that, place a `.simplecov`
+file at your project root with the shared configuration; each test helper then requires SimpleCov and explicitly
+starts it:
 
 ```ruby
-# test/test_helper.rb
+# .simplecov — configuration only
+SimpleCov.profiles.load 'rails'
+add_filter 'lib/generators'
+add_group 'Models', 'app/models'
+
+# spec/spec_helper.rb
 require 'simplecov'
+SimpleCov.start
 
 # features/support/env.rb
 require 'simplecov'
-
-# .simplecov
-SimpleCov.start 'rails' do
-  # any custom configs like groups and filters can be here at a central place
-end
+SimpleCov.start
 ```
 
-Using `.simplecov` rather than separately requiring SimpleCov multiple times is recommended if you are merging multiple
-test frameworks like Cucumber and RSpec that rely on each other, as invoking SimpleCov multiple times can cause coverage
-information to be lost.
+> **Note**: Calling `SimpleCov.start` directly from `.simplecov` is deprecated as of this release. Tracking
+> still begins for backward compatibility, but a one-time deprecation warning fires; a future release will
+> require the explicit `SimpleCov.start` from a test helper. Migrating prevents a long-standing bug where
+> `.simplecov` auto-loaded in a Rakefile or Rails' `Bundler.require` would leave an empty parent-process
+> report to overwrite the test subprocess's good one. See #581.
+
+Using `.simplecov` rather than duplicating configuration in every test helper is recommended if you are merging
+multiple test frameworks like Cucumber and RSpec that rely on each other.
 
 ## Branch coverage (Ruby "~> 2.5")
 Add branch coverage measurement statistics to your results. Supported in CRuby versions 2.5+.

lib/simplecov.rb

@@ -62,11 +62,40 @@ def external_at_exit?
     # Please check out the RDoc for SimpleCov::Configuration to find about available config options
     #
     def start(profile = nil, &)
+      warn_about_start_in_dot_simplecov if @autoloading_dot_simplecov
+
       initial_setup(profile, &)
       start_tracking
       install_at_exit_hook
     end
 
+    # @api private
+    #
+    # Mark the duration of a `.simplecov` auto-load so any `SimpleCov.start`
+    # call inside the file can warn about the impending migration to a
+    # config-only file. Tracking still begins for backward compatibility;
+    # the warning is the cue to move `SimpleCov.start` into a test helper.
+    # See #581.
+    def with_dot_simplecov_autoload
+      previous = @autoloading_dot_simplecov
+      @autoloading_dot_simplecov = true
+      yield
+    ensure
+      @autoloading_dot_simplecov = previous
+    end
+
+    def warn_about_start_in_dot_simplecov
+      return if @dot_simplecov_start_warned
+
+      @dot_simplecov_start_warned = true
+      warn "[DEPRECATION] Calling `SimpleCov.start` from `.simplecov` is deprecated and will " \
+           "be removed in a future release. `.simplecov` should contain configuration only; " \
+           "move the `SimpleCov.start` call into your `spec_helper.rb` / `test_helper.rb`. " \
+           "Coverage tracking still begins for backward compatibility, but a future release " \
+           "will require the explicit `SimpleCov.start` from a test helper. " \
+           "See https://github.com/simplecov-ruby/simplecov/issues/581."
+    end
+
     #
     # Install the at_exit hook that formats results and runs exit-code
     # checks. `SimpleCov.start` calls this automatically. Idempotent —
@@ -223,7 +252,48 @@ def at_exit_behavior
 
       # If Coverage is no longer running (e.g. someone manually stopped it
       # or a test consumed the result) then don't run exit tasks.
-      SimpleCov.run_exit_tasks! if Coverage.running?
+      return unless Coverage.running?
+
+      # Stand down when we'd only clobber a fresher report. See
+      # `defer_to_existing_report?` and issue #581.
+      return if defer_to_existing_report?
+
+      SimpleCov.run_exit_tasks!
+    end
+
+    # Returns true when our process has no coverage data to contribute
+    # (after the resultset merge) and a newer report already exists on
+    # disk. Typically fires when `SimpleCov.start` ran in a parent
+    # process — e.g. a Rakefile or Rails' `Bundler.require` — that
+    # shelled out to the test runner. Without this guard the parent's
+    # empty result would overwrite the subprocess's report on the way
+    # out. See https://github.com/simplecov-ruby/simplecov/issues/581.
+    def defer_to_existing_report?
+      return false unless existing_report_newer_than_us?
+
+      res = result
+      empty = res.nil? || res.files.empty?
+      warn_about_deferred_report if empty
+      empty
+    end
+
+    def existing_report_newer_than_us?
+      return false unless process_start_time
+
+      last_run_path = File.join(coverage_path, ".last_run.json")
+      File.exist?(last_run_path) && File.mtime(last_run_path) > process_start_time
+    end
+
+    def warn_about_deferred_report
+      return unless print_error_status
+
+      warn SimpleCov::Color.colorize(
+        "Skipping SimpleCov report — this process tracked no application code and a newer " \
+        "report already exists at #{coverage_path}. This usually means SimpleCov.start ran in a " \
+        "parent process (e.g. a Rakefile or Rails' Bundler.require) that shelled out to the test " \
+        "runner. See https://github.com/simplecov-ruby/simplecov/issues/581.",
+        :yellow
+      )
     end
 
     # @api private

lib/simplecov/defaults.rb

@@ -36,7 +36,10 @@
 loop do
   filename = config_path.join(".simplecov")
   if filename.exist?
-    begin
+    # `.simplecov` is a configuration file; SimpleCov.start calls inside
+    # it are intercepted and converted to configuration, with a deprecation
+    # warning. See `SimpleCov.with_dot_simplecov_autoload` and issue #581.
+    SimpleCov.with_dot_simplecov_autoload do
       load filename
     rescue LoadError, StandardError
       # simplecov:disable — only fires when .simplecov is unreadable

lib/simplecov/result_merger.rb

@@ -155,13 +155,34 @@ def store_result(result) # rubocop:disable Naming/PredicateMethod
 
           # A single result only ever has one command_name, see `SimpleCov::Result#to_hash`
           command_name, data = result.to_hash.first
-          new_resultset[command_name] = data
+          new_resultset[command_name] = merged_entry(new_resultset[command_name], data)
 
           atomic_write_resultset(new_resultset)
         end
         true
       end
 
+      # If an entry with the same command_name was written AFTER our process
+      # started, a sibling test runner (typically a subprocess our parent
+      # process shelled out to) wrote it. Combine coverage data rather than
+      # overwriting, so an empty parent-process result doesn't clobber the
+      # subprocess's real data. See https://github.com/simplecov-ruby/simplecov/issues/581.
+      def merged_entry(existing, incoming)
+        return incoming unless concurrent_runner_entry?(existing)
+
+        incoming.merge(
+          "coverage" => Combine.combine(Combine::ResultsCombiner, existing["coverage"], incoming["coverage"])
+        )
+      end
+
+      def concurrent_runner_entry?(entry)
+        return false unless entry.is_a?(Hash)
+
+        timestamp = entry["timestamp"]
+        process_start = SimpleCov.process_start_time
+        timestamp && process_start && timestamp.to_i >= process_start.to_i
+      end
+
       # Write to a temp file first, then atomically rename to avoid other
       # processes reading a truncated/incomplete .resultset.json.
       def atomic_write_resultset(resultset)

spec/result_merger_spec.rb

@@ -276,6 +276,63 @@
       described_class.store_result({})
       expect(described_class).to have_received(:synchronize_resultset)
     end
+
+    # See https://github.com/simplecov-ruby/simplecov/issues/581. When a parent
+    # process (Rakefile, Rails Bundler.require) shells out to the test runner,
+    # the subprocess writes its real result to the resultset and then the
+    # parent's at_exit hook stores its own (empty) result under the same
+    # command_name. Without merging, the parent overwrites the subprocess's
+    # data; with the guard, the parent's incoming entry is combined with the
+    # existing one so the subprocess's coverage survives.
+    describe "merging same-command-name entries written by a concurrent runner" do
+      let(:process_start) { Time.now }
+      let(:subprocess_result) do
+        SimpleCov::Result.new(
+          {source_fixture("sample.rb") => {"lines" => [nil, 1, 1, 1, nil, nil, 1, 1, nil, nil]}},
+          command_name: "RSpec"
+        )
+      end
+      let(:parent_empty_result) { SimpleCov::Result.new({}, command_name: "RSpec") }
+
+      before { allow(SimpleCov).to receive(:process_start_time).and_return(process_start) }
+
+      it "merges parent's incoming entry into the subprocess's when newer than our process_start_time" do
+        subprocess_result.created_at = process_start + 1 # subprocess finished after we started
+        described_class.store_result(subprocess_result)
+
+        parent_empty_result.created_at = process_start + 2
+        described_class.store_result(parent_empty_result)
+
+        merged = described_class.read_resultset.fetch("RSpec").fetch("coverage")
+        expect(merged.keys).to contain_exactly(source_fixture("sample.rb"))
+      end
+
+      it "still overwrites an older entry from a previous run (older than process_start)" do
+        # A stale entry from a previous test run shouldn't be merged in — it's
+        # not from a concurrent runner, just leftover state.
+        stale = SimpleCov::Result.new(
+          {source_fixture("sample.rb") => {"lines" => [nil, 1, 1, 1, nil, nil, 1, 1, nil, nil]}},
+          command_name: "RSpec"
+        )
+        stale.created_at = process_start - 60
+        described_class.store_result(stale)
+
+        parent_empty_result.created_at = process_start + 1
+        described_class.store_result(parent_empty_result)
+
+        expect(described_class.read_resultset.fetch("RSpec").fetch("coverage")).to be_empty
+      end
+
+      it "is a no-op when process_start_time is unset (e.g. SimpleCov.start was never called)" do
+        allow(SimpleCov).to receive(:process_start_time).and_return(nil)
+
+        subprocess_result.created_at = Time.now
+        described_class.store_result(subprocess_result)
+        described_class.store_result(parent_empty_result)
+
+        expect(described_class.read_resultset.fetch("RSpec").fetch("coverage")).to be_empty
+      end
+    end
   end
 
   describe ".resultset" do