-
Notifications
You must be signed in to change notification settings - Fork 0
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-3010: Split out Subscription Rejection tests #7
base: main
Are you sure you want to change the base?
Conversation
...ns_r5_backport_r4_server/subscription_rejection/reject_subscription_channel_endpoint_test.rb
Outdated
Show resolved
Hide resolved
...riptions_test_kit/suites/subscriptions_r5_backport_r4_server/subscription_rejection_group.rb
Outdated
Show resolved
Hide resolved
...ns_r5_backport_r4_server/subscription_rejection/reject_subscription_channel_endpoint_test.rb
Outdated
Show resolved
Hide resolved
...ackport_r4_server/subscription_rejection/reject_subscription_cross_version_extension_test.rb
Outdated
Show resolved
Hide resolved
|
||
subscription_field[field_name] = unsupported_info['field_value'] | ||
|
||
send_unsupported_subscription(subscription, unsupported_info['unsupported_title'], |
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.
This logic does not appear to be doing the right thing. When I run it against the client tests, which echo the request back with a 201, it succeeds. Seems like that should be a failure because the cross version extension is still in the returned subscription and it succeeded.
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.
Addressed in 06d86cb. Made changes to this test to actually just fail when receiving a 201
since I don't think it makes sense to alter the values based on the requirement description ("cross version extensions SHOULD NOT be used on R4 subscriptions to describe any elements") from HL7 guide. I can change this though to be closer to what it was before, or remove the test if we don't think it's as fleshed out as it doesn't have a requirement.
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.
Not sure I agree with the changes. I think both approaches - allowing removal of the offending value are requiring rejection - are defensible. The IG also says that "servers MAY reject or coerce, according to their policies". Its in a different section, but it feels like it could easily apply here as well and that "coerce" could include removal of invalid extensions. If the verification code was working, I would prefer to leave it in rather than rip it out at this point. Interested to hear your reactions.
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.
nevermind - I guess the point was that this logic wasn't working, so I'm fine leaving it out!
...ubscriptions_r5_backport_r4_server/subscription_rejection/reject_subscription_filter_test.rb
Show resolved
Hide resolved
to resolve the requirements-coverage CI failure, run the |
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.
see comments
Summary
Testing Guidance