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

[SDTEST-744] add memory leaks tests with ruby_memcheck #246

Merged
merged 3 commits into from
Oct 8, 2024

Conversation

anmarchenko
Copy link
Member

@anmarchenko anmarchenko commented Oct 8, 2024

What does this PR do?
Adds https://github.com/Shopify/ruby_memcheck gem to automatically find and report memory leaks using valgrind. This PR mostly copies the setup from dd-trace-rb which was done in DataDog/dd-trace-rb#3852

Motivation
Prevent memory leaks from the native extension

How to test the change?
Github Actions workflow is added

@codecov-commenter
Copy link

codecov-commenter commented Oct 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.86%. Comparing base (c12ad86) to head (157b0d8).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #246   +/-   ##
=======================================
  Coverage   98.86%   98.86%           
=======================================
  Files         279      279           
  Lines       13603    13603           
  Branches      637      637           
=======================================
  Hits        13449    13449           
  Misses        154      154           

☔ 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 October 8, 2024 12:27
@anmarchenko anmarchenko requested review from a team as code owners October 8, 2024 12:27
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.

👍 LGTM

bundler-cache: true # runs 'bundle install' and caches installed gems automatically
bundler: latest
cache-version: v1 # bump this to invalidate cache
- run: sudo apt install -y valgrind && valgrind --version
Copy link
Member

Choose a reason for hiding this comment

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

You may want to borrow some of the tweaks we've done since to avoid flakiness in installing valgrind: https://github.com/DataDog/dd-trace-rb/blob/master/.github/workflows/test-memory-leaks.yaml#L14

Comment on lines +37 to +53
# We don't care about the pkg-config external tool
{
pkg-config-memory
Memcheck:Leak
...
obj:/usr/bin/pkg-config
...
}

# We don't care about the tr external tool
{
pkg-config-memory
Memcheck:Leak
...
obj:/usr/bin/tr
...
}
Copy link
Member

Choose a reason for hiding this comment

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

I suspect you won't need these, but also they're harmless to keep, if you want to just keep this file mostly in sync with dd-trace-rb

@anmarchenko anmarchenko merged commit bbc6b66 into main Oct 8, 2024
31 checks passed
@anmarchenko anmarchenko deleted the anmarchenko/memcheck_tests branch October 8, 2024 12:45
@github-actions github-actions bot added this to the 1.8.0 milestone Oct 8, 2024
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.

3 participants