-
Notifications
You must be signed in to change notification settings - Fork 5
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
FI-3600: Requirements Support #588
base: main
Are you sure you want to change the base?
Changes from 4 commits
566abbc
c10ff1f
b188c5d
d9f6af5
e4d8ec0
f7e65d3
33e101b
837fa60
42d1c27
8a90054
1112fa9
e25cda3
68f4007
602d7e3
72e2b45
6125d32
aa0214a
ba03aad
8d00643
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
Req Set,ID,URL,Requirement,Conformance,Actor,Sub-Requirement(s),Conditionality | ||
sample-criteria-proposal,1,,Feugiat in ante metus dictum. Dignissim cras tincidunt lobortis feugiat.,SHALL,Client,"sample-criteria-proposal@2-3,4,6",FALSE | ||
sample-criteria-proposal,2,,tempor incididunt ut labore et dolore magna aliqua,SHALL,Client,sample-criteria-proposal@3,FALSE | ||
sample-criteria-proposal,3,,"Lorem ipsum dolor sit amet, consectetur adipiscing elit",SHALL,Client,sample-criteria-proposal@5,FALSE | ||
sample-criteria-proposal,4,,Consequat mauris nunc congue nisi vitae suscipit tellus mauris.,SHALL,Client,sample-criteria-proposal@6,FALSE | ||
sample-criteria-proposal,5,,condimentum vitae sapien pellentesque habitant morbi tristique senectus,SHALL,Client,sample-criteria-proposal@6,TRUE | ||
sample-criteria-proposal,6,,Faucibus scelerisque eleifend donec pretium vulputate sapien nec sagittis,SHALL,Client,,TRUE | ||
sample-criteria-proposal,7,,Nulla facilisi nullam vehicula ipsum a,SHALL,Client,,FALSE |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
module RequirementsSuite | ||
class Suite < Inferno::TestSuite | ||
title 'Requirements Suite' | ||
id :ig_requirements | ||
description 'Suite Description' | ||
|
||
verifies_requirements 'sample-criteria-proposal@1', 'sample-criteria-proposal@2', | ||
'sample-criteria-proposal@3', 'sample-criteria-proposal@4' | ||
|
||
group do | ||
title 'Test Requirements 1 and 2' | ||
verifies_requirements 'sample-criteria-proposal@1', 'sample-criteria-proposal@2' | ||
|
||
test do | ||
title 'Requirement 1' | ||
run { pass } | ||
end | ||
|
||
test do | ||
title 'Requirement 2' | ||
run { pass } | ||
end | ||
end | ||
|
||
group do | ||
title 'Test Requirements 3 and 4' | ||
verifies_requirements 'sample-criteria-proposal@3', 'sample-criteria-proposal@4' | ||
|
||
test do | ||
title 'Requirement 3' | ||
run { pass } | ||
end | ||
|
||
test do | ||
title 'Requirement 4' | ||
run { pass } | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
require_relative '../../serializers/requirement' | ||
|
||
module Inferno | ||
module Web | ||
module Controllers | ||
module Requirements | ||
class Index < Controller | ||
def handle(_req, res) | ||
res.body = serialize(repo.all) | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
require_relative '../../serializers/requirement' | ||
|
||
module Inferno | ||
module Web | ||
module Controllers | ||
module Requirements | ||
class Show < Controller | ||
def handle(req, res) | ||
requirement = repo.find(req.params[:id]) | ||
halt 404 if requirement.nil? | ||
|
||
res.body = serialize(requirement) | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,6 +45,11 @@ module Web | |
as: :check_configuration | ||
end | ||
|
||
scope 'requirements' do | ||
get '/', to: Inferno::Web::Controllers::Requirements::Index, as: :index | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems like a problem. I don't think there is a use case for loading all of the requirements for every test kit, so we need some way to scope them down to only the requirements relevant for a particular suite. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're thinking in a deployment where we have lots of test kits running / available within a single deployment? The vision I had was for the -requirements.csv file to have all requirements relevant to the test kit. Then suites within the test kit are associated with a specific set of requirements via actors - each suite is associated with an actor and so requirements for that actor from the test kit's requirement set apply to that suite. Actor mapping through referenced IGs can be a bit tricky and thinking about this, I think the tools I created are doing it wrong by mapping during the creation of the set of test kit requirements, probably should be after the fact so that requirements, e.g., from the HRex IG that are shared by multiple test kits and suites have the same representation in this database when it includes requirements from those different kits / suites. If I'm understanding Steve's suggestion correctly, it is that requirements in the database need to be associated with a test kit and suite in the database so that filtering can happen. Association with the test kit is from the -requirements.csv file. Association with each suite is designed to be based on the actor with maps from suites to actors present in a requirements configuration file that looks something like this:
Open to suggestions on how to tweak this, but I think the basic principles are there. Again, not sure if this is in scope for this PR or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. If groups inherit all of their child's requirements, then it should be easy to get the all of relevant requirements for a particular suite (which I think is the use case we actually care about), but the routing will have to reflect that rather than being a plain |
||
get '/:id', to: Inferno::Web::Controllers::Requirements::Show, as: :show | ||
end | ||
|
||
get '/requests/:id', to: Inferno::Web::Controllers::Requests::Show, as: :requests_show | ||
|
||
get '/version', to: ->(_env) { [200, {}, [{ 'version' => Inferno::VERSION.to_s }.to_json]] }, as: :version | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
require_relative 'serializer' | ||
|
||
module Inferno | ||
module Web | ||
module Serializers | ||
class Requirement < Serializer | ||
identifier :id | ||
|
||
field :requirement | ||
field :conformance | ||
field :actor | ||
field :sub_requirements | ||
field :conditionality | ||
field :url, if: :field_present? | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
require_relative '../../repositories/requirements' | ||
|
||
Inferno::Application.register_provider(:requirements) do | ||
prepare do | ||
target_container.start :suites | ||
|
||
requirements_repo = Inferno::Repositories::Requirements.new | ||
|
||
test_kit_gems = | ||
Bundler | ||
.definition | ||
.specs | ||
.select { |spec| spec.metadata.fetch('inferno_test_kit', 'false').casecmp? 'true' } | ||
|
||
files_to_load = Dir.glob(['lib/*test_kit/requirements/*.csv']) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will pull in the out of scope requirements files as well, which shouldn't be loaded in the same way - see https://github.com/inferno-framework/subscriptions-test-kit/tree/main/lib/subscriptions_test_kit/requirements for example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, should we pull in the out-of-scope requirements or exclude them for now? If we include, I can update the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The out of scope requirements are a subset of the requirements that will not be tested. The idea is that putting requirements into that file is the way of saying they shouldn't be expected to be verified by any tests. Assuming that we want this requirements API to represent the requirements coverage matrix at the right side of that diagram, then I think you're right that we want to add fields to support the reason and details, or rather out_of_scope_reason and out_of_scope_details or something similar tying it to the out-of-scope purpose and source). That said, right now you don't appear to be including links from requirements to tests in the requirements repo and these files are really meant to be partners of the verifies_requirements annotations. Personally, I think it would make sense to incorporate into the requirements database the logic that determines requirement coverage and allow it to be exported to the requirements_coverage csv file. If you're not going to do that (or at least not in this PR), then I would skip processing these out of scope files. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which requirements go with which tests is stored on the tests, not on the requirements. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Understood. I think that means that you don't want to include details from the out of scope files in the requirements repository and that those files should be ignored during this load. |
||
|
||
if ENV['LOAD_DEV_SUITES'].present? | ||
ENV['LOAD_DEV_SUITES'].split(',').map(&:strip).reject(&:empty?).each do |suite| | ||
files_to_load.concat Dir.glob(File.join(Inferno::Application.root, 'dev_suites', suite, 'requirements', | ||
'*.csv')) | ||
end | ||
end | ||
|
||
files_to_load += | ||
test_kit_gems.flat_map do |gem| | ||
[ | ||
Dir.glob([File.join(gem.full_gem_path, 'lib', '*test_kit', 'requirements', '*.csv')]) | ||
].flatten | ||
end | ||
|
||
files_to_load.compact! | ||
files_to_load.uniq! | ||
files_to_load.map! { |path| File.realpath(path) } | ||
|
||
files_to_load.each do |path| | ||
requirements_repo.insert_from_file(path) | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
require_relative 'attributes' | ||
require_relative 'entity' | ||
|
||
module Inferno | ||
module Entities | ||
# A `Requirement` represents the specific rule or behavior a runnable is testing. | ||
class Requirement < Entity | ||
ATTRIBUTES = [ | ||
:id, | ||
:url, | ||
:requirement, | ||
:conformance, | ||
:actor, | ||
:sub_requirements, | ||
:conditionality | ||
].freeze | ||
|
||
include Inferno::Entities::Attributes | ||
|
||
def initialize(params) | ||
super(params, ATTRIBUTES) | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
require 'csv' | ||
require_relative 'in_memory_repository' | ||
require_relative '../entities/requirement' | ||
|
||
module Inferno | ||
module Repositories | ||
# Repository that deals with persistence for the `Requirement` entity. | ||
class Requirements < InMemoryRepository | ||
def insert_from_file(path) # rubocop:disable Metrics/CyclomaticComplexity | ||
result = [] | ||
|
||
CSV.foreach(path, headers: true, header_converters: :symbol) do |row| | ||
req_set = row[:req_set] | ||
id = row[:id] | ||
sub_requirements_field = row[:subrequirements] | ||
|
||
combined_id = "#{req_set}@#{id}" | ||
|
||
# Processing sub requirements: e.g. "170.315(g)(31)_hti-2-proposal@5,17,23,26,27,32,35,38-41" | ||
sub_requirements = if sub_requirements_field.nil? || sub_requirements_field.strip.blank? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need to |
||
[] | ||
else | ||
base, ids = sub_requirements_field.split('@') | ||
ids.split(',').flat_map do |item| | ||
if item.include?('-') | ||
start_range, end_range = item.split('-').map(&:to_i) | ||
(start_range..end_range).map { |num| "#{base}@#{num}" } | ||
else | ||
"#{base}@#{item}" | ||
end | ||
end | ||
end | ||
|
||
result << { | ||
id: combined_id, | ||
url: row[:url], | ||
requirement: row[:requirement], | ||
conformance: row[:conformance], | ||
actor: row[:actor], | ||
sub_requirements: sub_requirements, | ||
conditionality: row[:conditionality]&.downcase | ||
} | ||
end | ||
|
||
result.each do |raw_req| | ||
requirement = Entities::Requirement.new(raw_req) | ||
|
||
insert(requirement) | ||
end | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Req Set,ID,URL,Requirement,Conformance,Actor,Sub-Requirement(s),Conditionality | ||
sample-criteria,1,,requirement,SHALL,Client,sample-criteria@2,FALSE | ||
sample-criteria,2,,requirement,SHALL,Client,,FALSE |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
RSpec.describe Inferno::Repositories::Requirements do | ||
subject(:requirements) { described_class.new } | ||
|
||
describe '#insert_from_file' do | ||
let(:csv) do | ||
File.realpath(File.join(Dir.pwd, 'spec/fixtures/simple_requirements.csv')) | ||
end | ||
let(:req_one) do | ||
Inferno::Entities::Requirement.new( | ||
{ | ||
id: 'sample-criteria@1', | ||
requirement: 'requirement', | ||
conformance: 'SHALL', | ||
actor: 'Client', | ||
sub_requirements: ['sample-criteria@2'], | ||
conditionality: 'false' | ||
} | ||
) | ||
end | ||
let(:req2) do | ||
Inferno::Entities::Requirement.new( | ||
{ | ||
id: 'sample-criteria@2', | ||
requirement: 'requirement', | ||
conformance: 'SHALL', | ||
actor: 'Client', | ||
sub_requirements: [], | ||
conditionality: 'false' | ||
} | ||
) | ||
end | ||
|
||
it 'creates and inserts all requirements from the csv file' do | ||
expect { requirements.insert_from_file(csv) }.to change { requirements.all.size }.by(2) | ||
expect(requirements.find(req_one.id).to_hash).to eq(req_one.to_hash) | ||
expect(requirements.find(req2.id).to_hash).to eq(req2.to_hash) | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't higher-level groups automatically inherit all of the requirements their children verify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think they should automatically inherit. Example illustrating why:
An execution of Group 1 where Test A succeeds and Test B fails would result in Group 1 succeeding but not in requirement 2 being met. I think we want to keep the property that if a runnable succeeds then that implies that all its associated requirements are met. Inheritance would break this property because Group 1 succeeded but requirement 2 is not met, so Group 1 cannot inherit the requirements from its children.
Its possible that I'm wrong about wanting a success to indicate requirements are met, but my gut says that's important, so if you have different thoughts, I'd want to talk through it further.