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

DEBUG-2507 Test individual product loading #3738

Merged
merged 10 commits into from
Jun 27, 2024
Merged

Conversation

p-datadog
Copy link
Contributor

Replacement for #3646.

This PR tests that each product can be loaded by itself, and repairs appsec to be loadable without tracing having already been loaded.

The tests are redone to use system rather than forking and no longer require a separate job.

@p-datadog p-datadog requested review from a team as code owners June 25, 2024 17:39
@github-actions github-actions bot added appsec Application Security monitoring product integrations Involves tracing integrations labels Jun 25, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.10%. Comparing base (154f72e) to head (d4189c3).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3738      +/-   ##
==========================================
- Coverage   98.10%   98.10%   -0.01%     
==========================================
  Files        1227     1228       +1     
  Lines       73014    73027      +13     
  Branches     3508     3508              
==========================================
+ Hits        71633    71645      +12     
- Misses       1381     1382       +1     

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

Copy link
Member

@marcotc marcotc left a comment

Choose a reason for hiding this comment

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

Awesome clean up!

Copy link
Contributor

@TonyCTHsu TonyCTHsu left a comment

Choose a reason for hiding this comment

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

How is this PR different from #3646 ?

@@ -0,0 +1,39 @@
require 'English'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this require 'English' ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it is necessary now, removed.

@p-datadog
Copy link
Contributor Author

@TonyCTHsu

The tests are redone to use system rather than forking and no longer require a separate job.

@p-datadog
Copy link
Contributor Author

Also the PR is a new one because the branch is in the main repo, not in a fork for CI reasons.

@p-datadog p-datadog requested a review from TonyCTHsu June 26, 2024 16:11
Copy link
Contributor

@TonyCTHsu TonyCTHsu left a comment

Choose a reason for hiding this comment

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

The test LGTM. Not entirely sure about the appsec changes.

@@ -1,6 +1,6 @@
# frozen_string_literal: true

require_relative '../../../tracing/contrib/rack/middlewares'
require_relative '../../../tracing/contrib'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Individual contribs require the REGISTRY constant defined by top-level contrib.

@@ -1,6 +1,7 @@
# frozen_string_literal: true

require_relative 'configuration'
require_relative '../core/configuration'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a dependency on Core::Configuration in this file.

@p-datadog
Copy link
Contributor Author

@TonyCTHsu I added comments for the appsec changes, if there is a different solution that would also be fine with me.

@p-datadog p-datadog merged commit 40c0118 into master Jun 27, 2024
166 checks passed
@p-datadog p-datadog deleted the require-encapsulation branch June 27, 2024 16:22
@github-actions github-actions bot added this to the 2.2.0 milestone Jun 27, 2024
end

it 'loads successfully by itself' do
rv = system("ruby -e #{Shellwords.shellescape(code)}")
Copy link
Member

Choose a reason for hiding this comment

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

Hey I didn't know shellescape existed! This is cool!

@p-datadog p-datadog changed the title Test individual product loading DEBUG-2507 Test individual product loading Jul 1, 2024
p-datadog pushed a commit to p-datadog/dd-trace-rb that referenced this pull request Jul 1, 2024
* master: (54 commits)
  Update gemfiles/*
  Generate Ruby 3.4 gemfiles and lockfiles
  Add Ruby 3.4 to appraisals
  use method instead of instance variable
  fix flaky test
  fix deadlock issue: don't flush_events in main thread when calling stop, override #work_pending? instead
  Remove useless (and conflicting) constant
  Disable Rubocop warning about encoding
  Declare encoding as UTF-8
  Adjust test to take new file into account
  Extract matrix from Rakefile
  Test individual product loading (DataDog#3738)
  Refactor
  Replace show command with select_spec without regex match
  Fix false positive of show command
  Prevent overwriting Gemfile
  Remove unnecessary output
  Invoke with `library_entrypoint
  Fix events array
  Implement telemetry
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appsec Application Security monitoring product integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants