Check require encapsulation of components #3646
Conversation
github-actions
bot
added
appsec
p-datadog
force-pushed
the
require-encapsulation
branch
from
69fb0d5
to
3e7edf7
Compare
| @@ -0,0 +1,31 @@ | |||
| require 'English' | |||
There was a problem hiding this comment.
Filename should be _spec.rb
| it 'loads' do | ||
| pid = fork do | ||
| require req | ||
| exec('true') |
| ruby-version: '3.2' | ||
| bundler-cache: true # runs 'bundle install' and caches installed gems automatically | ||
| - run: rm -f .rspec | ||
| - run: bundle exec rspec spec/require_encapsulation_check.rb |
There was a problem hiding this comment.
What's the rationale for a separate job?
If the file is named correctly it can be part of the usual rake spec tasks and tested across the matrix.
| REQUIRES = %w[ | ||
| datadog/appsec | ||
| datadog/core | ||
| datadog/kit | ||
| datadog/profiling | ||
| datadog/tracing | ||
| ].freeze |
There was a problem hiding this comment.
This constant will be leaking globally, as is it should probably go into the RSpec.describe block.
Could even be a plain local variable, or even just %w[ ... ].each do |req|.
| datadog/tracing | ||
| ].freeze | ||
|
|
||
| RSpec.describe 'require encapsulation' do |
There was a problem hiding this comment.
In a way I feel this description isn't quite correct, which led me to think that this spec isn't quite how we expect it to be.
Notably it puts some cross-component knowledge into that spec file. I think it might be better if each "require "works is isolated in each separate component:
- assert that
require datadog/coreworks => inspec/datadog/core_spec.rb - assert that
require datadog/appsecworks => inspec/datadog/appsec_spec.rb - ... and so on
| @@ -1,5 +1,6 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require_relative '../../../tracing/contrib' | |||
There was a problem hiding this comment.
I'm not sure about this require here: this integration file doesn't itself seem to need it.
| @@ -11,6 +11,7 @@ | |||
| require_relative 'gateway/route_params' | |||
| require_relative 'gateway/request' | |||
| require_relative '../../../tracing/contrib/sinatra/framework' | |||
| require_relative '../../../tracing/contrib' | |||
There was a problem hiding this comment.
That's a good catch!
But AppSec should be able to load without Tracing being present, so I think instead there's a bit of conditioning around the Tracing constants to be made below.
| REQUIRES.each do |req| | ||
| context req do | ||
| it 'loads' do | ||
| pid = fork do |
There was a problem hiding this comment.
fork isn't available on Windows (which we don't support.. yet. I think it's a worthy goal) but more importantly it's not available on JRuby, for which we want coverage as well.
| @@ -0,0 +1,31 @@ | |||
| require 'English' | |||
There was a problem hiding this comment.
require 'English'
That's in order to have $CHILD_STATUS instead of $??`
|
New PR: #3738 |
What does this PR do?
This PR adds tests that requiring individual components is successful. It also adds some requires that were missing in
appsecto make the tests pass.Motivation:
Troubleshooting configuration for debugging which I copied from appsec, appsec itself does not load cleanly.
Additional Notes:
How to test the change?
Added a unit test and a GH configuration to run it (since it needs a clean environment).
Unsure? Have a question? Request a review!