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] netrid: nominal_behavior also covers max area requirement for SPs #821

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

Shastick
Copy link
Contributor

@Shastick Shastick commented Oct 25, 2024

The nominal_behavior scenario already checks off NET0430 (which requires a Display Provider to not return data if the requested diagonal is too big) within the "Service Provider polling" test step.

This seems incorrect, as NET0430 specifically concerns a display provider, it should not be present in the Service Provider polling

The PR adds coverage for NET0250 simply by replacing NET0430 with NET0250

Part of the effort for #754

@mickmis
Copy link
Contributor

mickmis commented Oct 29, 2024

I'm not convinced the approach is ideal: those are different requirements for different participants, different requirements, with data coming from similar but different interfaces. However it does look like the implementation may require a refactoring as there is some mixing of DP and SP flows. I'd say that's a good topic for today's meeting?

@Shastick Shastick marked this pull request as draft October 29, 2024 13:41
@Shastick
Copy link
Contributor Author

I'm not convinced the approach is ideal: those are different requirements for different participants, different requirements, with data coming from similar but different interfaces. However it does look like the implementation may require a refactoring as there is some mixing of DP and SP flows. I'd say that's a good topic for today's meeting?

You're right that this is not ideal: I got confused and mixed SP and DP check logic.

nominal_behavior has separate test steps for Service Provider polling and Observer polling.

Taking this to draft while I move the check to the proper place.

@Shastick Shastick marked this pull request as ready for review October 29, 2024 16:24
@Shastick
Copy link
Contributor Author

@mickmis after double checking and our conversation at the community meeting, the present solution seems good enough: checks for the display provider and the service provider are properly separated in the code, and the present PR does not increase the complexity of the display data evaluator.

@Shastick Shastick force-pushed the net0250-sp-max-area-check branch 2 times, most recently from 22ec5ef to 56b7fe6 Compare October 29, 2024 16:30
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.

Approach LGTM, however looking at implementation the check text should be updated:

for mapping in mappings.values():
with self._test_scenario.check(
"Area too large", [mapping.injected_flight.uss_participant_id]
) as check:
check.record_failed(
summary="Flight discovered using too-large area request",
details=f"{mapping.injected_flight.uss_participant_id} was queried for flights in {_rect_str(rect)} with a diagonal of {diagonal} km which is larger than the maximum allowed diagonal of {self._rid_version.max_diagonal_km} km. The expected error code is 413, but instead a valid response containing the expected flight was received.",
severity=Severity.High,
query_timestamps=[
mapping.observed_flight.query.query.request.timestamp
],
)

In short, for an SP, this check is failed after the fact if a flight was discovered using a rectangle that was too large.

Additionally the check is created+failed after the failed conditions are evaluated: that means it will never show as a success. I think the implementation should be changed so that it can surface as successfully tested.

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.

LGTM, however since that check can't be set successful as is, I don't think we can consider NET0250 covered yet. We could relatively easily add a scenario or a case in an existing scenario that attempts direct queries to the SP with a too large diagonal. I've put a note in the spreadsheet tracking the progress.

@Shastick Shastick force-pushed the net0250-sp-max-area-check branch 2 times, most recently from 97f8a49 to edab217 Compare November 14, 2024 08:52
@mickmis mickmis merged commit 97aa8c7 into interuss:main Nov 14, 2024
20 checks passed
@mickmis mickmis deleted the net0250-sp-max-area-check branch November 14, 2024 13:56
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