[mock_uss] Modify sharing behavior in mock_uss on injection #269

punamverma · 2023-10-19T06:09:59Z

This PR adds the ability to modify sharing behavior of mock_uss on injection, as would be needed by negative tests for data validation of SUT.

Created MockUssInjectFlightRequest (subclass of InjectFlightRequest) that enables adding MockUssFlightBehavior for modifying fields while sharing ops using GET or POST request.
Handled the MockUssFlightBehavior in mock uss routes_injection and routes_scdsc to share modified op in GET and POST requests. DSS has the correct operational_intent, its the operational_intent shared with USSes that is modified for the purpose of writing data validation tests.
Added the ability to inject MockUssFlightBehavior through flight_planner's request_flight method.

BenjaminPelletier

I have to get to bed; more comments tomorrow, but here's what I have so far.

BenjaminPelletier · 2023-10-19T06:50:28Z

monitoring/monitorlib/mock_uss_interface/mock_uss_scd_injection_api.py

+class MockUssFlightBehavior(ImplicitDict):
+    modify_sharing_methods: List[str]
+    modify_fields: dict


These are very important to document as we're essentially establishing a new interface for mock_uss. Also, remember that flight planning is used for many testing activities, not just F3548-21, so we should be more specific about what sharing and field modifications we're making.

Added documentation.

BenjaminPelletier · 2023-10-19T06:59:13Z

monitoring/uss_qualifier/scenarios/flight_planning/test_steps.py

@@ -92,7 +97,7 @@ def plan_flight_intent(
    scenario: TestScenarioType,
    test_step: str,
    flight_planner: FlightPlanner,
-    flight_intent: InjectFlightRequest,
+    flight_intent: MockUssInjectFlightRequest,


These are test steps that are performed mostly with USSs that are not mock_uss instances so callers of this function often will not have an augmented request. The type of this variable should remain InjectFlightRequest, and a MockUssInjectFlightRequest can still be provided for this variable since it is an InjectFlightRequest.

BenjaminPelletier · 2023-10-19T07:01:22Z

monitoring/uss_qualifier/scenarios/flight_planning/test_steps.py

+import time
+from datetime import datetime


These imports appear unnecessary

BenjaminPelletier · 2023-10-19T07:07:35Z

monitoring/monitorlib/clients/flight_planning/client_scd.py

@@ -45,6 +49,7 @@ def _inject(
        flight_id: FlightID,
        flight_info: FlightInfo,
        execution_style: ExecutionStyle,
+        mod_flight_behavior: Optional[MockUssFlightBehavior] = None,


This client is a particular implementation of the generic client -- we shouldn't change method signatures as that will prevent the use of other implementations of the generic client. Instead, it seems reasonable to add an additional_fields: Optional[dict] = None argument to this function in FlightPlannerClient (and therefore here as well) whose fields get added to the request body just before it's dispatched to the remote server. Doing it this way will allow easy adaptation for other implementations of FlightPlannerClient of which there will soon be one more (for flight_planner interface).

Added additional_fields.

punamverma · 2023-10-19T20:45:30Z

I have to get to bed; more comments tomorrow, but here's what I have so far.

Thanks for the comments. Sorry, didn't mean to disturb you.

BenjaminPelletier · 2023-10-19T21:21:36Z

I have to get to bed; more comments tomorrow, but here's what I have so far.

Thanks for the comments. Sorry, didn't mean to disturb you.

Oh, no, not at all -- I only review when I'm willing to work. Please feel free to push/comment/update/message/etc at any time regardless of time of day. I just usually like providing a complete set of comments, so wanted to explain why this set was not complete (seemed useful to get these out sooner rather than waiting for a complete set)

punamverma · 2023-10-24T07:18:24Z

@BenjaminPelletier I have made changes using additional_fields. Could you please check. Thanks.

BenjaminPelletier · 2023-10-24T15:36:40Z

I can help with the merge on this one -- I'll make a PR to your branch

BenjaminPelletier

This PR both merges main and addresses these comments

BenjaminPelletier · 2023-10-24T16:03:53Z

monitoring/mock_uss/scdsc/database.py

@@ -16,6 +20,7 @@ class FlightRecord(ImplicitDict):
    op_intent_injection: scd_injection_api.OperationalIntentTestInjection
    flight_authorisation: scd_injection_api.FlightAuthorisationData
    op_intent_reference: OperationalIntentReference
+    mod_op_sharing_behavior: Optional[MockUssFlightBehavior]


We don't need to retain the capability to omit this field in an output, so we should default this field to None so we can be sure it's always present.

BenjaminPelletier · 2023-10-24T16:05:59Z

monitoring/mock_uss/scdsc/routes_injection.py

    except ValueError as e:
        msg = "Create flight {} unable to parse JSON: {}".format(flight_id, e)
        return msg, 400
    json, code = inject_flight(flight_id, req_body)
    return flask.jsonify(json), code


+def op_to_share(


This function is nearly identical to op_intent_from_flightrecord; let's delete this one and just use op_intent_from_flightrecord (we can compute the FlightRecord just a little earlier when this function is currently being used).

BenjaminPelletier · 2023-10-24T16:07:26Z

monitoring/mock_uss/scdsc/routes_scdsc.py

-    response = GetOperationalIntentDetailsResponse(
-        operational_intent=op_intent_from_flightrecord(flight),
-    )
+    response = {"operational_intent": op_intent_from_flightrecord(flight)}


There's no reason to drop typing here -- let's keep this how it was

I think I had tried that. But, that lead to validation of the operational_intent as per GetOperationalIntentDetailsResponse. Whereas, we want to have invalid data in our test.

Types are validated upon parsing, but not upon construction -- an invalid value can be provided upon construction.

BenjaminPelletier · 2023-10-24T16:08:30Z

monitoring/mock_uss/scdsc/routes_scdsc.py

+    op_intent = OperationalIntent(reference=ref, details=details)
+    method = "GET"
+    if "mod_op_sharing_behavior" in flight:
+        mod_op_sharing_behavior = ImplicitDict.parse(


There's no need to parse here; the FlightRecord is already typed such that flight.mod_op_sharing_behavior is a MockUssFlightBehavior

Yes, thanks. I think I hadn't defaulted to None, as you suggested in an earlier comment, that's why my tests had failed and I put this check.

BenjaminPelletier · 2023-10-24T16:09:35Z

monitoring/mock_uss/scdsc/routes_scdsc.py

+            if method not in mod_op_sharing_behavior.modify_sharing_methods:
+                return OperationalIntent(reference=ref, details=details)
+        if mod_op_sharing_behavior.modify_fields is not None:
+            if "operational_intent_reference" in mod_op_sharing_behavior.modify_fields:


Instead of looking for these fields specifically by name, let's just have modify_fields apply to the entire OperationalIntent -- that's simpler and less brittle.

I think that leads to validation of the contents for OperationalIntent. We want to have invalid data there. I ll check it with your PR as well. Thanks.

Why would having modify_fields apply directly to OperationalIntent cause validation versus applying to the two member separately?

Let me pull and check, as earlier I tried it, and while running my tests, I had come across validation errors. But, let me pull your PR and check. Could have been some issue with my code. Will check and let you know if I see any issues. Thanks

BenjaminPelletier · 2023-10-24T16:25:07Z

monitoring/monitorlib/mock_uss_interface/mock_uss_scd_injection_api.py

@@ -0,0 +1,30 @@
+from implicitdict import ImplicitDict


This file relates to client behavior of mock_uss; let's put it in monitorlib/clients/mock_uss

BenjaminPelletier · 2023-10-24T16:33:21Z

monitoring/monitorlib/clients/flight_planning/client_scd.py

-        return self._inject(str(uuid.uuid4()), flight_info, execution_style)
+        return self._inject(
+            str(uuid.uuid4()), flight_info, execution_style, additional_fields
+        )

    def try_update_flight(


It seems surprising that we would be able to add fields when planning a flight, but not when updating -- let's add additional_fields here.

BenjaminPelletier · 2023-10-24T16:52:52Z

monitoring/mock_uss/scdsc/routes_injection.py

+    return op_intent
+
+
+def mock_uss_flight_behavior_in_req(


Functions that are not document and not intended to be used outside the current file should be prefixed with an underscore

BenjaminPelletier · 2023-10-24T16:53:51Z

monitoring/mock_uss/scdsc/routes_injection.py

+
+def mock_uss_flight_behavior_in_req(
+    req_body: InjectFlightRequest,
+) -> MockUssFlightBehavior:


Because this function can also return None, the type annotation should be Optional[MockUssFlightBehavior]

BenjaminPelletier · 2023-10-24T17:07:33Z

monitoring/mock_uss/scdsc/routes_injection.py

        if datetime.utcnow() > deadline:
+            logger.debug(f"Now {datetime.utcnow} Deadline {deadline} ")


utcnow is a function; it seems like you want to invoke it here (and below). Also, there is a log message immediately below; let's add the content there instead of making a new message.

…ion_to_mock_uss Modified behavior injection to mock uss

* Modify sharing behavior in mock_uss on injection * Added additional_fields to InjectFlightRequest * Fix format * Added documentation for MockUssFlightBehavior * Fix format * Implement PR suggestions --------- Co-authored-by: Benjamin Pelletier <[email protected]> 0b6beea

Modify sharing behavior in mock_uss on injection

c2a35c1

BenjaminPelletier reviewed Oct 19, 2023

View reviewed changes

punamverma marked this pull request as draft October 19, 2023 19:11

punamverma added 5 commits October 23, 2023 21:33

Added additional_fields to InjectFlightRequest

299ba85

Merge branch 'main' into modified_behavior_injection_to_mock_uss

0769404

Fix format

89c8351

Added documentation for MockUssFlightBehavior

15a5cba

Fix format

50cb3b2

BenjaminPelletier added 2 commits October 24, 2023 15:41

Merge branch 'main' into modified_behavior_injection_to_mock_uss

a3e83d7

Implement PR suggestions

0b99728

BenjaminPelletier reviewed Oct 24, 2023

View reviewed changes

BenjaminPelletier mentioned this pull request Oct 24, 2023

Modified behavior injection to mock uss punamverma/monitoring#2

Merged

punamverma marked this pull request as ready for review October 24, 2023 19:00

punamverma added 2 commits October 24, 2023 12:02

Merge pull request #2 from BenjaminPelletier/modified_behavior_inject…

0e0911d

…ion_to_mock_uss Modified behavior injection to mock uss

Merge branch 'main' into modified_behavior_injection_to_mock_uss

f3631ff

BenjaminPelletier approved these changes Oct 24, 2023

View reviewed changes

BenjaminPelletier changed the title ~~Modify sharing behavior in mock_uss on injection~~ [mock_uss] Modify sharing behavior in mock_uss on injection Oct 24, 2023

BenjaminPelletier merged commit 0b6beea into interuss:main Oct 24, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mock_uss] Modify sharing behavior in mock_uss on injection #269

[mock_uss] Modify sharing behavior in mock_uss on injection #269

punamverma commented Oct 19, 2023

BenjaminPelletier left a comment

BenjaminPelletier Oct 19, 2023

punamverma Oct 24, 2023

BenjaminPelletier Oct 19, 2023

punamverma Oct 24, 2023

BenjaminPelletier Oct 19, 2023

punamverma Oct 24, 2023

BenjaminPelletier Oct 19, 2023

punamverma Oct 24, 2023

punamverma commented Oct 19, 2023

BenjaminPelletier commented Oct 19, 2023

punamverma commented Oct 24, 2023

BenjaminPelletier commented Oct 24, 2023

BenjaminPelletier left a comment •

edited

Loading

BenjaminPelletier Oct 24, 2023

BenjaminPelletier Oct 24, 2023

BenjaminPelletier Oct 24, 2023

punamverma Oct 24, 2023

BenjaminPelletier Oct 24, 2023

BenjaminPelletier Oct 24, 2023

punamverma Oct 24, 2023

BenjaminPelletier Oct 24, 2023

punamverma Oct 24, 2023

BenjaminPelletier Oct 24, 2023

punamverma Oct 24, 2023 •

edited

Loading

BenjaminPelletier Oct 24, 2023

BenjaminPelletier Oct 24, 2023

BenjaminPelletier Oct 24, 2023

BenjaminPelletier Oct 24, 2023

BenjaminPelletier Oct 24, 2023

		if datetime.utcnow() > deadline:
		logger.debug(f"Now {datetime.utcnow} Deadline {deadline} ")

[mock_uss] Modify sharing behavior in mock_uss on injection #269

[mock_uss] Modify sharing behavior in mock_uss on injection #269

Conversation

punamverma commented Oct 19, 2023

BenjaminPelletier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punamverma commented Oct 19, 2023

BenjaminPelletier commented Oct 19, 2023

punamverma commented Oct 24, 2023

BenjaminPelletier commented Oct 24, 2023

BenjaminPelletier left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punamverma Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminPelletier left a comment •

edited

Loading

punamverma Oct 24, 2023 •

edited

Loading