diff --git a/lib/simplecov.rb b/lib/simplecov.rb index 3e3ed5e8..d4247068 100644 --- a/lib/simplecov.rb +++ b/lib/simplecov.rb @@ -75,23 +75,21 @@ def add_not_loaded_files(result) # from cache using SimpleCov::ResultMerger if use_merging is activated (default) # def result - # Ensure the variable is defined to avoid ruby warnings - @result = nil unless defined?(@result) + return @result if result? # Collect our coverage result - if running && !result? + if running @result = SimpleCov::Result.new add_not_loaded_files(Coverage.result) end # If we're using merging of results, store the current result - # first, then merge the results and return those + # first (if there is one), then merge the results and return those if use_merging SimpleCov::ResultMerger.store_result(@result) if result? - - SimpleCov::ResultMerger.merged_result - else - @result + @result = SimpleCov::ResultMerger.merged_result end + + @result ensure self.running = false end @@ -158,6 +156,13 @@ def usable? false end end + + # + # Clear out the previously cached .result. Primarily useful in testing + # + def clear_result + @result = nil + end end end diff --git a/lib/simplecov/result_merger.rb b/lib/simplecov/result_merger.rb index 58f0dd47..21df15ba 100644 --- a/lib/simplecov/result_merger.rb +++ b/lib/simplecov/result_merger.rb @@ -17,25 +17,31 @@ def resultset_writelock File.join(SimpleCov.coverage_path, ".resultset.json.lock") end - # Loads the cached resultset from JSON and returns it as a Hash + # Loads the cached resultset from JSON and returns it as a Hash, + # caching it for subsequent accesses. def resultset - if stored_data - begin - JSON.parse(stored_data) - rescue + @resultset ||= begin + data = stored_data + if data + begin + JSON.parse(data) || {} + rescue + {} + end + else {} end - else - {} end end # Returns the contents of the resultset cache as a string or if the file is missing or empty nil def stored_data - return unless File.exist?(resultset_path) - data = File.read(resultset_path) - return if data.nil? || data.length < 2 - data + synchronize_resultset do + return unless File.exist?(resultset_path) + data = File.read(resultset_path) + return if data.nil? || data.length < 2 + data + end end # Gets the resultset hash and re-creates all included instances @@ -76,8 +82,9 @@ def merged_result # Saves the given SimpleCov::Result in the resultset cache def store_result(result) - File.open(resultset_writelock, "w+") do |f| - f.flock(File::LOCK_EX) + synchronize_resultset do + # Ensure we have the latest, in case it was already cached + clear_resultset new_set = resultset command_name, data = result.to_hash.first new_set[command_name] = data @@ -87,6 +94,28 @@ def store_result(result) end true end + + # Ensure only one process is reading or writing the resultset at any + # given time + def synchronize_resultset + # make it reentrant + return yield if defined?(@resultset_locked) && @resultset_locked + + begin + @resultset_locked = true + File.open(resultset_writelock, "w+") do |f| + f.flock(File::LOCK_EX) + yield + end + ensure + @resultset_locked = false + end + end + + # Clear out the previously cached .resultset + def clear_resultset + @resultset = nil + end end end end diff --git a/spec/result_merger_spec.rb b/spec/result_merger_spec.rb index bfaf61ab..27850af6 100644 --- a/spec/result_merger_spec.rb +++ b/spec/result_merger_spec.rb @@ -1,10 +1,16 @@ require "helper" +require "tempfile" +require "timeout" if SimpleCov.usable? describe SimpleCov::ResultMerger do + before do + SimpleCov::ResultMerger.clear_resultset + File.delete(SimpleCov::ResultMerger.resultset_path) if File.exist?(SimpleCov::ResultMerger.resultset_path) + end + describe "with two faked coverage resultsets" do before do - SimpleCov.use_merging true @resultset1 = { source_fixture("sample.rb") => [nil, 1, 1, 1, nil, nil, 1, 1, nil, nil], source_fixture("app/models/user.rb") => [nil, 1, 1, 1, nil, nil, 1, 0, nil, nil], @@ -73,15 +79,92 @@ expect(SimpleCov::ResultMerger.results.length).to eq(1) end end + end + end + end + + describe ".store_result" do + it "refreshes the resultset" do + set = SimpleCov::ResultMerger.resultset + SimpleCov::ResultMerger.store_result({}) + new_set = SimpleCov::ResultMerger.resultset + expect(new_set).not_to be(set) + end - context "with merging disabled" do - before { SimpleCov.use_merging false } + it "persists to disk" do + SimpleCov::ResultMerger.store_result("a" => [1]) + SimpleCov::ResultMerger.clear_resultset + new_set = SimpleCov::ResultMerger.resultset + expect(new_set).to eq("a" => [1]) + end + + it "synchronizes writes" do + expect(SimpleCov::ResultMerger).to receive(:synchronize_resultset) + SimpleCov::ResultMerger.store_result({}) + end + end - it "returns nil for SimpleCov.result" do - expect(SimpleCov.result).to be_nil + describe ".resultset" do + it "caches" do + set = SimpleCov::ResultMerger.resultset + new_set = SimpleCov::ResultMerger.resultset + expect(new_set).to be(set) + end + + it "synchronizes reads" do + expect(SimpleCov::ResultMerger).to receive(:synchronize_resultset) + SimpleCov::ResultMerger.resultset + end + end + + describe ".synchronize_resultset" do + it "is reentrant (i.e. doesn't block its own process)" do + # without @resultset_locked, this spec would fail and + # `.store_result` wouldn't work + expect do + Timeout.timeout(1) do + SimpleCov::ResultMerger.synchronize_resultset do + SimpleCov::ResultMerger.synchronize_resultset do + end end end + end.not_to raise_error + end + + it "blocks other processes" do + file = Tempfile.new("foo") + + other_process = open("|ruby -e " + Shellwords.escape(<<-CODE) + " 2>/dev/null") + require "simplecov" + SimpleCov.coverage_dir(#{SimpleCov.coverage_dir.inspect}) + + # ensure the parent process has enough time to get a + # lock before we do + sleep 0.5 + + $stdout.sync = true + puts "running" # see `sleep`s in parent process + + SimpleCov::ResultMerger.synchronize_resultset do + File.open(#{file.path.inspect}, "a") { |f| f.write("process 2\n") } + end + CODE + + SimpleCov::ResultMerger.synchronize_resultset do + # wait until the child process is going, and then wait some more + # so we can be sure it blocks on the lock we already have. + sleep 0.1 until other_process.gets == "running\n" + sleep 1 + + # despite the sleeps, this will be written first since we got + # the first lock + File.open(file.path, "a") { |f| f.write("process 1\n") } end + + # wait for it to finish + other_process.gets + + expect(file.read).to eq("process 1\nprocess 2\n") end end end diff --git a/spec/simplecov_spec.rb b/spec/simplecov_spec.rb new file mode 100644 index 00000000..ae3a85f1 --- /dev/null +++ b/spec/simplecov_spec.rb @@ -0,0 +1,109 @@ +require "helper" + +if SimpleCov.usable? + describe SimpleCov do + describe ".result" do + before do + SimpleCov.clear_result + allow(Coverage).to receive(:result).once.and_return({}) + end + + context "with merging disabled" do + before do + allow(SimpleCov).to receive(:use_merging).once.and_return(false) + end + + context "when not running" do + before do + allow(SimpleCov).to receive(:running).and_return(false) + end + + it "returns nil" do + expect(SimpleCov.result).to be_nil + end + end + + context "when running" do + before do + allow(SimpleCov).to receive(:running).and_return(true, false) + end + + it "uses the result from Coverage" do + expect(Coverage).to receive(:result).once.and_return(__FILE__ => [0, 1]) + expect(SimpleCov.result.filenames).to eq [__FILE__] + end + + it "adds not-loaded-files" do + expect(SimpleCov).to receive(:add_not_loaded_files).once.and_return({}) + SimpleCov.result + end + + it "doesn't store the current coverage" do + expect(SimpleCov::ResultMerger).not_to receive(:store_result) + SimpleCov.result + end + + it "doesn't merge the result" do + expect(SimpleCov::ResultMerger).not_to receive(:merged_result) + SimpleCov.result + end + + it "caches its result" do + result = SimpleCov.result + expect(SimpleCov.result).to be(result) + end + end + end + + context "with merging enabled" do + let(:the_merged_result) { double } + + before do + allow(SimpleCov).to receive(:use_merging).once.and_return(true) + allow(SimpleCov::ResultMerger).to receive(:store_result).once + allow(SimpleCov::ResultMerger).to receive(:merged_result).once.and_return(the_merged_result) + end + + context "when not running" do + before do + allow(SimpleCov).to receive(:running).and_return(false) + end + + it "merges the result" do + expect(SimpleCov.result).to be(the_merged_result) + end + end + + context "when running" do + before do + allow(SimpleCov).to receive(:running).and_return(true, false) + end + + it "uses the result from Coverage" do + expect(Coverage).to receive(:result).once.and_return({}) + SimpleCov.result + end + + it "adds not-loaded-files" do + expect(SimpleCov).to receive(:add_not_loaded_files).once.and_return({}) + SimpleCov.result + end + + it "stores the current coverage" do + expect(SimpleCov::ResultMerger).to receive(:store_result).once + SimpleCov.result + end + + it "merges the result" do + expect(SimpleCov.result).to be(the_merged_result) + end + + it "caches its result" do + result = SimpleCov.result + expect(SimpleCov.result).to be(result) + end + end + end + end + end +end