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.

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

[uss_qualifier] oir_simple: check oir can be created without sub and attached to explicit sub #804

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Shastick
Copy link
Contributor

Extend OIRSimple with a test case that:

  • checks we can create a subscription-free OIR
  • then checks an explicit subscription can be attached, and that the subscription needs to cover the OIR.

This covers point (4) on interuss/dss#1088

Note: this builds on top of #803, only the last commit of this PR is relevant.

@Shastick Shastick force-pushed the oir-simple-1-check-explicit-can-be-added-to-none branch from 358cde9 to 6c9828a Compare October 15, 2024 10:51
@Shastick Shastick marked this pull request as ready for review October 15, 2024 15:37
Copy link
Contributor

@mickmis mickmis left a comment

Choose a reason for hiding this comment

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

Note: I've only reviewed the .md so far.

@@ -34,6 +34,14 @@
)
from monitoring.uss_qualifier.suites.suite import ExecutionContext

# The official DSS implementation will set an OIR's subscription ID to 00000000-0000-4000-8000-000000000000
Copy link
Contributor

Choose a reason for hiding this comment

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

Not relevant to this PR, but IMO that means the OpenAPI is wrong and the subscription ID should not be a mandatory field. We should consider fixing that.

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, that would be the correct approach.

I think this was used as an immediate stop-gap because the DSS needed to be made compliant (by allowing ACCEPTED OIRs without subscriptions, which the standard explicitly allows) without breaking the OpenAPI spec, but we lost track of this.

I created #806 to keep track of it

@@ -8,3 +8,10 @@ Check query succeeds

If the OIR returned by the DSS under test is not attached to the expected subscription,
it is in violation of **[astm.f3548.v21.DSS0005,1](../../../../../../requirements/astm/f3548/v21.md)**

## [Get referenced Subscription](../sub/crud/read_query.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that this should succeed if the the subscription does not exist? That warrants a separate check, or usage of a dedicated fragment 'check subscription does not exist'.


This step verifies that an OIR can be created in the ACCEPTED state without providing any subscription information (implicit or explicit) in the request.

### [OIR is not attached to any subscription test step](./fragments/oir/oir_has_expected_subscription.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like this shouldn't be a step but rather included as a fragment under the above step.


## Validate explicit subscription on OIR creation test case

Ensures that the explicit subscription provided upon creation of an OIR is properly validated and attached to the OIR.

### [Ensure clean workspace test step](./clean_workspace.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as other PR: a 'clean workspace' step in middle of scenario is not a good idea IMO.

If the DSS under test allows the qualifier to attach an insufficient explicit subscription to a subscription-free OIR,
it is in violation of **[astm.f3548.v21.DSS0005,1](../../../../requirements/astm/f3548/v21.md)**

### [OIR is not attached to any subscription test step](./fragments/oir/oir_has_expected_subscription.md)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like this shouldn't be a step but rather included as a fragment under the above step.

Comment on lines +70 to +76
### [Attach explicit subscription to OIR test step](./fragments/oir/crud/update_query.md)

This step verifies that an explicit subscription covering the OIR can be attached to an OIR that currently has no subscription.

### [OIR is attached to expected subscription test step](./fragments/oir/oir_has_expected_subscription.md)

This step verifies that the OIR is attached to the subscription provided upon creation.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel like those belong in this test case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test case's intention is to:

  • check that a subscription that does not cover the OIR cannot be attached to it
  • check that a subscription that covers the OIR can be attached to it

The first step demonstrates that some validation indeed occurs. The second step demonstrates that the validation logic is correct (at least superficially). In other words, the test case demonstrates that validation occurs and is not buggy.

Possibly I could rename the test case to Properly validate explicit subscription being attached to OIR without subscription?

@Shastick
Copy link
Contributor Author

Note: I've only reviewed the .md so far.

Thanks. I think we should first figure out the open points on #803 before diving deeper into this PR anyway.

@Shastick Shastick marked this pull request as draft October 24, 2024 15:59
@Shastick
Copy link
Contributor Author

Moved to draft while we figure out things on the parent PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants