Tickets/dm 42402 #124
Tickets/dm 42402 #124
Conversation
| True to enable, False to disable the specified force component | ||
| controlled by the slew controller. | ||
| """ | ||
| setting_key = self._map_m1m3_slew_setting_to_name(slew_setting) |
There was a problem hiding this comment.
I don't think you need this method. You can get the "name" of the enumeration by using:
slew_setting.name
There was a problem hiding this comment.
I removed the map method here, but I needed to create a different version of it to deal with mock methods.
| for key, attr in expected_attributes.items(): | ||
| if not hasattr(settings_event, attr): | ||
| raise RuntimeError( | ||
| f"Expected attribute '{attr}' not found in slew controller settings event." |
There was a problem hiding this comment.
Maybe print what are the valid ones? something like.
f"Expected attribute '{attr}' not found in slew controller settings event. "
f"Must be one of {expected_attributes.keys()}."
tests/maintel/test_mtcs.py
Outdated
| desired_setting: MTM1M3.SetSlewControllerSettings, | ||
| desired_value: bool, | ||
| ) -> None: | ||
| # Prepare initial settings |
There was a problem hiding this comment.
Consider moving these things to the MTCSAsyncMock class. and actually implementing a mock to the internals of get_m1m3_slew_controller_settings, instead of mocking the entire thing.
Basically, in setup_types you should set an attribute
self._evt_slew_controller_settings = types.SimpleNamespace(
useAccelerationForces=False,
useBalanceForces=False,
triggerBoosterValves=False,
useVelocityForces=False,
)
then define a method to retrieve the value, mtm1m3_evt_slew_controller_settings:
async def mtm1m3_evt_slew_controller_settings(
self, *args: typing.Any, **kwargs: typing.Any
) -> types.SimpleNamespace:
await asyncio.sleep(self.heartbeat_time / 4.0)
return self._evt_slew_controller_settings
and set it up in the setup_mtm1m3 method:
m1m3_mocks = {
...,
"evt_slewControllerSettings.aget.side_effect": mtm1m3_evt_slew_controller_settings,
...
}
Then do a similar thing on the command side to mock the behavior of setting the value.
There was a problem hiding this comment.
I have it working, but not happy with everything. Please take a look and tell me if you see anything wrong or if you see things to be improved.
dsanmartim
force-pushed
the
tickets/DM-42402
branch
from
d81ce1d to
9767936
Compare
|
|
||
| setting_key = slew_setting.name | ||
|
|
||
| if setting_key is None: |
There was a problem hiding this comment.
I don't thing setting_key can even be None.
| True to enable, False to disable the specified force component | ||
| controlled by the slew controller. | ||
| """ | ||
| if not isinstance(slew_setting, MTM1M3.SetSlewControllerSettings): |
There was a problem hiding this comment.
I think you can remove this check here. Maybe replace the type hint of slew_setting to MTM1M3.SetSlewControllerSettings?
There was a problem hiding this comment.
If I remove this check, the test_set_m1m3_slew_controller_settings_with_invalid_setting fails.
| enable_slew_management = kwargs.get("enableSlewManagement") | ||
|
|
||
| if slew_setting_value is None or enable_slew_management is None: | ||
| raise ValueError( |
There was a problem hiding this comment.
Please, use RuntimeError instead.
|
|
||
| # Validate and convert the integer to the corresponding enumeration | ||
| # member name | ||
| try: |
There was a problem hiding this comment.
I think this try/except here is not really necessary. If you call MTM1M3.SetSlewControllerSettings(slew_setting_value) with an invalid slew_setting_value it is already going to raise a ValueError exception with enough information. So I think it is kind of duplicated effort to catch and re-raise the same exception with a different error message.
I suggest you just call
setting_enum = MTM1M3.SetSlewControllerSettings(slew_setting_value)
here
| setting_attr = self.mtcs.map_slew_setting_to_attribute(setting_enum) | ||
|
|
||
| if setting_attr is None: | ||
| raise ValueError(f"Invalid slew setting value: {slew_setting_value}") |
| self._evt_slew_controller_settings, setting_attr, enable_slew_management | ||
| ) | ||
| else: | ||
| raise ValueError( |
dsanmartim
force-pushed
the
tickets/DM-42402
branch
2 times, most recently
from
362a516 to
dae4691
Compare
dsanmartim
force-pushed
the
tickets/DM-42402
branch
from
dae4691 to
08b525e
Compare
Hello. Here is the first try of this ticket. I hope it is not too bad :)