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.

Sign up for GitHub

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

Jump to bottom

Add tests for EigerHandler and EigerConfigHandler #53

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+241 −39
Open

Add tests for EigerHandler and EigerConfigHandler #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d2a86eb
Add tests for EigerHandler and EigerConfigHandler
jsouter Nov 8, 2024
f6b2abd
implement review comments for update-handlers-tests
jsouter Nov 15, 2024
2d7bba0
Update top controller stale_parameters after subsystem controllers
jsouter Nov 21, 2024
26597b5
use Mocks to simplify mock_connection
jsouter Nov 25, 2024
aaad89f
Add dummy_getter and dummy_putter fixtures to reduce code reuse
jsouter Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions src/fastcs_eiger/eiger_controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ class EigerConfigHandler(EigerHandler):

first_poll_complete: bool = False

async def update(self, controller: "EigerController", attr: AttrR) -> None:
async def update(self, controller: "EigerSubsystemController", attr: AttrR) -> None:
# Only poll once on startup
if not self.first_poll_complete:
await super().update(controller, attr)
Expand All @@ -109,7 +109,9 @@ async def update(self, controller: "EigerController", attr: AttrR) -> None:

self.first_poll_complete = True

async def config_update(self, controller: "EigerController", attr: AttrR) -> None:
async def config_update(
self, controller: "EigerSubsystemController", attr: AttrR
) -> None:
await super().update(controller, attr)


Expand Down Expand Up @@ -228,11 +230,11 @@ async def initialise(self) -> None:
@scan(0.1)
async def update(self):
"""Periodically check for parameters that need updating from the detector."""
controller_updates = [c.update() for c in self.get_sub_controllers().values()]
await asyncio.gather(*controller_updates)
await self.stale_parameters.set(
any(c.stale_parameters.get() for c in self.get_sub_controllers().values())
)
controller_updates = [c.update() for c in self.get_sub_controllers().values()]
await asyncio.gather(*controller_updates)


class EigerSubsystemController(SubController):
Expand Down
108 changes: 84 additions & 24 deletions tests/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from typing import Any

import pytest
from pytest_mock import MockerFixture

# Prevent pytest from catching exceptions when debugging in vscode so that break on
# exception works correctly (see: https://github.com/pytest-dev/pytest/issues/7409)
Expand Down Expand Up @@ -115,7 +114,7 @@ def pytest_internalerror(excinfo: pytest.ExceptionInfo[Any]):


@pytest.fixture
def detector_config_keys():
def detector_config_keys() -> list[str]:
return _detector_config_keys


Expand All @@ -125,26 +124,87 @@ def detector_status_keys():


@pytest.fixture
def mock_connection(mocker: MockerFixture):
connection = mocker.patch("fastcs_eiger.http_connection.HTTPConnection")
connection.get = mocker.AsyncMock()

async def _connection_get(uri):
if "detector/api/1.8.0/status/keys" in uri:
return _detector_status_keys
elif "detector/api/1.8.0/config/keys" in uri:
return _detector_config_keys
elif "monitor/api/1.8.0/status/keys" in uri:
return _monitor_status_keys
elif "monitor/api/1.8.0/config/keys" in uri:
return _monitor_config_keys
elif "stream/api/1.8.0/status/keys" in uri:
return _stream_status_keys
elif "stream/api/1.8.0/config/keys" in uri:
return _stream_config_keys
else:
# dummy response
return {"access_mode": "rw", "value": 0.0, "value_type": "float"}
def monitor_config_keys():
return _monitor_config_keys


@pytest.fixture
def monitor_status_keys():
return _monitor_status_keys


@pytest.fixture
def stream_config_keys():
return _stream_config_keys


connection.get.side_effect = _connection_get
return connection
@pytest.fixture
def stream_status_keys():
return _stream_status_keys


@pytest.fixture
def keys_mapping() -> dict[str, list[str]]:
return {
"detector/api/1.8.0/status/keys": _detector_status_keys,
"detector/api/1.8.0/config/keys": _detector_config_keys,
"monitor/api/1.8.0/status/keys": _monitor_status_keys,
"monitor/api/1.8.0/config/keys": _monitor_config_keys,
"stream/api/1.8.0/status/keys": _stream_status_keys,
"stream/api/1.8.0/config/keys": _stream_config_keys,
}


@pytest.fixture
def put_response_mapping() -> dict[str, list[str]]:
time_keys = [
"bit_depth_image",
"bit_depth_readout",
"count_time",
"countrate_correction_count_cutoff",
"frame_count_time",
"frame_time",
]
energy_keys = [
"element",
"flatfield",
"incident_energy",
"photon_energy",
"threshold/1/energy",
"threshold/1/flatfield",
"threshold/2/energy",
"threshold/2/flatfield",
"threshold_energy",
"wavelength",
]
threshold_energy_keys = [
"flatfield",
"threshold/1/energy",
"threshold/1/flatfield",
"threshold/2/flatfield",
"threshold_energy",
]
return {
"auto_summation": ["auto_summation", "frame_count_time"],
"count_time": time_keys,
"frame_time": time_keys,
"flatfield": ["flatfield", "threshold/1/flatfield"],
"incident_energy": energy_keys,
"photon_energy": energy_keys,
"pixel_mask": ["pixel_mask", "threshold/1/pixel_mask"],
"threshold/1/flatfield": ["flatfield", "threshold/1/flatfield"],
"roi_mode": ["count_time", "frame_time", "roi_mode"],
"threshold_energy": threshold_energy_keys,
"threshold/1/energy": threshold_energy_keys,
"threshold/2/energy": [
"flatfield",
"threshold/1/flatfield",
"threshold/2/energy",
"threshold/2/flatfield",
],
"threshold/1/mode": ["threshold/1/mode", "threshold/difference/mode"],
"threshold/2/mode": ["threshold/2/mode", "threshold/difference/mode"],
"threshold/1/pixel_mask": ["pixel_mask", "threshold/1/pixel_mask"],
"threshold/difference/mode": ["difference_mode"],
# replicating API inconsistency
}
162 changes: 151 additions & 11 deletions tests/test_controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import asyncio
from collections.abc import Awaitable, Callable
from typing import Any
from unittest import mock

import pytest
from fastcs.attributes import Attribute
Expand All @@ -7,20 +10,51 @@
from fastcs_eiger.eiger_controller import (
IGNORED_KEYS,
MISSING_KEYS,
EigerConfigHandler,
EigerController,
EigerDetectorController,
EigerHandler,
EigerMonitorController,
EigerStreamController,
)

_lock = asyncio.Lock()

Getter = Callable[[str], Awaitable[dict[str, Any] | list[str]]]
Putter = Callable[[str, Any], Awaitable[list[str]]]


@pytest.fixture
def dummy_getter(keys_mapping: dict[str, list[str]]) -> Getter:
# if not in mapping, get dummy parameter dict
async def _getter(uri: str):
return keys_mapping.get(
uri, {"access_mode": "rw", "value": 0.0, "value_type": "float"}
)

return _getter


@pytest.fixture
def dummy_putter(put_response_mapping: dict[str, list[str]]) -> Putter:
async def _putter(uri: str, _: Any):
key = uri.split("/", 4)[-1]
# return [key] if not in mapping
return put_response_mapping.get(key, [key])

return _putter


@pytest.mark.asyncio
async def test_detector_controller(
mock_connection, detector_config_keys, detector_status_keys
dummy_getter: Getter,
detector_config_keys: list[str],
detector_status_keys: list[str],
):
detector_controller = EigerDetectorController(mock_connection, _lock)
connection = mock.Mock()
connection.get = dummy_getter

detector_controller = EigerDetectorController(connection, _lock)
parameters = await detector_controller._introspect_detector_subsystem()
assert all(parameter.key not in IGNORED_KEYS for parameter in parameters)
for parameter in parameters:
Expand All @@ -44,35 +78,44 @@ async def test_detector_controller(


@pytest.mark.asyncio
async def test_monitor_controller_initialises(mock_connection):
subsystem_controller = EigerMonitorController(mock_connection, _lock)
async def test_monitor_controller_initialises(dummy_getter: Getter):
connection = mock.Mock()
connection.get = dummy_getter
subsystem_controller = EigerMonitorController(connection, _lock)
await subsystem_controller.initialise()


@pytest.mark.asyncio
async def test_stream_controller_initialises(mock_connection):
subsystem_controller = EigerStreamController(mock_connection, _lock)
async def test_stream_controller_initialises(dummy_getter: Getter):
connection = mock.Mock()
connection.get = dummy_getter
subsystem_controller = EigerStreamController(connection, _lock)
await subsystem_controller.initialise()


@pytest.mark.asyncio
async def test_detector_subsystem_controller(mock_connection):
subsystem_controller = EigerDetectorController(mock_connection, _lock)
async def test_detector_subsystem_controller(dummy_getter: Getter):
connection = mock.Mock()
connection.get = dummy_getter
subsystem_controller = EigerDetectorController(connection, _lock)
await subsystem_controller.initialise()

for attr_name in dir(subsystem_controller):
attr = getattr(subsystem_controller, attr_name)
if isinstance(attr, Attribute) and "threshold" in attr_name:
if attr_name == "threshold_energy":
continue
assert "Threshold" in attr.group
assert attr.group and "Threshold" in attr.group


@pytest.mark.asyncio
async def test_eiger_controller_initialises(mocker: MockerFixture, mock_connection):
async def test_eiger_controller_initialises(
mocker: MockerFixture, dummy_getter: Getter
):
eiger_controller = EigerController("127.0.0.1", 80)
connection = mocker.patch.object(eiger_controller, "connection")
connection.get = mock_connection.get
connection.get.side_effect = dummy_getter

await eiger_controller.initialise()
assert list(eiger_controller.get_sub_controllers().keys()) == [
"Detector",
Expand All @@ -82,3 +125,100 @@ async def test_eiger_controller_initialises(mocker: MockerFixture, mock_connecti
connection.get.assert_any_call("detector/api/1.8.0/status/state")
connection.get.assert_any_call("stream/api/1.8.0/status/state")
connection.get.assert_any_call("monitor/api/1.8.0/status/state")


@pytest.mark.asyncio
async def test_eiger_handler_update_updates_value(keys_mapping: dict[str, list[str]]):
connection = mock.Mock()

async def _get(uri: str) -> dict[str, Any] | list[str]:
if "state" in uri: # get 1 as value for state
return {"access_mode": "r", "value": 1, "value_type": "int"}
# if not in mapping, get dummy parameter dict
return keys_mapping.get(
uri, {"access_mode": "rw", "value": 0.0, "value_type": "float"}
)

connection.get = _get
subsystem_controller = EigerDetectorController(connection, _lock)
await subsystem_controller.initialise()

assert type(subsystem_controller.state.updater) is EigerHandler
assert subsystem_controller.state.get() == 0

# show that value changes after update is awaited
await subsystem_controller.state.updater.update(
subsystem_controller, subsystem_controller.state
)
assert subsystem_controller.state.get() == 1


@pytest.mark.asyncio
async def test_eiger_config_handler_put(dummy_getter: Getter, dummy_putter: Putter):
connection = mock.Mock()
connection.get.side_effect = dummy_getter
connection.put.side_effect = dummy_putter
subsystem_controller = EigerDetectorController(connection, _lock)
await subsystem_controller.initialise()
attr = subsystem_controller.threshold_1_energy
handler = attr.sender
assert isinstance(handler, EigerConfigHandler)
assert not subsystem_controller.stale_parameters.get()
await handler.put(subsystem_controller, attr, 100.0)
expected_changed_params = [
"flatfield",
"threshold/1/energy",
"threshold/1/flatfield",
"threshold/2/flatfield",
"threshold_energy",
]
assert subsystem_controller._parameter_updates == set(expected_changed_params)
assert subsystem_controller.stale_parameters.get()

# flatfields are ignored keys
subsystem_controller.threshold_energy.updater.config_update = mock.AsyncMock()

await subsystem_controller.update()
assert subsystem_controller.stale_parameters.get()
subsystem_controller.threshold_energy.updater.config_update.assert_called_once_with(
subsystem_controller, subsystem_controller.threshold_energy
)

await subsystem_controller.update()
# stale does not get set False unless there are no stale parameters at start of
# update call
assert not subsystem_controller.stale_parameters.get()
assert not subsystem_controller._parameter_updates
Copy link
Contributor

Choose a reason for hiding this comment

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

There is still a lot of logic going on in the Getter and Putter and I think it is because you are testing too much here.

All we want to do is test EigerHandler.put. We don't even need a Controller instance to do that. We just need to create an EigerHandler, call put and check the result (a method being called on the given controller).

I think this is all that is needed to test this method (minus the "difference_threshold" branch, which needs a separate test):

@pytest.mark.asyncio
async def test_eiger_config_handler_put(mocker: MockerFixture):
    handler = EigerHandler("/uri")
    controller = mocker.AsyncMock()

    await handler.put(controller, mocker.Mock(), 0.1)

    controller.connection.put.assert_awaited_once_with("/uri", 0.1)
    controller.queue_update.assert_awaited_once_with(
        controller.connection.put.return_value
    )

We can test EigerController._introspect_detector_subsystem, EigerController.initialise and EigerController.update each in a similarly simple unit test and in the system tests we can verify the interplay of all of these methods together against the tickit sim.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, think that makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK cool. Could you try doing something similar for all of these new tests?



@pytest.mark.asyncio
async def test_stale_parameter_propagates_to_top_controller(
mocker: MockerFixture,
dummy_getter: Getter,
dummy_putter: Putter,
):
eiger_controller = EigerController("127.0.0.1", 80)
connection = mocker.patch.object(eiger_controller, "connection")
connection.get.side_effect = dummy_getter
connection.put.side_effect = dummy_putter

await eiger_controller.initialise()

detector_controller = eiger_controller.get_sub_controllers()["Detector"]
attr = detector_controller.threshold_energy

assert not detector_controller.stale_parameters.get()
assert not eiger_controller.stale_parameters.get()

await attr.sender.put(detector_controller, attr, 100.0)
assert detector_controller.stale_parameters.get()
# top controller not stale until update called
assert not eiger_controller.stale_parameters.get()
await eiger_controller.update()
assert eiger_controller.stale_parameters.get()

# need to update again to make detector controller update its
# stale parameter attribute
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually a problem that we will need to figure out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've moved the stale check to the bottom of the controller's update loop so we no longer have to wait for two updates for it to change, but we still do need to wait for one update. It may be possible to have the subcontrollers prompt the parent controller to update its stale_parameter when it changes for any of the subcontrollers, but maybe that gets too complicated.

await eiger_controller.update()
assert not detector_controller.stale_parameters.get()
assert not eiger_controller.stale_parameters.get()
Loading