Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion lib/openapi_first/test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,11 @@ def self.setup

@exit_handler = method(:handle_exit)

main_process = Process.pid
@setup ||= at_exit do
# :nocov:
@exit_handler&.call
# Only handle exit once in the main process
@exit_handler&.call if Process.pid == main_process
# :nocov:
end
end
Expand Down
65 changes: 54 additions & 11 deletions lib/openapi_first/test/coverage.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
# frozen_string_literal: true

require_relative 'coverage/plan'
require_relative 'coverage/tracker'
require_relative 'coverage/covered_request'
require_relative 'coverage/covered_response'
require 'drb'

module OpenapiFirst
module Test
Expand All @@ -12,19 +16,20 @@ module Coverage

Result = Data.define(:plans, :coverage)

@current_run = {}

class << self
# @visibility private
def install
raise NoMethodError, 'Coverage.install was removed. Please use Test.setup instead'
end

def start(skip_response: nil, skip_route: nil)
@current_run = Test.definitions.values.to_h do |oad|
plan = Plan.for(oad, skip_response:, skip_route:)
[oad.key, plan]
end
return if @drb_uri
Copy link
Owner

Choose a reason for hiding this comment

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

👍🏼


tracker = Tracker.new(Test.definitions, skip_response:, skip_route:)
Copy link
Owner

Choose a reason for hiding this comment

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

I am looking for code to distinguish the main process from "remote" process… but that is not needed here, because Test.start only run in the main process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I assume here that Coverage.start (or Test.setup) is called in the main process. The issue is I'm not sure how to differentiate between main and worker process without storing the main process PID somewhere (which requires that some code is expected to run in the main process anyway).

Do you consider the Coverage module to be public API? In the Test module we at least safe-guard against multiple calls to setup which also prevents multiple calls to Coverage.start.

Copy link
Owner

@ahx ahx Sep 30, 2025

Choose a reason for hiding this comment

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

Do you consider the Coverage module to be public API?

Good question. I think I started out with Test::Coverage being public API, but added Test.setup after that. So I think Coverage should be considered private. …which could make some class methods on Coverage obsolete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you want to we can add something like Test.in_main_process? and guard Coverage.start with this. However, this would still require that Test.setup is called in the main process, which would then set the PID. What do you think? Alternatively we could also return early from Coverage.start if @drb_uri is non-nil?

…which could make some class methods on Coverage obsolete.

I thought about this as well while exchanging the current_run Hash with the Tracker class. For example, the current_run method is currently only used in specs. Same for the plans or the reset method.

Copy link
Owner

@ahx ahx Oct 1, 2025

Choose a reason for hiding this comment

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

If you want to we can add something like Test.in_main_process? and guard […] What do you think? Alternatively we could also return early from Coverage.start if @drb_uri is non-nil?

Hey @richardboehme, I think both approaches are fine. I don't have a strong opinion on this things right now. Maybe the second variant is simpler and respects the lines between Test and Coverage? Do you want to add that guard in this PR?

I thought about this as well while exchanging the current_run Hash with the Tracker class.

I will have removed Coverage.current_run, .plans, .install, .uninstall in another PR. #401

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I added a small guard on top of the Coverage.start method!


# We need a custom DRbServer (not using DRb.start_service) because otherwise
# we'd conflict with Rails's DRb server
@drb_uri = DRb::DRbServer.new(nil, tracker).uri
end

# @visibility private
Expand All @@ -34,15 +39,45 @@ def uninstall

# Clear current coverage run
def reset
@current_run = {}
@tracker = nil

return unless @drb_uri

service = DRb.fetch_server(@drb_uri)
service&.stop_service
@drb_uri = nil
end

def track_request(request, oad)
current_run[oad.key]&.track_request(request)
return unless request.known?

# The call to `track_request` may happen remotely in the main process that started
# the coverage collection.
# To make this work we need to keep arguments trivial, which is the reason the request
# is wrapped in a CoveredRequest data object.
tracker&.track_request(
oad.key,
CoveredRequest.new(
key: request.request_definition.key,
error: request.error
)
)
end

def track_response(response, _request, oad)
current_run[oad.key]&.track_response(response)
return unless response.known?

# The call to `track_response` may happen remotely in the main process that started
# the coverage collection.
# To make this work we need to keep arguments trivial, which is the reason the response
# is wrapped in a CoveredResponse data object.
tracker&.track_response(
oad.key,
CoveredResponse.new(
Copy link
Owner

Choose a reason for hiding this comment

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

👍🏼

key: response.response_definition.key,
error: response.error
)
)
end

def result
Expand All @@ -51,11 +86,19 @@ def result

private

attr_reader :current_run
def current_run
tracker.plans_by_key
end

# Returns all plans (Plan) that were registered for this run
def plans
current_run.values
tracker&.plans || []
end

def tracker
return unless @drb_uri

@tracker ||= DRbObject.new_with_uri(@drb_uri)
end

def coverage
Expand Down
11 changes: 11 additions & 0 deletions lib/openapi_first/test/coverage/covered_request.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

module OpenapiFirst
module Test
module Coverage
CoveredRequest = Data.define(:key, :error) do
def valid? = error.nil?
end
end
end
end
11 changes: 11 additions & 0 deletions lib/openapi_first/test/coverage/covered_response.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# frozen_string_literal: true

module OpenapiFirst
module Test
module Coverage
CoveredResponse = Data.define(:key, :error) do
def valid? = error.nil?
end
end
end
end
4 changes: 2 additions & 2 deletions lib/openapi_first/test/coverage/plan.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ def initialize(definition_key:, filepath: nil)
private attr_reader :index

def track_request(validated_request)
index[validated_request.request_definition.key]&.track(validated_request) if validated_request.known?
index[validated_request.key]&.track(validated_request)
end

def track_response(validated_response)
index[validated_response.response_definition.key]&.track(validated_response) if validated_response.known?
index[validated_response.key]&.track(validated_response)
end

def done?
Expand Down
32 changes: 32 additions & 0 deletions lib/openapi_first/test/coverage/tracker.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# frozen_string_literal: true

module OpenapiFirst
module Test
module Coverage
# Class that allows tracking requests and response for OAD definitions.
# For each definition it builds a plan and forwards tracking to the correct plan.
class Tracker
attr_reader :plans_by_key

def initialize(definitions, skip_response: nil, skip_route: nil)
@plans_by_key = definitions.values.to_h do |oad|
plan = Plan.for(oad, skip_response:, skip_route:)
[oad.key, plan]
end
end

def track_request(key, request)
@plans_by_key[key]&.track_request(request)
end

def track_response(key, response)
@plans_by_key[key]&.track_response(response)
end

def plans
@plans_by_key.values
end
end
end
end
end
40 changes: 10 additions & 30 deletions spec/test/coverage/plan_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,28 +47,32 @@

let(:valid_request) do
request = Rack::Request.new(Rack::MockRequest.env_for('/stuff/24'))
oad.validate_request(request)
request = oad.validate_request(request)
OpenapiFirst::Test::Coverage::CoveredRequest.new(key: request.request_definition.key, error: request.error)
end

let(:invalid_request) do
request = Rack::Request.new(Rack::MockRequest.env_for('/stuff/2t4'))
oad.validate_request(request)
request = oad.validate_request(request)
OpenapiFirst::Test::Coverage::CoveredRequest.new(key: request.request_definition.key, error: request.error)
end

let(:valid_response) do
request = Rack::Request.new(Rack::MockRequest.env_for('/stuff/24'))
response = Rack::Response.new
response.content_type = 'application/json'
response.write JSON.generate({})
oad.validate_response(request, response)
response = oad.validate_response(request, response)
OpenapiFirst::Test::Coverage::CoveredResponse.new(key: response.response_definition.key, error: response.error)
end

let(:invalid_response) do
request = Rack::Request.new(Rack::MockRequest.env_for('/stuff/24'))
response = Rack::Response.new
response.content_type = 'application/json'
response.write JSON.generate('foo')
oad.validate_response(request, response)
response = oad.validate_response(request, response)
OpenapiFirst::Test::Coverage::CoveredResponse.new(key: response.response_definition.key, error: response.error)
end

let(:valid_400_response) do
Expand All @@ -77,7 +81,8 @@
response.status = 400
response.content_type = 'application/json'
response.write JSON.generate({})
oad.validate_response(request, response)
response = oad.validate_response(request, response)
OpenapiFirst::Test::Coverage::CoveredResponse.new(key: response.response_definition.key, error: response.error)
end

subject(:plan) { described_class.for(oad) }
Expand Down Expand Up @@ -116,20 +121,6 @@
expect(request.last_error_message).to eq('Path segment is invalid: value at `/id` is not an integer')
end

it 'ignores unknown requests' do
request = Rack::Request.new(Rack::MockRequest.env_for('/unknown/24'))
plan.track_request(oad.validate_request(request))
expect(plan.coverage).to eq(0)
end

it 'ignores unknown responses' do
request = Rack::Request.new(Rack::MockRequest.env_for('/stuff/24'))
response = Rack::Response.new
response.status = 309
plan.track_response(oad.validate_response(request, response))
expect(plan.coverage).to eq(0)
end

it 'returns coverage in percentage' do
expect(plan.coverage).to eq(0)

Expand Down Expand Up @@ -169,17 +160,6 @@
expect(plan.tasks[2].status).to eq('4XX')
end

it 'ignores unknown responses' do
request = Rack::Request.new(Rack::MockRequest.env_for('/stuff/24'))
response = Rack::Response.new
response.status = 208
unknown_response = oad.validate_response(request, response)

plan.track_response(unknown_response)

expect(plan.tasks.count(&:finished?)).to eq(0)
end

it 'ignores skipped responses' do
plan = described_class.for(oad, skip_response: ->(res) { res.status == '200' })

Expand Down
32 changes: 30 additions & 2 deletions spec/test/coverage_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,29 @@
end
end

describe '.start' do
before do
described_class.reset
end

it 'starts drb service only once' do
expect(DRb).to receive(:regist_server).once
2.times { described_class.start }
end
end

describe '.track_request' do
it 'ignores unregistered OADs' do
oad = double(key: 'unknown')
described_class.track_request(double, oad)
described_class.track_request(
double(:request, known?: true, request_definition: double(key: nil), error: nil),
oad
)
end

it 'ignores unknown requests' do
request = double(known?: false)
described_class.track_request(request, definition)
end

it 'ignores skipped request' do
Expand All @@ -83,7 +102,16 @@
describe '.track_response' do
it 'ignores unregistered OADs' do
oad = double(key: 'unknown')
described_class.track_response(double(:response), double(:request), oad)
described_class.track_response(
double(:response, known?: true, response_definition: double(key: nil), error: nil),
double(:request),
oad
)
end

it 'ignores unknown response' do
response = double(known?: false)
described_class.track_response(response, double(:request), definition)
end
end
end
4 changes: 4 additions & 0 deletions spec/test_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ def call(_env)
end

describe 'Callable[]' do
before do
require 'openapi_first/test/callable'
end

it 'returns a Module that can call the api' do
mod = described_class::Callable[definition]
app.prepend(mod)
Expand Down