-
Notifications
You must be signed in to change notification settings - Fork 30
[netrid] Check response validity when calling all_flights #1388
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
Open
the-glu
wants to merge
1
commit into
interuss:main
Choose a base branch
from
Orbitalize:fix_1386
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
As you noticed in your PR comment,
rid.all_flightsdoesn't just perform an ISA query; it performs an ISA query (potentially) followed by a bunch of queries for flights and flight details from individual USSs. The DSS failing to return ISAs is a violation of different requirements than the core USS failing to respond correctly to a flights query or a flight details query. Probably part of #1386 involves resolving the TODO just above this section.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.
Yes, I think the todo is resolved, however the name is wrong (and probably wrong for existing
ISA queryas well, since they do the same).Do you have a better name in mind?
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.
For misbehaviour I switched to "Initial queries" and improved description based on comment bellow, but I think the point still hold for display data evaluator ones.
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.
Remember that test steps are where actions are performed, and then checks (following the actions) determine if any requirements were violated based on the information obtained from those actions. With that in mind, I'm not sure what "Initial queries" would be checking. I think this first check description makes sense:
But, it seems like the thing we're checking is "Did the (single) DSS query to search for ISAs succeed?" I would expect that check to be named something like "Successful ISA search" or "DSS ISAs search succeeded" or "ISA query succeeded" or "ISA query success".
This check description makes less sense to me (additional emphasis added):
This seems to be documenting a check (the first sentence) then another, separate test step (second sentence). Actions in a test step belong in the test step documentation (one documentation level higher than check documentation) and then checks should describe only how the information already obtained will be evaluated to determine if a requirement was violated.
So, it seems like there are three actions in this test step (due to
rid.all_flights):The InterUSS requirement
interuss.f3411.dss_endpoints.SearchISAsapplies only to the information obtained from action 1.interuss.f3411.dss_endpoints.SearchISAsdoesn't relate at all to the information obtained in actions 2 nor 3 -- instead, presumably requirements NET0710,1 and NET0710,2 apply to actions 2 and 3, respectively (when those queries are expected to succeed). But, these should be two separate checks since the information we're evaluating for NET0710,1 is coming from action 2 and the information we're evaluating for NET0710,2 is coming from action 3 -- the action, information, and relevant requirement are all different between action 2/NET0710,1 and action 3/NET0720,2 so they shouldn't be combined into a single check.The original problem prompting #1386 was that these two checks AND the ISA query check were all combined into a single check. When that check failed, it was extremely unclear what had actually gone wrong. That's because a check for
A && B && C && Dfailing provides little information as to whether A failed, B failed, and/or C failed. This was especially problematic because the check only documented that D was being checked, so the fact that B was failing was extremely difficult to determine.