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

TST: Implement test IOC #34

Merged
merged 6 commits into from
Aug 29, 2024
Merged

Conversation

shilorigins
Copy link
Collaborator

@shilorigins shilorigins commented Jun 11, 2024

Description

Add a fixture and module for instantiating IOCs that make PVs accessible via EPICS during tests and for live interaction (closes #31)

Fix a bug where the dummy_cl fixture was altering the SHIMS global variable, causing the IOC test to fail only if run after test_cl.py. I did this both by making a new dict in dummy_cl, and by copying SHIMS in ControlLayer.__init__ rather than assigning it to an instance attribute.

Motivation and Context

Motivation

This application needs to interface with EPICS via Channel Access and PV Access, so we need a way to test this interface. While mocking can be useful, using actual EPICS tools in at least some tests will make the test suite more complete. This test IOC system is intended to provide test PVs that use the same interface as and behave like production PVs, while being predicable, repeatable, and configurable.

Design

Because caproto IOCs use class attributes to store their PVs, all desired PVs must be passed in before the IOC class is defined, instantiated, and run (there may be a way to reconfigure IOCs after subclassing / instantiation, but I haven't figured it out). IOCFactory collects any "PV"s within a group of Entry trees, which includes Parameters, Setpoints, and Readbacks, then defines and instantiates a test IOC subclass. These instances can be used as context managers, automatically starting and stopping themselves for the test scope.

The prefix in the test example helps prevent test PVs from colliding with production PVs in the same EPICS namespace.

Intended use

This IOC fixture is intended to be used for integration tests, where we can scope the fixture to the module and limit the number of IOC instantiations.

To run the IOC for live interaction, run python -m superscore.tests.ioc.linac in a terminal. You can then query the IOC's PVs from another terminal, or background the process and query it from the same terminal.

How Has This Been Tested?

Basic PV interaction test in test_ioc.py

Where Has This Been Documented?

This PR

Pre-merge checklist

  • Code works interactively
  • Code follows the style guide
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@shilorigins shilorigins added the enhancement New feature or request label Jun 11, 2024
@shilorigins shilorigins added this to the Minimum Viable Product milestone Jun 11, 2024
@shilorigins shilorigins self-assigned this Jun 11, 2024
@tangkong
Copy link
Contributor

tangkong commented Jul 2, 2024

Is there still work being done on this? Or is it ready for review?

@shilorigins
Copy link
Collaborator Author

I haven't decided how to set up the actual fixture. The code here works though, so I can touch it up and submit for review if we don't mind adding the fixture in a later PR.

@tangkong
Copy link
Contributor

tangkong commented Jul 4, 2024

A suggestion that might result in a refactor, but might also fix our segfault issues:

Maybe we should make explicit modules that hold these iocs, similar to caproto.ioc_examples.random_walk etc. This would allow us to have people try superscore against the GUI outside of the test suite. Critically, this would also let us use the test-ioc pattern used in caproto: run_example_ioc

Maybe a hook in pytest would regenerate those module files based on the fixtures we've defined as well

@shilorigins shilorigins changed the title Implement test IOC TST: Implement test IOC Jul 19, 2024
@shilorigins shilorigins force-pushed the devagr/test-ioc branch 2 times, most recently from fa16e7b to f6e5b6f Compare August 1, 2024 21:44
@shilorigins
Copy link
Collaborator Author

caproto keeps fighting me, but I've gotten it to the point where we can launch the IOC and manually query it, strings don't get mapped to char arrays, and the test passes when run individually (pytest tests/test_ioc.py). However, the test fails when the whole suite is run (pytest). Once I figure out the discrepancy, I still need to clean up the code as well.

TempIOC can be used as a context manager to make PVs accessible
via EPICS during tests. IOCFactory uses Entry trees to define and
instantiate TempIOCs, so that tests can choose which PVs they
need to be available.
Since ControlLayer sets its .shims to SHIMS by default, setting
attributes in dummy cl .shims changes SHIMS for the rest of the
test session. Creating a new dict for the dummy_shim fixture avoids
this side-effect.

For redundancy, I also changed ControlLayer.__init__ to copy SHIMS
instead of using the global instance.
@shilorigins
Copy link
Collaborator Author

I resolved the individual vs suite testing issue (dummy_cl fixture was altering the SHIMS global variable). There's some work to be done making the module and IOC fixture more interdependent, but this is enough for one PR. I only included basic tests for the IOC itself, not integration tests.

@shilorigins shilorigins marked this pull request as ready for review August 19, 2024 22:27
Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

I think this looks really nice! Good job finding the SHIMS issue and winning the fight.

Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

A hard fought, and pretty awesome bit of test suite functionality. I just had a couple of questions, but nothing that needs to change. Nice work! 👍

@@ -32,7 +32,7 @@ class ControlLayer:
def __init__(self, *args, shims: Optional[List[str]] = None, **kwargs):
if shims is None:
# load all available shims
self.shims = SHIMS
self.shims = SHIMS.copy()
Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, I think this is fine and no changes need to be made. But I do want to clarify:

Was this changed to provide assurance that we couldn't accidentally clobber the global SHIMS? I think my intent was for there to be only one shim of each type per session, on the off chance we started to track running coroutines or caching values. I didn't implement any of that, so there's no reason to revert this right now.

I also thought the way you fixed this in the test suite would have been sufficient, but better safe than sorry haha

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change is redundant for fixing the issue, but I figured it would help avoid similar mistakes in the future. If we want to configure shims for a session, as you say, we can revert this line later.



@pytest.fixture(scope='function')
def linac_backend():
def linac_data():
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to make this a fixture as well? I suppose if I ever find myself wanting to use it as a fixture we could add the decorator.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Marking it as a fixture makes it difficult to use outside of a pytest context, which I ran into when trying to run the test IOC. You can still import and call it in other fixtures, it just won't happen automatically. If we do want fixture controls, I suppose we could make a fixture that simply calls and returns linac_data to get the benefits of both options.

return IOC

@staticmethod
def collect_pvs(entries: Iterable[Entry]) -> Iterable[Union[Parameter, Setpoint, Readback]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

We've written this function a couple of times at this point (or at least similar looking ones), we should find a good way to DRY this out (later, not this PR)

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 agree. As a note for the future PR, the reason I didn't do it here is because it requires untangling backend calls in the client version, which are required for resolving UUIDs.

@tangkong
Copy link
Contributor

@shilorigins Any last thoughts or can we merge this?

@shilorigins shilorigins merged commit 77c96c6 into pcdshub:master Aug 29, 2024
11 checks passed
@shilorigins shilorigins deleted the devagr/test-ioc branch August 29, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setup caproto test IOC
3 participants