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] initial constraint reference sync scenario - creation - 1/n #704

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented May 30, 2024

Initial scenario for validating synchronization of constraint references. This currently covers constraint creation only (without even checking synchronization), along with the requirements for cleaning up constraints.

(This PR was split off from #687 to bring it in in multiple steps.)

@@ -0,0 +1,9 @@
# Create constraint reference response format test step fragment
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the context of this PR, some of the fragments may seem like they are overkill. However, the rest of the scenario as well as the upcoming update of the authentication check scenario will make use of them.

(I'm essentially reusing the approach that was taken for the scenarios focused on operation intent references)

@Shastick Shastick force-pushed the constraint-ref-0 branch 2 times, most recently from 0094dda to 7304995 Compare May 30, 2024 11:21
@Shastick Shastick changed the title [uss_qualifier] initial constraint reference sync scenario (creation+boradcast) [uss_qualifier] initial constraint reference sync scenario - creation - 1/n May 30, 2024
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.

All comments are minor, please address at least non-nits ones before merging, otherwise LGTM.

"Constraint references can be queried by ID", [dss.participant_id]
) as check:
try:
cr, q = dss.get_constraint_ref(cr_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

q should be recorded in case of success (it is recorded only if there is a failure).

Comment on lines 145 to 147
for cr in crs:
if cr.manager == manager_identity:
remove_constraint_ref(scenario, dss, cr.id, cr.ovn)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move this outside of the with block.

"Constraint references can be searched for", [dss.participant_id]
) as check:
try:
crs, query = dss.find_constraint_ref(volume)
Copy link
Contributor

Choose a reason for hiding this comment

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

Record query also when successful.

scenario.record_queries(qe.queries)
check.record_failed(
summary="Failed to query constraint references",
details=f"Failed to query constraint references: got response code {qe.queries[0].status_code}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Include in details the error message from the query if any.

def cleanup_constraint_ref(
scenario: TestScenarioType, dss: DSSInstance, cr_id: EntityID
) -> None:
"""Remove the specified constraint reference from the DSS, if it exists."""
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: mention which check this implements.

self._fail_sub_check(
check,
summary="Returned manager is incorrect",
details=f"Expected. {self._expected_manager}, got {dss_cr.manager}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
details=f"Expected. {self._expected_manager}, got {dss_cr.manager}",
details=f"Expected {self._expected_manager}, got {dss_cr.manager}",

"Returned constraint reference has an USS base URL", self._pid
) as check:
# If uss_base_url is not present, or it is None or Empty, we should fail:
if "uss_base_url" not in dss_cr or not dss_cr.uss_base_url:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this is similar to manager, no need to check if the field is present, the validation would have caught 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.

This would also be true of some fields in other scenarios (version and OVN for example): there is some overlap between checks that I think is acceptable and even beneficial, as it results in having one check per field in the test report – it might be confusing for people reading a test report that some fields don't have an explicit check.


## ⚠️ Returned constraint reference has an end time check

constraint references need a defined end time in order to limit their duration: if the DSS omits to set the end time, it will be in violation of **[astm.f3548.v21.DSS0005,3](../../../../../../../requirements/astm/f3548/v21.md)**.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
constraint references need a defined end time in order to limit their duration: if the DSS omits to set the end time, it will be in violation of **[astm.f3548.v21.DSS0005,3](../../../../../../../requirements/astm/f3548/v21.md)**.
Constraint references need a defined end time in order to limit their duration: if the DSS omits to set the end time, it will be in violation of **[astm.f3548.v21.DSS0005,3](../../../../../../../requirements/astm/f3548/v21.md)**.

Comment on lines 216 to 268
# If the previous OVN is not None, we are dealing with a mutation:
if previous_ovn is not None:
with self._scenario.check(
"Mutated constraint reference OVN is updated", self._pid
) as check:
if dss_cr.ovn == previous_ovn:
self._fail_sub_check(
check,
summary="Returned CR OVN was not updated",
details=f"Expected OVN to be different from {previous_ovn}, but it was not",
t_dss=t_dss,
)

if expected_ovn is not None:
with self._scenario.check(
"Non-mutated constraint reference keeps the same OVN", self._pid
) as check:
if dss_cr.ovn != expected_ovn:
self._fail_sub_check(
check,
summary="Returned CR OVN was updated",
details=f"Expected OVN to be {expected_ovn}, Returned: {dss_cr.ovn}",
t_dss=t_dss,
)

# If the previous version is not None, we are dealing with a mutation:
if previous_version is not None:
with self._scenario.check(
"Mutated constraint reference version is updated", self._pid
) as check:
# TODO confirm that a mutation should imply a version update
if dss_cr.version == previous_version:
self._fail_sub_check(
check,
summary="Returned CR version was not updated",
details=f"Expected version to be different from {previous_ovn}, but it was not",
t_dss=t_dss,
)

# TODO version _might_ get incremented due to changes caused outside of the uss_qualifier
# and we should probably check if it is equal or higher.
if expected_version is not None:
with self._scenario.check(
"Non-mutated constraint reference keeps the same version",
self._pid,
) as check:
if dss_cr.version != expected_version:
self._fail_sub_check(
check,
summary="Returned CR version was updated",
details=f"Expected version to be {expected_ovn}, Returned: {dss_cr.version}",
t_dss=t_dss,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove those checks from the PR? They don't have the matching documentation, and are never invoked (in this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.


The returned end time must be the same as the provided one, otherwise the DSS is in violation of **[astm.f3548.v21.DSS0005,3](../../../../../../../requirements/astm/f3548/v21.md)**.

## ⚠️ Returned constraint reference has a version check
Copy link
Contributor

Choose a reason for hiding this comment

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

Implementation of this check is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. There was no check for the OVN either, added it as well.

@Shastick Shastick force-pushed the constraint-ref-0 branch 2 times, most recently from 99a478a to d5b018e Compare June 8, 2024 10:02
@Shastick
Copy link
Contributor Author

This is ready to merge.

@barroco barroco merged commit 1f423f9 into interuss:main Jun 11, 2024
19 checks passed
github-actions bot added a commit that referenced this pull request Jun 11, 2024
… - 1/n (#704)

[uss_qualifier] new constraint ref validator, based on the OIR validator 1f423f9
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