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-2892] datadog_cov native extension for per test code coverage #137

Merged
merged 39 commits into from
Mar 21, 2024

Conversation

anmarchenko
Copy link
Member

@anmarchenko anmarchenko commented Mar 14, 2024

What does this PR do?
Adds datadog_cov native extension for per test code coverage.

Usage of Datadog::CI::ITR::Coverage::DDCov:

# track files coverage in the current folder
cov = Datadog::CI::ITR::Coverage::DDCov.new(root: Dir.pwd)
cov.start_coverage
Calculator.new.add(1, 2) # => 3
cov.stop_coverage # => { "/path/to/current-folder/calculator.rb" => true }

# track lines coverage
cov = Datadog::CI::ITR::Coverage::DDCov.new(root: Dir.pwd, mode: :lines)
cov.start_coverage
Calculator.new.add(1, 2) # => 3
cov.stop_coverage # => { "/path/to/current-folder/calculator.rb" => {12 => true, 13 => true} }

Additional Notes
Current limitations:

  • UTF-8 in source file paths is not supported because of additional overhead of parsing UTF-8 strings: will be added later as an opt-in via environment variable as non-ASCII characters in paths and filenames are rare
  • Lines coverage is barebones and not optimized: ITR will use only files coverage in the first stage

How to test the change?
Unit tests are provided in spec/datadog/ci/cov_spec.rb

Scenarios tested:

  • coverage with different root values
  • root folder path is longer than covered files' paths
  • coverage is not collected after stop_coverage call
  • coverage of mixed in code from helpers
  • multi threaded coverage (coverage is collected per thread)
  • UTF-8 in file names does not fail (but produces incorrect result)

@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2024

Codecov Report

Attention: Patch coverage is 99.57447% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 99.22%. Comparing base (6a994b6) to head (e4e86f3).

Files Patch % Lines
lib/datadog/ci/utils/git.rb 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              1.0     #137      +/-   ##
==========================================
+ Coverage   99.19%   99.22%   +0.02%     
==========================================
  Files         163      171       +8     
  Lines        7240     7468     +228     
  Branches      302      310       +8     
==========================================
+ Hits         7182     7410     +228     
  Misses         58       58              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anmarchenko anmarchenko marked this pull request as ready for review March 15, 2024 14:00
@anmarchenko anmarchenko requested review from a team as code owners March 15, 2024 14:00
@anmarchenko anmarchenko force-pushed the anmarchenko/ddcov_per_test_coverage branch from c1ab331 to 29a064b Compare March 18, 2024 15:16
Copy link
Contributor

@juan-fernandez juan-fernandez left a comment

Choose a reason for hiding this comment

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

🎉 very nice!

Copy link
Member

@tonyredondo tonyredondo left a comment

Choose a reason for hiding this comment

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

LGTM, I left a couple of nits, and I have a question regarding what happens if a test spawn new threads, is that possible? how the code coverage works in that case?

ext/datadog_cov/datadog_cov.c Show resolved Hide resolved
.gitignore Show resolved Hide resolved
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Left a few notes! :)

ext/datadog_cov/extconf.rb Show resolved Hide resolved
ext/datadog_cov/datadog_cov.c Show resolved Hide resolved
ext/datadog_cov/datadog_cov.c Outdated Show resolved Hide resolved
ext/datadog_cov/datadog_cov.c Outdated Show resolved Hide resolved
spec/cov/cov_spec.rb Outdated Show resolved Hide resolved
spec/cov/cov_spec.rb Outdated Show resolved Hide resolved
spec/datadog/ci/itr/runner_spec.rb Outdated Show resolved Hide resolved
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Ahaha clearly my head kept reviewing the code as an async background task and when I took a break to go get some tea the async task resolved and pointed out these two issues 🤣

ext/datadog_cov/datadog_cov.c Outdated Show resolved Hide resolved
ext/datadog_cov/datadog_cov.c Outdated Show resolved Hide resolved
@anmarchenko anmarchenko force-pushed the anmarchenko/ddcov_per_test_coverage branch from 2f21495 to 09c27e5 Compare March 20, 2024 11:36
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

Left a few more notes, I think you're very close ;)

One thing that occurred to me is that we have a weird setup for packaging dd-trace-rb when used by single step instrumentation (sometimes known as "lib injection" in dd-trace-rb).

TL;DR we handle gems with native extensions in a specific way, so if this native gem is still going to become a dependency of ddtrace 1.x, it's probably worth checking nothing is broken there.

I'm not entirely sure of all the details, @TonyCTHsu is the right person to ask "if this new native extension going to break or not".

ext/datadog_cov/datadog_cov.c Show resolved Hide resolved
ext/datadog_cov/datadog_cov.c Outdated Show resolved Hide resolved
ext/datadog_cov/datadog_cov.c Outdated Show resolved Hide resolved
@anmarchenko anmarchenko force-pushed the anmarchenko/ddcov_per_test_coverage branch from 483aff8 to 2d4f694 Compare March 20, 2024 15:51
@anmarchenko anmarchenko changed the base branch from main to 1.0 March 20, 2024 15:51
@anmarchenko
Copy link
Member Author

This pull request is now rebased on top of 1.0 branch and the base is set to 1.0.

From now on, all ITR functionality will go into 1.0 branch so that we don't add additional risks for ddtrace 1.x gem.

@anmarchenko anmarchenko added this to the 1.0 milestone Mar 21, 2024
@anmarchenko
Copy link
Member Author

@ivoanjo this is ready for another pass

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

👍 Native bits look good to me! I didn't re-review the other things that came in from the rebase/changing the branch :)

@anmarchenko anmarchenko merged commit 61be62e into 1.0 Mar 21, 2024
23 checks passed
@anmarchenko anmarchenko deleted the anmarchenko/ddcov_per_test_coverage branch March 21, 2024 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants