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

Update core and tango tests to match structure of epics tests #723

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

jsouter
Copy link
Contributor

@jsouter jsouter commented Jan 8, 2025

Closing #715
very WIP, I should rework the tango version of OneOfEverythingDevice to create signals for each type as scalar, spectrum and image, so I can combine test_tango_signals and test_tango_one_of_everything.

Also trying to rework MonitorQueue to use the existing assert_value/assert_reading methods

@jsouter
Copy link
Contributor Author

jsouter commented Jan 29, 2025

Still very WIP, sketching out how to handle tango enums, also probably some debug comments left in.
test_tango_signals.py is a little broken, will probably merge with test_tango_one_of_everything.py when I figure out how everything should work.

@jsouter jsouter force-pushed the tests-structure branch 3 times, most recently from 71d937c to ebb58d8 Compare February 12, 2025 09:24
@jsouter
Copy link
Contributor Author

jsouter commented Feb 12, 2025

Seems like running the test context in a subprocess does allow us to run tango tests on windows without forking. Still need to fix some race conditions in the tango one_of_everything tests. Possibly I should add a fixture that adds a short sleep between the tango tests too as there is still a small chance of the tests freezing without this.

TangoDeviceConnector.connect_real also needs more attention, currently using a bit of a hack to guess the TRL when we provide a tango device with a proxy instead of a trl and the trl is remote (i.e. includes [protocol://]server:port)

@jsouter jsouter force-pushed the tests-structure branch 3 times, most recently from 30d68ce to d3606ba Compare February 12, 2025 14:30
@jsouter jsouter requested a review from coretl February 12, 2025 14:32
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will explode the amount of code, but I would prefer if the tango server was in a completely separate file to the ophyd device. This mirrors the epics code where there is a parallel database file and ophyd device, and they just happen to declare the same attributes and types. Would that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've separated the dataclasses which provide the tests with the initial values to check etc, but otherwise the ophyd device is just a TangoDevice(trl=...) call, do you want each of the signals in the to be explicitly declared in a subclass of TangoDevice?

elif info.data_format == AttrDataFormat.SPECTRUM:
return TangoEnumSpectrumConverter(list(info.enum_labels))
elif info.data_format == AttrDataFormat.IMAGE:
return TangoEnumImageConverter(list(info.enum_labels))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we also handle the override case for enums? Where a StrictEnum or SubsetEnum is defined as an override datatype and passed in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this for when we have a DevEnum on the server end or a DevString that we restrict to some set of values on the ophyd-async end?

I think the one other thing I'm stuck on at the moment is commands that are backed by DevEnum, it seems like commands can't have specified enum_labels so we can't easily construct the converters from the labels. It would be possible to provide an enum class if the command signal is being explicitly declared with an override datatype, but in the case where ophyd-async is automatically creating the child command from the tango device trl we would still have to get and set using the underlying int value instead of the string name. This isn't a problem with attributes, just commands.

@@ -88,6 +100,7 @@ class TangoProxy:
support_events: bool = True
_proxy: DeviceProxy
_name: str
_converter: TangoConverter = TangoConverter()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try and make this nicer. It made sense for me to move the converter to the TangoProxy classes so that the Readings in the backend and cache would have values of the same type. But then I'm not sure if we should have a DisconnectedTangoConverter and require that connect be called on AttributeProxy/CommandProxy and set the converter based on the config info there before we call any get/get_reading/put on it

use assert_reading and assert_value in MonitorQueue.assert_updates

Give default values for alarm_severity and timestamp fields in assert_reading
Add outline for enum tests for tango in everything device
test str image in tango

add tests for comparing bool and enum arrays with assert_* methods
Use convenience functions from testing module in tango.testing
Reset tango everything device values between tests
@jsouter jsouter marked this pull request as ready for review February 19, 2025 14:17
@jsouter
Copy link
Contributor Author

jsouter commented Feb 19, 2025

Note to self: I haven't touched the docs so I will have to go through and update those.

@jsouter jsouter requested a review from coretl February 19, 2025 14:40
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.

2 participants