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] Clean up get op data validation scenario #419

Merged

Conversation

BenjaminPelletier
Copy link
Member

@BenjaminPelletier BenjaminPelletier commented Dec 18, 2023

This PR cleans up a few things in the Data Validation of GET operational intents by USS test scenario:

  1. begin_test_step and end_test_step are removed from reusable test step fragments per Change reusable test steps to reusable test step fragments #380
  2. control_uss is changed to mock_uss because we know that resource is a mock USS, so therefore there is no difference between control_uss and mock_uss, and one consistent name is easier to comprehend
  3. The test step to check for notifications is moved to just before its outcome is needed
  4. The purpose of the test step to check for notifications is clarified
  5. Redundant explanation of the Validate flight2 GET interaction test step is removed to avoid the possibility of inconsistency (the same explanation remains in the reusable test step fragment)
  6. Validation of flight intents is performed using ExpectedFlightIntents, similar to [uss_qualifier] Add flight intent validation and fix conflict same priority scenario #393
  7. Test case function names are shortened for clarity
  8. Severity overrides are removed where severity is specified in documentation per Specify check severity in test scenario documentation only #404

-- edit --

  1. Identification of desired mock_uss interactions is improved

-- edit 2 --

  1. Fix infinite loop preventing f3548_self_contained to complete #425

The test driver attempts to plan flight 1 via the tested USS. It checks if any conflicts with flight 2
which is of equal priority and came first.

### [Validate flight 1 sharing test step](../validate_shared_operational_intent.md)
Validate flight 1 is planned.

### Check for notification to tested_uss due to subscription in flight 2 area test step
Copy link
Contributor

Choose a reason for hiding this comment

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

After sharing flight1, there are other POST interactions that might happen. We will be able to catch the flight2 notification immediately after it's planned. Or else we will need to know the base_url for uss_endpoint of the tested_uss, which we don't have access to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point -- currently, the test looks for any POST interaction which is needlessly broad since the notifications we're looking for would only be outgoing from mock_uss. I'll clean that up as well, and then this move should be fine since mock_uss does not make any outgoing POST notifications in the steps in between this step's old position and its new position.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is now fixed -- instead of just searching according to POST generally (which is only one small characteristic of the interactions of interest), we now search much more specifically and can differentiate (for instance) between incoming and outgoing operational intent details notifications.

The test driver attempts to plan the flight 1 via the tested_uss. It checks if any conflicts with flight 2
which is of equal priority and came first.

### [Validate flight 1 not shared by tested_uss test step](../validate_not_shared_operational_intent.md)
Validate flight 1 is not shared with DSS, as plan failed.

### Check for notification to tested_uss due to subscription in flight 2 area test step
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed per above.

"Validate flight2 GET interaction, if no notification",

self.begin_test_step(
"Check for notification to tested_uss due to subscription in flight 2 area"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why we would want to check for notifications here, instead of close to when it was sent after planning flight 2. Then we have to go through less number of interactions to filter it out.
I am not saying this is wrong, but trying to understand the reason behind it. I understand its close to where its being used, is that the reason?

Copy link
Member Author

Choose a reason for hiding this comment

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

Checking later gives more time for the notification to happen, we're searching since flight_2_planning_time in any case, and the result isn't used until later (where this step is now). We're now filtering for the notification we actually want via method, URL, and op intent ID, so the likelihood of a false positive is very low regardless of the number of other interactions in the mean time.

Copy link
Contributor

Choose a reason for hiding this comment

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

We actually don't need to wait here, as the outgoing notifications from mock_uss are sent sequentially before sending Completed response to uss_qualifier. So, if we get a success plan from mock_uss, we know the notifications have already been sent.
Checking later is fine with me.


This step obtains interactions of interest from mock_uss.

## 🛑 MockUSS interactions request check
Copy link
Contributor

@punamverma punamverma Dec 19, 2023

Choose a reason for hiding this comment

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

Ok, we just copy the High Severity symbol into the doc. How does it get related to the code?

Copy link
Member Author

Choose a reason for hiding this comment

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

@punamverma
Copy link
Contributor

punamverma commented Dec 19, 2023

The code looks good.
However, when I tried running the tests locally, somehow they are failing at Validation of operational intents scenario. I see CI ran successfully. Could be my local environment has issues.
But, I do see that from previous code that I had missed to assign None to Optional field in this line -

expected_invalid_fields: Optional[List[str]],

@@ -1,54 +1,56 @@
from __future__ import annotations

import datetime
import re
from typing import List, Tuple, Optional
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing import for Dict and Callable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, thanks. My IDE is going crazy on this file specifically

from implicitdict import StringBasedDateTime
from loguru import logger
from uas_standards.astm.f3548.v21 import api
from uas_standards.astm.f3548.v21.api import OperationID
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing import for EntityID

Copy link
Member Author

Choose a reason for hiding this comment

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

Added, thanks

@punamverma
Copy link
Contributor

punamverma commented Dec 19, 2023

I like the idea of removing begin_test_step and end_test_step from reusable code. Initially, it took me some time to understand the flow of the tests because of that. I thought it was a convention being used in the code. Taking it out is a good idea.
Liked renaming control_uss to mock_uss.
Also, refactoring the code with mock_uss_interactions method makes it better. Thanks.

@@ -2,8 +2,8 @@

## Description
This test checks that the USS being tested validates the operational intents received as response to its GET request from another USS.
Control_uss which is a mock uss plans a nearby V-shaped operation, and provides the data that tested_uss GETs.
tested_uss validates the GET response from control_uss and accordingly plan its operation.
mock_uss which is a mock uss plans a nearby V-shaped operation, and provides the data that tested_uss GETs.
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
mock_uss which is a mock uss plans a nearby V-shaped operation, and provides the data that tested_uss GETs.
mock_uss plans a nearby V-shaped operation, and provides the data that tested_uss GETs.

The control_uss, which is mock_uss is instructed to share invalid data with other USS, for negative test.
### [mock_uss plans flight 2, sharing invalid operational intent data test step](../../../flight_planning/plan_flight_intent.md)
Flight 2 should be successfully planned by the mock_uss.
The mock_uss, which is mock_uss is instructed to share invalid data with other USS, for negative test.
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
The mock_uss, which is mock_uss is instructed to share invalid data with other USS, for negative test.
mock_uss is instructed to share invalid data with other USS, for negative test.

@BenjaminPelletier BenjaminPelletier merged commit d1d21a7 into interuss:main Dec 19, 2023
9 checks passed
@BenjaminPelletier BenjaminPelletier deleted the clean-up-data-validation branch December 19, 2023 17:40
github-actions bot added a commit that referenced this pull request Dec 19, 2023
* Clean up get op data validation scenario

* Clean up interaction search and severities

* Add missing imports

* Improve docs per comments, fix #425 d1d21a7
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.

infinite loop preventing f3548_self_contained to complete
3 participants