-
Notifications
You must be signed in to change notification settings - Fork 8
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
Organise testing utils #567
Conversation
- and sort test utils into submodules
…ource/dodal into 564_organise_testing_utils
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #567 +/- ##
==========================================
+ Coverage 92.78% 93.20% +0.42%
==========================================
Files 99 104 +5
Lines 3769 4062 +293
==========================================
+ Hits 3497 3786 +289
- Misses 272 276 +4 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I'm not convinced about the structure as whole yet. Could you make the changes in hyperion so we have an example of how this improves code in a downstream app? Then I think maybe it needs a brief in person discussion
|
||
|
||
async def get_mock_device() -> Synchrotron: | ||
async with DeviceCollector(mock=True): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: I think from looking at the docs DeviceCollector
doesn't need to be async e.g. you can just do:
with DeviceCollector(mock=True):
device = Synchrotron()
Then the whole function can be synchronous too. This is true in multiple places in this PR, can you do the same there?
src/dodal/testing_utils/constants.py
Outdated
SYNCHR_RING_CURRENT = 0.556677 | ||
SYNCHR_USER_COUNTDOWN = 55.0 | ||
SYNCHR_START_COUNTDOWN = 66.0 | ||
SYNCHR_END_COUNTDOWN = 77.0 | ||
SYNCHR_BEAM_ENERGY = 3.0158 | ||
SYNCHR_MODE = "Injection" | ||
SYNCHR_NUMBER = "number" | ||
SYNCHR_STRING = "string" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could: I sort of feel these should be with the device too but I don't want to pollute the device namespace too much. Maybe we add the convention of as well as get_mock_device
you also have a dict/dataclass of TESTING_CONSTS
?
async def mock_aperturescatterguard_with_call_log(): | ||
call_log = MagicMock() | ||
ap_sg = await get_mock_ap_sg() | ||
with ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: patch_ophyd_async_motor
isn't a context manager
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it is, it's type-hinted wrong in ophyd-async, fix has been merged but not released, doesn't affect functionality
patch_ophyd_async_motor(ap_sg.aperture.y, call_log=call_log), | ||
patch_ophyd_async_motor(ap_sg.aperture.z, call_log=call_log), | ||
patch_ophyd_async_motor(ap_sg.scatterguard.x, call_log=call_log), | ||
patch_ophyd_async_motor(ap_sg.scatterguard.y, call_log=call_log), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should: Can we do this patching in get_mock_ap_sg
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, it leads to circular import errors if we import test utils to device classes and then device classes to test utils
|
||
|
||
@pytest.fixture | ||
def mock_eiger_and_stage(mock_eiger): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Rename to something like mock_eiger_with_noop_stage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, that's actually what I had first and I changed it to this
dc9e8ea
to
34edbc3
Compare
34edbc3
to
6aa2310
Compare
never mind, this is too hard, I now think it's better to just copy and paste stuff everywhere |
Fixes #564
Moves many mock device test fixtures and utility functions to a central place under
src/
, so that they can easily be reused by application codeInstructions to reviewer on how to test:
Checks for reviewer