Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CIVIS-9951] add settings option to ignore code coverage for bundled gems location #174

Merged
merged 6 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Steepfile
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,5 @@ target :lib do
library "msgpack"
library "ci_queue"
library "knapsack_pro"
library "bundler"
end
46 changes: 36 additions & 10 deletions ext/datadog_cov/datadog_cov.c
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,35 @@
#define DD_COV_TARGET_FILES 1
#define DD_COV_TARGET_LINES 2

static int is_prefix(VALUE prefix, const char *str)
{
if (prefix == Qnil)
{
return 0;
}

const char *c_prefix = RSTRING_PTR(prefix);
if (c_prefix == NULL)
{
return 0;
}

long prefix_len = RSTRING_LEN(prefix);
if (strncmp(c_prefix, str, prefix_len) == 0)
{
return 1;
}
else
{
return 0;
}
}

// Data structure
struct dd_cov_data
{
VALUE root;
VALUE ignored_path;
Comment on lines +8 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I guess the same suggestion from #171 would apply here -- keeping the prefix as a char * and caching its length would save us some per-line work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! If it's not blocking I would merge this one as is, and then optimized it together with everything else in #171

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think it's a blocker. I guess it's a slight performance regression until it's paired with #171, but I'm guessing the plan will be to release 'em together :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will have to release it earlier than the performance improvements, but there is an important customer value in this: we have a customer that evaluates ITR and current implementation that does not ignore bundled gems installed in project folder induces up to 400% code coverage overhead.

I measured ineffectiveness of this solution using test-environment: for the worst case it increases overhead by 7% (113 vs 106) if bundled gems are installed in project folder. When gems are not installed in project folder, this change does not induce any overhead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable, I'll defer to your wise judgment here :)

int mode;
VALUE coverage;
};
Expand All @@ -18,6 +43,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->root);
rb_gc_mark_movable(dd_cov_data->ignored_path);
}

static void dd_cov_free(void *ptr)
Expand All @@ -32,6 +58,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->root = rb_gc_location(dd_cov_data->root);
dd_cov_data->ignored_path = rb_gc_location(dd_cov_data->ignored_path);
}

const rb_data_type_t dd_cov_data_type = {
Expand All @@ -49,6 +76,7 @@ static VALUE dd_cov_allocate(VALUE klass)
VALUE obj = TypedData_Make_Struct(klass, struct dd_cov_data, &dd_cov_data_type, dd_cov_data);
dd_cov_data->coverage = rb_hash_new();
dd_cov_data->root = Qnil;
dd_cov_data->ignored_path = Qnil;
dd_cov_data->mode = DD_COV_TARGET_FILES;
return obj;
}
Expand All @@ -66,6 +94,8 @@ static VALUE dd_cov_initialize(int argc, VALUE *argv, VALUE self)
rb_raise(rb_eArgError, "root is required");
}

VALUE rb_ignored_path = rb_hash_lookup(opt, ID2SYM(rb_intern("ignored_path")));

VALUE rb_mode = rb_hash_lookup(opt, ID2SYM(rb_intern("mode")));
if (!RTEST(rb_mode) || rb_mode == ID2SYM(rb_intern("files")))
{
Expand All @@ -84,6 +114,7 @@ static VALUE dd_cov_initialize(int argc, VALUE *argv, VALUE self)
TypedData_Get_Struct(self, struct dd_cov_data, &dd_cov_data_type, dd_cov_data);

dd_cov_data->root = rb_root;
dd_cov_data->ignored_path = rb_ignored_path;
dd_cov_data->mode = mode;

return Qnil;
Expand All @@ -100,20 +131,15 @@ static void dd_cov_update_line_coverage(rb_event_flag_t event, VALUE data, VALUE
return;
}

if (dd_cov_data->root == Qnil)
// if given filename is not located under the root, we skip it
if (is_prefix(dd_cov_data->root, filename) == 0)
{
return;
}

char *c_root = RSTRING_PTR(dd_cov_data->root);
if (c_root == NULL)
{
return;
}
long root_len = RSTRING_LEN(dd_cov_data->root);
// check that root is a prefix of the filename
// so this file is located under the given root
if (strncmp(c_root, filename, root_len) != 0)
// if ignored_path is provided and given filename is located under the ignored_path, we skip it too
// this is useful for ignoring bundled gems location
if (RTEST(dd_cov_data->ignored_path) && is_prefix(dd_cov_data->ignored_path, filename) == 1)
{
return;
}
Expand Down
3 changes: 2 additions & 1 deletion lib/datadog/ci/configuration/components.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ def activate_ci!(settings)
dd_env: settings.env,
config_tags: custom_configuration_tags,
coverage_writer: coverage_writer,
enabled: settings.ci.enabled && settings.ci.itr_enabled
enabled: settings.ci.enabled && settings.ci.itr_enabled,
bundle_location: settings.ci.itr_code_coverage_excluded_bundle_path
)

git_tree_uploader = Git::TreeUploader.new(api: test_visibility_api)
Expand Down
12 changes: 12 additions & 0 deletions lib/datadog/ci/configuration/settings.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

require_relative "../ext/settings"
require_relative "../utils/bundle"

module Datadog
module CI
Expand Down Expand Up @@ -67,6 +68,14 @@ def self.add_settings!(base)
o.default true
end

option :itr_code_coverage_excluded_bundle_path do |o|
o.type :string, nilable: true
o.env CI::Ext::Settings::ENV_ITR_CODE_COVERAGE_EXCLUDED_BUNDLE_PATH
o.default do
Datadog::CI::Utils::Bundle.location
end
end

define_method(:instrument) do |integration_name, options = {}, &block|
return unless enabled

Expand All @@ -89,6 +98,9 @@ def self.add_settings!(base)
fetch_integration(integration_name).configuration
end

# @deprecated Will be removed on datadog-ci-rb 1.0.
anmarchenko marked this conversation as resolved.
Show resolved Hide resolved
alias_method :use, :instrument

option :trace_flush

option :writer_options do |o|
Expand Down
5 changes: 5 additions & 0 deletions lib/datadog/ci/ext/environment.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,11 @@ module Environment
TAG_NODE_NAME = "ci.node.name"
TAG_CI_ENV_VARS = "_dd.ci.env_vars"

POSSIBLE_BUNDLE_LOCATIONS = [
"vendor/bundle",
".bundle"
].freeze

module_function

def tags(env)
Expand Down
1 change: 1 addition & 0 deletions lib/datadog/ci/ext/settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ module Settings
ENV_FORCE_TEST_LEVEL_VISIBILITY = "DD_CIVISIBILITY_FORCE_TEST_LEVEL_VISIBILITY"
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"

# Source: https://docs.datadoghq.com/getting_started/site/
DD_SITE_ALLOWLIST = [
Expand Down
16 changes: 13 additions & 3 deletions lib/datadog/ci/itr/runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,27 @@ def initialize(
config_tags: {},
api: nil,
coverage_writer: nil,
enabled: false
enabled: false,
bundle_location: nil
)
@enabled = enabled
@api = api
@dd_env = dd_env
@config_tags = config_tags || {}

@bundle_location = if bundle_location && !File.absolute_path?(bundle_location)
File.join(Git::LocalRepository.root, bundle_location)
else
bundle_location
end

@test_skipping_enabled = false
@code_coverage_enabled = false

@coverage_writer = coverage_writer

@correlation_id = nil
@skippable_tests = []
@skippable_tests = Set.new

@skipped_tests_count = 0
@mutex = Mutex.new
Expand Down Expand Up @@ -177,7 +184,10 @@ def write(event)
end

def coverage_collector
Thread.current[:dd_coverage_collector] ||= Coverage::DDCov.new(root: Git::LocalRepository.root)
Thread.current[:dd_coverage_collector] ||= Coverage::DDCov.new(
root: Git::LocalRepository.root,
ignored_path: @bundle_location
)
end

def load_datadog_cov!
Expand Down
26 changes: 26 additions & 0 deletions lib/datadog/ci/utils/bundle.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
# frozen_string_literal: true

require_relative "../ext/environment"
require_relative "../git/local_repository"

module Datadog
module CI
module Utils
module Bundle
def self.location
require "bundler"
bundle_path = Bundler.bundle_path.to_s
bundle_path if bundle_path&.start_with?(Datadog::CI::Git::LocalRepository.root)
rescue => e
Datadog.logger.warn("Failed to find bundled gems location: #{e}")

Ext::Environment::POSSIBLE_BUNDLE_LOCATIONS.each do |location|
path = File.join(Datadog::CI::Git::LocalRepository.root, location)
return path if File.directory?(path)
end
nil
end
end
end
end
end
2 changes: 2 additions & 0 deletions sig/datadog/ci/ext/environment.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ module Datadog

TAG_CI_ENV_VARS: String

POSSIBLE_BUNDLE_LOCATIONS: Array[String]

PROVIDERS: ::Array[Array[String | Symbol]]

def self?.tags: (untyped env) -> Hash[String, String]
Expand Down
1 change: 1 addition & 0 deletions sig/datadog/ci/ext/settings.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ module Datadog
ENV_FORCE_TEST_LEVEL_VISIBILITY: String
ENV_ITR_ENABLED: String
ENV_GIT_METADATA_UPLOAD_ENABLED: String
ENV_ITR_CODE_COVERAGE_EXCLUDED_BUNDLE_PATH: String

DD_SITE_ALLOWLIST: Array[String]
end
Expand Down
2 changes: 1 addition & 1 deletion sig/datadog/ci/itr/coverage/ddcov.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module Datadog
module Coverage
class DDCov

def initialize: (root: String, ?mode: Symbol) -> void
def initialize: (root: String, ?mode: Symbol, ignored_path: String?) -> void

def start: () -> void

Expand Down
3 changes: 2 additions & 1 deletion sig/datadog/ci/itr/runner.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ module Datadog
@api: Datadog::CI::Transport::Api::Base?
@dd_env: String?
@config_tags: Hash[String, String]
@bundle_location: String?

@skipped_tests_count: Integer
@mutex: Thread::Mutex
Expand All @@ -22,7 +23,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]?) -> 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?) -> void

def configure: (Hash[String, untyped] remote_configuration, test_session: Datadog::CI::TestSession, git_tree_upload_worker: Datadog::CI::Worker) -> void

Expand Down
9 changes: 9 additions & 0 deletions sig/datadog/ci/utils/bundle.rbs
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module Datadog
module CI
module Utils
module Bundle
def self.location: () -> String?
end
end
end
end
48 changes: 48 additions & 0 deletions spec/datadog/ci/configuration/settings_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,54 @@ def patcher
end
end

describe "#itr_code_coverage_excluded_bundle_path" do
subject(:itr_code_coverage_excluded_bundle_path) do
settings.ci.itr_code_coverage_excluded_bundle_path
end

it { is_expected.to be nil }

context "when #{Datadog::CI::Ext::Settings::ENV_ITR_CODE_COVERAGE_EXCLUDED_BUNDLE_PATH}" do
around do |example|
ClimateControl.modify(
Datadog::CI::Ext::Settings::ENV_ITR_CODE_COVERAGE_EXCLUDED_BUNDLE_PATH => path
) do
example.run
end
end

context "is not defined" do
let(:path) { nil }

it { is_expected.to be nil }

context "and when bundle location is found in project folder" do
let(:bundle_location) { "/path/to/repo/vendor/bundle" }
before do
allow(Datadog::CI::Utils::Bundle).to receive(:location).and_return(bundle_location)
end

it { is_expected.to eq bundle_location }
end
end

context "is set to some value" do
let(:path) { "/path/to/excluded" }

it { is_expected.to eq path }
end
end
end

describe "#itr_code_coverage_excluded_bundle_path=" do
it "updates the #enabled setting" do
expect { settings.ci.itr_code_coverage_excluded_bundle_path = "/path/to/excluded" }
.to change { settings.ci.itr_code_coverage_excluded_bundle_path }
.from(nil)
.to("/path/to/excluded")
end
end

describe "#instrument" do
let(:integration_name) { :fake }

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module Helper
def self.help?
true
end
end
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
require_relative "helpers/helper"

Then "datadog" do
true
Helper.help?
end

Then "failure" do
Expand Down
3 changes: 2 additions & 1 deletion spec/datadog/ci/contrib/cucumber/instrumentation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
let(:itr_enabled) { true }
let(:code_coverage_enabled) { true }
let(:tests_skipping_enabled) { true }
let(:bundle_path) { "step_definitions/helpers" }
end

let(:cucumber_8_or_above) { Gem::Version.new("8.0.0") <= Datadog::CI::Contrib::Cucumber::Integration.version }
Expand Down Expand Up @@ -196,7 +197,7 @@
context "collecting coverage with features dir as root" do
before { skip if PlatformHelpers.jruby? }

it "creates coverage events for each non-skipped test" do
it "creates coverage events for each non-skipped test ignoring bundle_path" do
expect(coverage_events).to have(1).item

expect_coverage_events_belong_to_session(test_session_span)
Expand Down
Loading
Loading