[CIVIS-9951] add settings option to ignore code coverage for bundled gems location #174
Conversation
anmarchenko
changed the title
anmarchenko
force-pushed
the
anmarchenko/skip_per_test_coverage_for_bundled_gems
branch
from
a4961a4
to
16b04ed
Compare
…single itr_code_coverage_excluded_bundle_path
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #174 +/- ##
==========================================
- Coverage 98.98% 98.98% -0.01%
==========================================
Files 198 205 +7
Lines 9637 9816 +179
Branches 423 435 +12
==========================================
+ Hits 9539 9716 +177
- Misses 98 100 +2 ☔ View full report in Codecov by Sentry. |
| 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree! If it's not blocking I would merge this one as is, and then optimized it together with everything else in #171
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sounds reasonable, I'll defer to your wise judgment here :)
anmarchenko
changed the title
| 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; |
There was a problem hiding this comment.
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.
anmarchenko
deleted the
anmarchenko/skip_per_test_coverage_for_bundled_gems
branch
What does this PR do?
In CI environments bundled gems are often installed inside the project folder for caching purposes. Unfortunately, it makes per test code coverage for ITR extremely slow as we need to unnecessary track all the code files from the external gems.
This PR adds
DD_CIVISIBILITY_ITR_CODE_COVERAGE_EXCLUDED_BUNDLE_PATHenvironment variable that allows us to set a certain path to be excluded from code coverage (supports both relative and absolute paths). Even better, for the most cases users won't have to use this configuration: we will locate bundle location and set it for them.It works as following:
bundlergem is available in the environment, we useBundler.bundle_pathand set this result as ignored pathvendor/bundleor.bundleunder the git root - if any of them exist, we ignore themHow to test the change?
Unit tests are provided.
I have run test-environment with gems bundled inside the project directory to confirm the fix:
https://github.com/DataDog/test-environment/pull/284
The overhead with bundling gems in the project folder in this case is insignificantly larger: 113% vs 106% for rubocop for example. Without this change, it would be 450% vs 106%.