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.

Sign up for GitHub

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

Jump to bottom

Check require encapsulation of components #3646

Closed
wants to merge 5 commits into from
Closed

Check require encapsulation of components #3646

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e8c0741
require encapsulation test
p May 14, 2024
d70f956
add missing requires to appsec
p May 14, 2024
3e7edf7
run on push?
p May 14, 2024
07072b9
need ruby
p May 14, 2024
c9a2998
rubocop
p May 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions .github/workflows/require-encapsulation.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
name: Check require encapsulation

on:
push:
branches:
- "**"
pull_request:
# The branches below must be a subset of the branches above
branches: [ master ]

jobs:
check-require-encapsulation:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: ruby/setup-ruby@v1
with:
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

1 change: 1 addition & 0 deletions lib/datadog/appsec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true

require_relative 'core/configuration'
require_relative 'appsec/configuration'
require_relative 'appsec/extensions'
require_relative 'appsec/scope'
Expand Down
1 change: 1 addition & 0 deletions lib/datadog/appsec/contrib/sinatra/integration.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true

require_relative '../../../tracing/contrib'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this require here: this integration file doesn't itself seem to need it.

require_relative '../integration'

require_relative 'patcher'
Expand Down
1 change: 1 addition & 0 deletions lib/datadog/appsec/contrib/sinatra/patcher.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
require_relative 'gateway/route_params'
require_relative 'gateway/request'
require_relative '../../../tracing/contrib/sinatra/framework'
require_relative '../../../tracing/contrib'
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


module Datadog
module AppSec
Expand Down
31 changes: 31 additions & 0 deletions spec/require_encapsulation_check.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
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.

Filename should be _spec.rb

Copy link
Contributor

Choose a reason for hiding this comment

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

require 'English'

That's in order to have $CHILD_STATUS instead of $??`

REQUIRES = %w[
datadog/appsec
datadog/core
datadog/kit
datadog/profiling
datadog/tracing
].freeze
Comment on lines +2 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

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|.


RSpec.describe 'require encapsulation' do
Copy link
Contributor

Choose a reason for hiding this comment

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

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/core works => in spec/datadog/core_spec.rb
  • assert that require datadog/appsec works => in spec/datadog/appsec_spec.rb
  • ... and so on

before(:all) do
# Permit Datadog::VERSION to be defined but no other constants.
# See the note in gemspec about requiring 'datadog/version'.
expect(defined?(Datadog)).to eq 'constant'
expect(Datadog.constants).to eq([:VERSION])
end

REQUIRES.each do |req|
context req do
it 'loads' do
pid = fork do
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

require req
exec('true')
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you tricky ;)

end

Process.waitpid(pid)
expect($CHILD_STATUS.exitstatus).to be 0
end
end
end
end
Loading