From 1f1d962b9792b0f3277e23f09f10a150f55b2907 Mon Sep 17 00:00:00 2001 From: Andrey Date: Wed, 5 Jun 2024 15:54:47 +0200 Subject: [PATCH 1/9] multi threaded code coverage support for datadog_cov --- ext/datadog_cov/datadog_cov.c | 54 +++++++++++++++--- sig/datadog/ci/itr/coverage/ddcov.rbs | 3 +- spec/ddcov/ddcov_spec.rb | 81 ++++++++++++++++++--------- 3 files changed, 101 insertions(+), 37 deletions(-) diff --git a/ext/datadog_cov/datadog_cov.c b/ext/datadog_cov/datadog_cov.c index 9d926001..f96389bd 100644 --- a/ext/datadog_cov/datadog_cov.c +++ b/ext/datadog_cov/datadog_cov.c @@ -1,7 +1,11 @@ #include #include -const int PROFILE_FRAMES_BUFFER_SIZE = 1; +#define PROFILE_FRAMES_BUFFER_SIZE 1 + +// threading modes +#define SINGLE_THREADED_COVERAGE_MODE 0 +#define MULTI_THREADED_COVERAGE_MODE 1 char *ruby_strndup(const char *str, size_t size) { @@ -26,6 +30,8 @@ struct dd_cov_data VALUE coverage; uintptr_t last_filename_ptr; + + int threading_mode; }; static void dd_cov_mark(void *ptr) @@ -68,6 +74,7 @@ static VALUE dd_cov_allocate(VALUE klass) dd_cov_data->ignored_path = NULL; dd_cov_data->ignored_path_len = 0; dd_cov_data->last_filename_ptr = 0; + dd_cov_data->threading_mode = MULTI_THREADED_COVERAGE_MODE; return obj; } @@ -85,9 +92,25 @@ static VALUE dd_cov_initialize(int argc, VALUE *argv, VALUE self) } VALUE rb_ignored_path = rb_hash_lookup(opt, ID2SYM(rb_intern("ignored_path"))); + VALUE rb_threading_mode = rb_hash_lookup(opt, ID2SYM(rb_intern("threading_mode"))); + int threading_mode; + if (!RTEST(rb_threading_mode) || rb_threading_mode == ID2SYM(rb_intern("multi"))) + { + threading_mode = MULTI_THREADED_COVERAGE_MODE; + } + else if (rb_threading_mode == ID2SYM(rb_intern("single"))) + { + threading_mode = SINGLE_THREADED_COVERAGE_MODE; + } + else + { + rb_raise(rb_eArgError, "mode is invalid"); + } + struct dd_cov_data *dd_cov_data; TypedData_Get_Struct(self, struct dd_cov_data, &dd_cov_data_type, dd_cov_data); + dd_cov_data->threading_mode = threading_mode; dd_cov_data->root_len = RSTRING_LEN(rb_root); dd_cov_data->root = ruby_strndup(RSTRING_PTR(rb_root), dd_cov_data->root_len); @@ -152,8 +175,6 @@ static void dd_cov_update_coverage(rb_event_flag_t event, VALUE data, VALUE self static VALUE dd_cov_start(VALUE self) { - // get current thread - VALUE thval = rb_thread_current(); struct dd_cov_data *dd_cov_data; TypedData_Get_Struct(self, struct dd_cov_data, &dd_cov_data_type, dd_cov_data); @@ -163,6 +184,16 @@ static VALUE dd_cov_start(VALUE self) rb_raise(rb_eRuntimeError, "root is required"); } + if (dd_cov_data->threading_mode == SINGLE_THREADED_COVERAGE_MODE) + { + VALUE thval = rb_thread_current(); + rb_thread_add_event_hook(thval, dd_cov_update_coverage, RUBY_EVENT_LINE, self); + } + else + { + rb_add_event_hook(dd_cov_update_coverage, RUBY_EVENT_LINE, self); + } + // add event hook rb_thread_add_event_hook(thval, dd_cov_update_coverage, RUBY_EVENT_LINE, self); @@ -171,14 +202,21 @@ static VALUE dd_cov_start(VALUE self) static VALUE dd_cov_stop(VALUE self) { - // get current thread - VALUE thval = rb_thread_current(); - // remove event hook for the current thread - rb_thread_remove_event_hook(thval, dd_cov_update_coverage); - struct dd_cov_data *dd_cov_data; TypedData_Get_Struct(self, struct dd_cov_data, &dd_cov_data_type, dd_cov_data); + if (dd_cov_data->threading_mode == SINGLE_THREADED_COVERAGE_MODE) + { + // get current thread + VALUE thval = rb_thread_current(); + // remove event hook for the current thread + rb_thread_remove_event_hook(thval, dd_cov_update_coverage); + } + else + { + rb_remove_event_hook(dd_cov_update_coverage); + } + VALUE res = dd_cov_data->coverage; dd_cov_data->coverage = rb_hash_new(); diff --git a/sig/datadog/ci/itr/coverage/ddcov.rbs b/sig/datadog/ci/itr/coverage/ddcov.rbs index 7431b1b2..0027dc6c 100644 --- a/sig/datadog/ci/itr/coverage/ddcov.rbs +++ b/sig/datadog/ci/itr/coverage/ddcov.rbs @@ -3,8 +3,9 @@ module Datadog module ITR module Coverage class DDCov + type threading_mode = :multi | :single - def initialize: (root: String, ignored_path: String?) -> void + def initialize: (root: String, ignored_path: String?, ?threading_mode: threading_mode?) -> void def start: () -> void diff --git a/spec/ddcov/ddcov_spec.rb b/spec/ddcov/ddcov_spec.rb index d40633f7..f76503f4 100644 --- a/spec/ddcov/ddcov_spec.rb +++ b/spec/ddcov/ddcov_spec.rb @@ -158,50 +158,75 @@ def absolute_path(path) context "multi threaded execution" do def thread_local_cov - Thread.current[:datadog_ci_cov] ||= described_class.new(root: root) + Thread.current[:datadog_ci_cov] ||= described_class.new(root: root, threading_mode: threading_mode) end - it "collects coverage for each thread separately" do - t1_queue = Queue.new - t2_queue = Queue.new + context "in single threaded coverage mode" do + let(:threading_mode) { :single } - t1 = Thread.new do - cov = thread_local_cov - cov.start + it "collects coverage for each thread separately" do + t1_queue = Queue.new + t2_queue = Queue.new - t1_queue << :ready - expect(t2_queue.pop).to be(:ready) + t1 = Thread.new do + cov = thread_local_cov + cov.start - expect(calculator.add(1, 2)).to eq(3) - expect(calculator.multiply(1, 2)).to eq(2) + t1_queue << :ready + expect(t2_queue.pop).to be(:ready) - t1_queue << :done - expect(t2_queue.pop).to be :done + expect(calculator.add(1, 2)).to eq(3) + expect(calculator.multiply(1, 2)).to eq(2) - coverage = cov.stop - expect(coverage.size).to eq(2) - expect(coverage.keys).to include(absolute_path("calculator/operations/add.rb")) - expect(coverage.keys).to include(absolute_path("calculator/operations/multiply.rb")) + t1_queue << :done + expect(t2_queue.pop).to be :done + + coverage = cov.stop + expect(coverage.size).to eq(2) + expect(coverage.keys).to include(absolute_path("calculator/operations/add.rb")) + expect(coverage.keys).to include(absolute_path("calculator/operations/multiply.rb")) + end + + t2 = Thread.new do + cov = thread_local_cov + cov.start + + t2_queue << :ready + expect(t1_queue.pop).to be(:ready) + + expect(calculator.subtract(1, 2)).to eq(-1) + + t2_queue << :done + expect(t1_queue.pop).to be :done + + coverage = cov.stop + expect(coverage.size).to eq(1) + expect(coverage.keys).to include(absolute_path("calculator/operations/subtract.rb")) + end + + [t1, t2].each(&:join) end + end + + context "in multi threaded code coverage mode" do + let(:threading_mode) { :multi } - t2 = Thread.new do + it "collects coverage for background threads" do cov = thread_local_cov cov.start - t2_queue << :ready - expect(t1_queue.pop).to be(:ready) - - expect(calculator.subtract(1, 2)).to eq(-1) + t = Thread.new do + expect(calculator.add(1, 2)).to eq(3) + end - t2_queue << :done - expect(t1_queue.pop).to be :done + expect(calculator.multiply(1, 2)).to eq(2) + t.join coverage = cov.stop - expect(coverage.size).to eq(1) - expect(coverage.keys).to include(absolute_path("calculator/operations/subtract.rb")) + expect(coverage.size).to eq(2) + expect(coverage.keys).to include(absolute_path("calculator/operations/add.rb")) + expect(coverage.keys).to include(absolute_path("calculator/operations/multiply.rb")) end - - [t1, t2].each(&:join) end end end From bb02f5d3e2a6572d407fdc82e31977439a29265c Mon Sep 17 00:00:00 2001 From: Andrey Date: Thu, 6 Jun 2024 09:52:52 +0200 Subject: [PATCH 2/9] change the exception on invalid coverage mode --- ext/datadog_cov/datadog_cov.c | 2 +- spec/ddcov/ddcov_spec.rb | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/ext/datadog_cov/datadog_cov.c b/ext/datadog_cov/datadog_cov.c index f96389bd..ee20e573 100644 --- a/ext/datadog_cov/datadog_cov.c +++ b/ext/datadog_cov/datadog_cov.c @@ -104,7 +104,7 @@ static VALUE dd_cov_initialize(int argc, VALUE *argv, VALUE self) } else { - rb_raise(rb_eArgError, "mode is invalid"); + rb_raise(rb_eArgError, "threading mode is invalid"); } struct dd_cov_data *dd_cov_data; diff --git a/spec/ddcov/ddcov_spec.rb b/spec/ddcov/ddcov_spec.rb index f76503f4..5832312f 100644 --- a/spec/ddcov/ddcov_spec.rb +++ b/spec/ddcov/ddcov_spec.rb @@ -228,6 +228,16 @@ def thread_local_cov expect(coverage.keys).to include(absolute_path("calculator/operations/multiply.rb")) end end + + context "when threading mode is invalid" do + let(:threading_mode) { :invalid_mode } + + it "raises an error" do + expect { described_class.new(root: root, threading_mode: threading_mode) }.to( + raise_error(ArgumentError, "threading mode is invalid") + ) + end + end end end end From f9c53167a4a743695e7e4bfefde27a92ebc68b3e Mon Sep 17 00:00:00 2001 From: Andrey Date: Thu, 6 Jun 2024 10:08:55 +0200 Subject: [PATCH 3/9] remove unneeded comments --- ext/datadog_cov/datadog_cov.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/ext/datadog_cov/datadog_cov.c b/ext/datadog_cov/datadog_cov.c index ee20e573..ed020e86 100644 --- a/ext/datadog_cov/datadog_cov.c +++ b/ext/datadog_cov/datadog_cov.c @@ -207,9 +207,7 @@ static VALUE dd_cov_stop(VALUE self) if (dd_cov_data->threading_mode == SINGLE_THREADED_COVERAGE_MODE) { - // get current thread VALUE thval = rb_thread_current(); - // remove event hook for the current thread rb_thread_remove_event_hook(thval, dd_cov_update_coverage); } else From f6a4e073b5a15895ded0ac0bebe58707928b3b89 Mon Sep 17 00:00:00 2001 From: Andrey Date: Fri, 7 Jun 2024 11:03:37 +0200 Subject: [PATCH 4/9] add one more test case that confirms that background jobs are covered by multithreading code coverage --- spec/datadog/ci/worker_spec.rb | 2 +- spec/ddcov/ddcov_spec.rb | 31 +++++++++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/spec/datadog/ci/worker_spec.rb b/spec/datadog/ci/worker_spec.rb index ce118761..d55db070 100644 --- a/spec/datadog/ci/worker_spec.rb +++ b/spec/datadog/ci/worker_spec.rb @@ -24,7 +24,7 @@ context "when the worker has started" do context "when the worker is running" do - let(:queue) { Queue.new } + let(:queue) { Thread::Queue.new } subject(:worker) { described_class.new { queue.pop } } it do diff --git a/spec/ddcov/ddcov_spec.rb b/spec/ddcov/ddcov_spec.rb index 5832312f..a6c944d0 100644 --- a/spec/ddcov/ddcov_spec.rb +++ b/spec/ddcov/ddcov_spec.rb @@ -165,8 +165,8 @@ def thread_local_cov let(:threading_mode) { :single } it "collects coverage for each thread separately" do - t1_queue = Queue.new - t2_queue = Queue.new + t1_queue = Thread::Queue.new + t2_queue = Thread::Queue.new t1 = Thread.new do cov = thread_local_cov @@ -227,6 +227,33 @@ def thread_local_cov expect(coverage.keys).to include(absolute_path("calculator/operations/add.rb")) expect(coverage.keys).to include(absolute_path("calculator/operations/multiply.rb")) end + + it "collects coverage for background threads that started before the coverage collection" do + jobs_queue = Thread::Queue.new + background_jobs_worker = Thread.new do + loop do + job = jobs_queue.pop + break if job == :done + + job.call + end + end + + cov = described_class.new(root: root, threading_mode: :multi) + cov.start + + jobs_queue << -> { expect(calculator.add(1, 2)).to eq(3) } + jobs_queue << -> { expect(calculator.multiply(1, 2)).to eq(2) } + + jobs_queue << :done + + background_jobs_worker.join + + coverage = cov.stop + expect(coverage.size).to eq(2) + expect(coverage.keys).to include(absolute_path("calculator/operations/add.rb")) + expect(coverage.keys).to include(absolute_path("calculator/operations/multiply.rb")) + end end context "when threading mode is invalid" do From 18edce679f0b17bcb22cf08393d42f4a8169f280 Mon Sep 17 00:00:00 2001 From: Andrey Date: Fri, 7 Jun 2024 12:25:57 +0200 Subject: [PATCH 5/9] add DD_CIVISIBILITY_ITR_CODE_COVERAGE_USE_SINGLE_THREADED_MODE env variable to switch to single threaded code coverage mode if needed --- lib/datadog/ci/configuration/components.rb | 3 +- lib/datadog/ci/configuration/settings.rb | 6 ++ lib/datadog/ci/ext/settings.rb | 1 + lib/datadog/ci/itr/runner.rb | 13 +++- sig/datadog/ci/ext/settings.rbs | 1 + sig/datadog/ci/itr/runner.rbs | 5 +- .../minitest/helpers/addition_helper.rb | 5 ++ .../contrib/minitest/instrumentation_spec.rb | 60 ++++++++++++++++++- spec/support/contexts/ci_mode.rb | 2 + 9 files changed, 91 insertions(+), 5 deletions(-) create mode 100644 spec/datadog/ci/contrib/minitest/helpers/addition_helper.rb diff --git a/lib/datadog/ci/configuration/components.rb b/lib/datadog/ci/configuration/components.rb index a9f97891..4bc5579a 100644 --- a/lib/datadog/ci/configuration/components.rb +++ b/lib/datadog/ci/configuration/components.rb @@ -129,7 +129,8 @@ def activate_ci!(settings) config_tags: custom_configuration_tags, coverage_writer: coverage_writer, enabled: settings.ci.enabled && settings.ci.itr_enabled, - bundle_location: settings.ci.itr_code_coverage_excluded_bundle_path + bundle_location: settings.ci.itr_code_coverage_excluded_bundle_path, + use_single_threaded_coverage: settings.ci.itr_code_coverage_use_single_threaded_mode ) git_tree_uploader = Git::TreeUploader.new(api: test_visibility_api) diff --git a/lib/datadog/ci/configuration/settings.rb b/lib/datadog/ci/configuration/settings.rb index 27be078b..5caa6f41 100644 --- a/lib/datadog/ci/configuration/settings.rb +++ b/lib/datadog/ci/configuration/settings.rb @@ -76,6 +76,12 @@ def self.add_settings!(base) end end + option :itr_code_coverage_use_single_threaded_mode do |o| + o.type :bool + o.env CI::Ext::Settings::ENV_ITR_CODE_COVERAGE_USE_SINGLE_THREADED_MODE + o.default false + end + define_method(:instrument) do |integration_name, options = {}, &block| return unless enabled diff --git a/lib/datadog/ci/ext/settings.rb b/lib/datadog/ci/ext/settings.rb index 917fdb33..37b1e653 100644 --- a/lib/datadog/ci/ext/settings.rb +++ b/lib/datadog/ci/ext/settings.rb @@ -13,6 +13,7 @@ module Settings ENV_ITR_ENABLED = "DD_CIVISIBILITY_ITR_ENABLED" ENV_GIT_METADATA_UPLOAD_ENABLED = "DD_CIVISIBILITY_GIT_METADATA_UPLOAD_ENABLED" ENV_ITR_CODE_COVERAGE_EXCLUDED_BUNDLE_PATH = "DD_CIVISIBILITY_ITR_CODE_COVERAGE_EXCLUDED_BUNDLE_PATH" + ENV_ITR_CODE_COVERAGE_USE_SINGLE_THREADED_MODE = "DD_CIVISIBILITY_ITR_CODE_COVERAGE_USE_SINGLE_THREADED_MODE" # Source: https://docs.datadoghq.com/getting_started/site/ DD_SITE_ALLOWLIST = %w[ diff --git a/lib/datadog/ci/itr/runner.rb b/lib/datadog/ci/itr/runner.rb index e35c857d..b1ccfa31 100644 --- a/lib/datadog/ci/itr/runner.rb +++ b/lib/datadog/ci/itr/runner.rb @@ -31,7 +31,8 @@ def initialize( api: nil, coverage_writer: nil, enabled: false, - bundle_location: nil + bundle_location: nil, + use_single_threaded_coverage: false ) @enabled = enabled @api = api @@ -43,6 +44,7 @@ def initialize( else bundle_location end + @use_single_threaded_coverage = use_single_threaded_coverage @test_skipping_enabled = false @code_coverage_enabled = false @@ -186,12 +188,15 @@ def write(event) def coverage_collector Thread.current[:dd_coverage_collector] ||= Coverage::DDCov.new( root: Git::LocalRepository.root, - ignored_path: @bundle_location + ignored_path: @bundle_location, + threading_mode: code_coverage_mode ) end def load_datadog_cov! require "datadog_cov.#{RUBY_VERSION}_#{RUBY_PLATFORM}" + + Datadog.logger.debug("Loaded Datadog code coverage collector, using coverage mode: #{code_coverage_mode}") rescue LoadError => e Datadog.logger.error("Failed to load coverage collector: #{e}. Code coverage will not be collected.") @@ -222,6 +227,10 @@ def fetch_skippable_tests(test_session:, git_tree_upload_worker:) Datadog.logger.debug { "Found #{@skippable_tests.count} skippable tests." } Datadog.logger.debug { "ITR correlation ID: #{@correlation_id}" } end + + def code_coverage_mode + @use_single_threaded_coverage ? :single : :multi + end end end end diff --git a/sig/datadog/ci/ext/settings.rbs b/sig/datadog/ci/ext/settings.rbs index 83ed4fad..841134d2 100644 --- a/sig/datadog/ci/ext/settings.rbs +++ b/sig/datadog/ci/ext/settings.rbs @@ -10,6 +10,7 @@ module Datadog ENV_ITR_ENABLED: String ENV_GIT_METADATA_UPLOAD_ENABLED: String ENV_ITR_CODE_COVERAGE_EXCLUDED_BUNDLE_PATH: String + ENV_ITR_CODE_COVERAGE_USE_SINGLE_THREADED_MODE: String DD_SITE_ALLOWLIST: Array[String] end diff --git a/sig/datadog/ci/itr/runner.rbs b/sig/datadog/ci/itr/runner.rbs index 1ce20b44..7c4daae4 100644 --- a/sig/datadog/ci/itr/runner.rbs +++ b/sig/datadog/ci/itr/runner.rbs @@ -15,6 +15,7 @@ module Datadog @dd_env: String? @config_tags: Hash[String, String] @bundle_location: String? + @use_single_threaded_coverage: bool @skipped_tests_count: Integer @mutex: Thread::Mutex @@ -23,7 +24,7 @@ module Datadog attr_reader skipped_tests_count: Integer attr_reader correlation_id: String? - def initialize: (dd_env: String?, ?enabled: bool, ?coverage_writer: Datadog::CI::ITR::Coverage::Writer?, ?api: Datadog::CI::Transport::Api::Base?, ?config_tags: Hash[String, String]?, ?bundle_location: String?) -> void + def initialize: (dd_env: String?, ?enabled: bool, ?coverage_writer: Datadog::CI::ITR::Coverage::Writer?, ?api: Datadog::CI::Transport::Api::Base?, ?config_tags: Hash[String, String]?, ?bundle_location: String?, ?use_single_threaded_coverage: bool) -> void def configure: (Hash[String, untyped] remote_configuration, test_session: Datadog::CI::TestSession, git_tree_upload_worker: Datadog::CI::Worker) -> void @@ -58,6 +59,8 @@ module Datadog def fetch_skippable_tests: (test_session: Datadog::CI::TestSession, git_tree_upload_worker: Datadog::CI::Worker) -> void def increment_skipped_tests_counter: () -> void + + def code_coverage_mode: () -> Datadog::CI::ITR::Coverage::DDCov::threading_mode end end end diff --git a/spec/datadog/ci/contrib/minitest/helpers/addition_helper.rb b/spec/datadog/ci/contrib/minitest/helpers/addition_helper.rb new file mode 100644 index 00000000..f56753fe --- /dev/null +++ b/spec/datadog/ci/contrib/minitest/helpers/addition_helper.rb @@ -0,0 +1,5 @@ +module AdditionHelper + def self.add(a, b) + a + b + end +end diff --git a/spec/datadog/ci/contrib/minitest/instrumentation_spec.rb b/spec/datadog/ci/contrib/minitest/instrumentation_spec.rb index a1196b81..e5ee8fb9 100644 --- a/spec/datadog/ci/contrib/minitest/instrumentation_spec.rb +++ b/spec/datadog/ci/contrib/minitest/instrumentation_spec.rb @@ -27,7 +27,7 @@ def test_foo end end - context "with service name configured" do + context "with service name configured and code coverage enabled" do include_context "CI mode activated" do let(:integration_name) { :minitest } let(:integration_options) { {service_name: "ltest"} } @@ -402,12 +402,18 @@ def test_foo before(:context) do Minitest::Runnable.reset + require_relative "helpers/addition_helper" class SomeTest < Minitest::Test def test_pass assert true end def test_pass_other + # add thread to test that code coverage is collected + t = Thread.new do + AdditionHelper.add(1, 2) + end + t.join assert true end end @@ -494,6 +500,13 @@ def test_pass_other expect_coverage_events_belong_to_suite(first_test_suite_span) expect_coverage_events_belong_to_tests(test_spans) expect_non_empty_coverages + + # expect that background thread is covered + test_span = test_spans.find { |span| span.get_tag("test.name") == "test_pass_other" } + cov_event = find_coverage_for_test(test_span) + expect(cov_event.coverage.keys).to include( + File.expand_path(File.join(__dir__, "helpers/addition_helper.rb")) + ) end context "when ITR skips tests" do @@ -881,4 +894,49 @@ class SomeUnskippableSpec < Minitest::Spec end end end + + context "when using single threaded code coverage" do + include_context "CI mode activated" do + let(:integration_name) { :minitest } + + let(:itr_enabled) { true } + let(:code_coverage_enabled) { true } + let(:use_single_threaded_coverage) { true } + end + + before do + Minitest.run([]) + end + + before(:context) do + Thread.current[:dd_coverage_collector] = nil + + Minitest::Runnable.reset + + require_relative "helpers/addition_helper" + class SomeTestWithThreads < Minitest::Test + def test_with_background_thread + # add thread to test that code coverage is collected + t = Thread.new do + AdditionHelper.add(1, 2) + end + t.join + assert true + end + end + end + + it "does not cover the background thread" do + skip if PlatformHelpers.jruby? + + expect(test_spans).to have(1).item + expect(coverage_events).to have(1).item + + # expect that background thread is not covered + cov_event = find_coverage_for_test(first_test_span) + expect(cov_event.coverage.keys).not_to include( + File.expand_path(File.join(__dir__, "helpers/addition_helper.rb")) + ) + end + end end diff --git a/spec/support/contexts/ci_mode.rb b/spec/support/contexts/ci_mode.rb index f266c5c9..2b830136 100644 --- a/spec/support/contexts/ci_mode.rb +++ b/spec/support/contexts/ci_mode.rb @@ -23,6 +23,7 @@ let(:git_metadata_upload_enabled) { false } let(:require_git) { false } let(:bundle_path) { nil } + let(:use_single_threaded_coverage) { false } let(:itr_correlation_id) { "itr_correlation_id" } let(:itr_skippable_tests) { [] } @@ -77,6 +78,7 @@ c.ci.itr_enabled = itr_enabled c.ci.git_metadata_upload_enabled = git_metadata_upload_enabled c.ci.itr_code_coverage_excluded_bundle_path = bundle_path + c.ci.itr_code_coverage_use_single_threaded_mode = use_single_threaded_coverage unless integration_name == :no_instrument c.ci.instrument integration_name, integration_options end From 1d6553f523e5ffcc6cccb5e0c63197c8dc4cbf9a Mon Sep 17 00:00:00 2001 From: Andrey Date: Mon, 10 Jun 2024 13:49:19 +0200 Subject: [PATCH 6/9] fix rebase issue --- ext/datadog_cov/datadog_cov.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/ext/datadog_cov/datadog_cov.c b/ext/datadog_cov/datadog_cov.c index ed020e86..9e8cbb8a 100644 --- a/ext/datadog_cov/datadog_cov.c +++ b/ext/datadog_cov/datadog_cov.c @@ -194,9 +194,6 @@ static VALUE dd_cov_start(VALUE self) rb_add_event_hook(dd_cov_update_coverage, RUBY_EVENT_LINE, self); } - // add event hook - rb_thread_add_event_hook(thval, dd_cov_update_coverage, RUBY_EVENT_LINE, self); - return self; } From 573eeda66fe65d8a73c457a70205b18e2cef0b8e Mon Sep 17 00:00:00 2001 From: Andrey Date: Mon, 10 Jun 2024 13:57:59 +0200 Subject: [PATCH 7/9] make threading_mode mandatory argument for DDCov --- ext/datadog_cov/datadog_cov.c | 2 +- sig/datadog/ci/itr/coverage/ddcov.rbs | 2 +- spec/ddcov/ddcov_spec.rb | 3 ++- spec/ddcov/ddcov_with_symlinks_spec.rb | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/ext/datadog_cov/datadog_cov.c b/ext/datadog_cov/datadog_cov.c index 9e8cbb8a..c397ec50 100644 --- a/ext/datadog_cov/datadog_cov.c +++ b/ext/datadog_cov/datadog_cov.c @@ -94,7 +94,7 @@ static VALUE dd_cov_initialize(int argc, VALUE *argv, VALUE self) VALUE rb_threading_mode = rb_hash_lookup(opt, ID2SYM(rb_intern("threading_mode"))); int threading_mode; - if (!RTEST(rb_threading_mode) || rb_threading_mode == ID2SYM(rb_intern("multi"))) + if (rb_threading_mode == ID2SYM(rb_intern("multi"))) { threading_mode = MULTI_THREADED_COVERAGE_MODE; } diff --git a/sig/datadog/ci/itr/coverage/ddcov.rbs b/sig/datadog/ci/itr/coverage/ddcov.rbs index 0027dc6c..156789ff 100644 --- a/sig/datadog/ci/itr/coverage/ddcov.rbs +++ b/sig/datadog/ci/itr/coverage/ddcov.rbs @@ -5,7 +5,7 @@ module Datadog class DDCov type threading_mode = :multi | :single - def initialize: (root: String, ignored_path: String?, ?threading_mode: threading_mode?) -> void + def initialize: (root: String, ignored_path: String?, threading_mode: threading_mode) -> void def start: () -> void diff --git a/spec/ddcov/ddcov_spec.rb b/spec/ddcov/ddcov_spec.rb index a6c944d0..0eb8ca94 100644 --- a/spec/ddcov/ddcov_spec.rb +++ b/spec/ddcov/ddcov_spec.rb @@ -11,7 +11,8 @@ def absolute_path(path) end let(:ignored_path) { nil } - subject { described_class.new(root: root, ignored_path: ignored_path) } + let(:threading_mode) { :multi } + subject { described_class.new(root: root, ignored_path: ignored_path, threading_mode: threading_mode) } describe "code coverage collection" do let!(:calculator) { Calculator.new } diff --git a/spec/ddcov/ddcov_with_symlinks_spec.rb b/spec/ddcov/ddcov_with_symlinks_spec.rb index 052ca98b..c6bb0583 100644 --- a/spec/ddcov/ddcov_with_symlinks_spec.rb +++ b/spec/ddcov/ddcov_with_symlinks_spec.rb @@ -27,7 +27,7 @@ def absolute_path(path) ) end - subject { described_class.new(root: root) } + subject { described_class.new(root: root, threading_mode: :multi) } describe "code coverage collection" do let!(:calculator) { Calculator.new } From 020b5fccb43c29d26f1405ae11a25e55e3d9dd06 Mon Sep 17 00:00:00 2001 From: Andrey Date: Mon, 10 Jun 2024 14:23:16 +0200 Subject: [PATCH 8/9] raise runtime error if coverage is stopped by a different thread than one that started it --- ext/datadog_cov/datadog_cov.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/ext/datadog_cov/datadog_cov.c b/ext/datadog_cov/datadog_cov.c index c397ec50..aa32c426 100644 --- a/ext/datadog_cov/datadog_cov.c +++ b/ext/datadog_cov/datadog_cov.c @@ -31,6 +31,9 @@ struct dd_cov_data uintptr_t last_filename_ptr; + // for single threaded mode: thread that is being covered + VALUE th_covered; + int threading_mode; }; @@ -38,6 +41,7 @@ static void dd_cov_mark(void *ptr) { struct dd_cov_data *dd_cov_data = ptr; rb_gc_mark_movable(dd_cov_data->coverage); + rb_gc_mark_movable(dd_cov_data->th_covered); } static void dd_cov_free(void *ptr) @@ -52,6 +56,7 @@ static void dd_cov_compact(void *ptr) { struct dd_cov_data *dd_cov_data = ptr; dd_cov_data->coverage = rb_gc_location(dd_cov_data->coverage); + dd_cov_data->th_covered = rb_gc_location(dd_cov_data->th_covered); } const rb_data_type_t dd_cov_data_type = { @@ -188,6 +193,7 @@ static VALUE dd_cov_start(VALUE self) { VALUE thval = rb_thread_current(); rb_thread_add_event_hook(thval, dd_cov_update_coverage, RUBY_EVENT_LINE, self); + dd_cov_data->th_covered = thval; } else { @@ -205,7 +211,13 @@ static VALUE dd_cov_stop(VALUE self) if (dd_cov_data->threading_mode == SINGLE_THREADED_COVERAGE_MODE) { VALUE thval = rb_thread_current(); - rb_thread_remove_event_hook(thval, dd_cov_update_coverage); + if (!rb_equal(thval, dd_cov_data->th_covered)) + { + rb_raise(rb_eRuntimeError, "Coverage was not started by this thread"); + } + + rb_thread_remove_event_hook(dd_cov_data->th_covered, dd_cov_update_coverage); + dd_cov_data->th_covered = Qnil; } else { From 19e5d8b370a02aacad56345840255e3f6267333d Mon Sep 17 00:00:00 2001 From: Andrey Date: Mon, 10 Jun 2024 15:14:35 +0200 Subject: [PATCH 9/9] make sure DDCov does not track code coverage when stopped in single threaded mode --- spec/ddcov/ddcov_spec.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/spec/ddcov/ddcov_spec.rb b/spec/ddcov/ddcov_spec.rb index 0eb8ca94..70873407 100644 --- a/spec/ddcov/ddcov_spec.rb +++ b/spec/ddcov/ddcov_spec.rb @@ -255,6 +255,20 @@ def thread_local_cov expect(coverage.keys).to include(absolute_path("calculator/operations/add.rb")) expect(coverage.keys).to include(absolute_path("calculator/operations/multiply.rb")) end + + it "does not track coverage when stopped" do + subject.start + expect(calculator.add(1, 2)).to eq(3) + subject.stop + + expect(calculator.subtract(1, 2)).to eq(-1) + + subject.start + expect(calculator.multiply(1, 2)).to eq(2) + coverage = subject.stop + expect(coverage.size).to eq(1) + expect(coverage.keys).to include(absolute_path("calculator/operations/multiply.rb")) + end end context "when threading mode is invalid" do