[uss_qualifier] Create PlanningSequenceScenario base class for commonalities in nominal planning scenarios#1426
Conversation
| @abstractmethod | ||
| def run_planning_sequence(self, context: ExecutionContext): | ||
| """Run the main planning sequence of the test scenario, assuming the test scenario has already begun.""" | ||
| raise NotImplementedError() |
There was a problem hiding this comment.
Nit/Info looking at the PR: abstractmethod already takes care of forcing the implementation, so the raise is probably not needed.
There was a problem hiding this comment.
It seems like something needs to go in the body of the abstract method and raising is better than pass since the former will catch more types of problems. Is there a better alternative than raising or pass?
There was a problem hiding this comment.
(Will follow up on change in a separate PR if a better option is identified)
| @@ -70,22 +58,13 @@ class ConflictEqualPriorityNotPermitted(TestScenario): | |||
| flight2_activated: FlightInfoTemplate | |||
| flight2_nonconforming: FlightInfoTemplate | |||
There was a problem hiding this comment.
Could those be moved to PlanningSequenceScenario as well?
There was a problem hiding this comment.
I don't think every planning sequence would necessarily involve a Flight 2 at all, let alone one that is differentiated between Activated and Nonconforming. So, I think this is probably above the PlanningSequenceScenario abstraction, but I agree there could be another abstraction for the commonalities between conflict_equal_priority_not_permitted and conflict_higher_priority (but that would be a different PR).
| self.flight_intents_templates = ( | ||
| flight_intents.get_flight_intents() if flight_intents else {} | ||
| ) | ||
| try: | ||
| validate_flight_intent_templates( | ||
| self.flight_intents_templates, expected_flight_intents | ||
| ) | ||
| except ValueError as e: | ||
| raise ValueError( | ||
| f"`{self.me()}` TestScenario requirements for flight_intents not met: {e}" | ||
| ) | ||
|
|
||
| for efi in expected_flight_intents: | ||
| setattr( | ||
| self, | ||
| efi.intent_id.replace("equal_prio_", ""), | ||
| self.flight_intents_templates[efi.intent_id], | ||
| ) |
There was a problem hiding this comment.
That should be in the equal-priority scenario specifically -- thanks, and good catch. I'll have to investigate why this didn't cause a test failure
There was a problem hiding this comment.
There wasn't a problem before because the equal_prio_ attributes were being set on all subclass scenarios even if they weren't used. I've fixed the issue by simply using the full explicit intent_id for these rather than making (somewhat confusing) aliases.
3 participants
The root of #1424 is that two very similar test scenarios (conflict_equal_priority_not_permitted and conflict_higher_priority) were imperfectly duplicating common elements. This PR fixes that problem by establishing a PlanningSequenceScenario base class with the commonalities between these scenarios and factoring out the redundancies into that base class. This incidentally results in the prerequisite clear-area check from conflict_equal_priority_not_permitted to be applied to conflict_higher_priority, and this is reflected in a documentation change.
None of these scenarios could be run without flight intents, so that resource's optionality is removed also.