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] Add test scenario for OVN request API #816

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

mickmis
Copy link
Contributor

@mickmis mickmis commented Oct 24, 2024

This PR sits on top of #815, please review only the last commit.

This adds the test scenario for the feature implemented in interuss/dss#1119.

Copy link
Contributor

@Shastick Shastick left a comment

Choose a reason for hiding this comment

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

Side question: how do we determine if a DSS should support the optional extension? Do we just assume it does? And if it does not, it will have the related requirements shown as failed?

I guess that's fine at the moment, as the only DSS we are working is the reference implementation.

@BenjaminPelletier
Copy link
Member

Side question: how do we determine if a DSS should support the optional extension? Do we just assume it does? And if it does not, it will have the related requirements shown as failed?

I guess that's fine at the moment, as the only DSS we are working is the reference implementation.

I think this is a good question and there are multiple possible options. One option could be that test suites need to be constructed such that this scenario is only included when testing a DSS instance known to be complying with this requirement. But, if that were the case, I would not expect to find this scenario in a test suite labeled "F3548-21" since it is testing a requirement that is neither part of F3548-21 nor needed to enable testing of F3548-21 requirements.

Instead, I would suggest that we can add a field to the DSSInstance resource indicating whether the DSS instance has this capability (the field would be populated from a field in the resource specification which would be populated by the test designer when describing the test environment). This scenario could then cause itself to be skipped by raising a MissingResourceError in the constructor if the DSSInstance it was provided is not indicated as supporting this feature. This approach would make the scenario appropriate to include in F3548-21 testing since it's related to F3548-21 and would not cause failures if one or more DSS instances didn't support it.

@@ -63,6 +63,9 @@ class DSSInstanceSpecification(ImplicitDict):
base_url: str
"""Base URL for the DSS instance according to the ASTM F3548-21 API"""

supports_ovn_request: Optional[bool] = None
Copy link
Member

Choose a reason for hiding this comment

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

Setting a default value of None to an Optional[bool] means that this field will always be populated when writing an instance of DSSInstanceSpecification, and it will be populated with None; e.g.:

{"participant_id":"foo","base_url":"http://example.com","supports_ovn_request":null}

If we want the field to always be populated, I think it should probably be populated with the native type (i.e., = False). If we want the ability to omit the field, that omission should probably be symmetric (input and output) so we shouldn't populate the field with a default value. We should probably define a standard style for things like this. The disadvantage of allowing omission is the need to check for field presence before accessing the field, so a default value eliminates that risk. But, that comes at the expense of not being able to differentiate between omission of the field value and positive specification of the default value. I think the importance of that depends on the context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably define a standard style for things like this.

Agreed! Maybe for such classes that have a 1-1 match with a yaml file we should forbid default values? Or is that still required in some cases?

The disadvantage of allowing omission is the need to check for field presence before accessing the field

Indeed that was the reason why I've put that default.
I've removed it, dropped the Optional and added a check when accessing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually keeping the Optional since this seems to otherwise mark the field as required.

@mickmis mickmis force-pushed the dss1078/testscenario branch 2 times, most recently from 06b7977 to 1e37f6f Compare November 13, 2024 10:19
@mickmis mickmis force-pushed the dss1078/testscenario branch from 1e37f6f to f89e680 Compare November 13, 2024 10:23
@mickmis mickmis merged commit 58e40fe into interuss:main Nov 13, 2024
20 checks passed
@mickmis mickmis deleted the dss1078/testscenario branch November 13, 2024 10:36
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.

3 participants