Distinguish shutter types in existing tests

[CoePaul]

• Renames ambiguously named "shutter tests" according to their specific shutter type Examples (of renaming) covered here: shutter -> zebra_shutter shutter -> hutch_shutter

Fixes ambiguous names ( side work for Issue #1066 )

Instructions to reviewer on how to test:

1. Run all tests in project
2. Confirm tests all pass

Checks for reviewer

• Would the PR title make sense to a scientist on a set of release notes
• If a new device has been added does it follow the standards
• If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
• Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.75%. Comparing base (f135ed4) to head (8f92c86).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1068   +/-   ##
=======================================
  Coverage   97.75%   97.75%           
=======================================
  Files         170      170           
  Lines        6849     6851    +2     
=======================================
+ Hits         6695     6697    +2     
  Misses        154      154           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[DominicOram]

Looks good, thanks! Mostly minor points. As discussed, I think these should also be in test_hutch_conditional_shutter.py.

[DominicOram]

Should: We use pytest rather than unittest. pytest sort of uses the assert keyword instead so the way to do this in pytest is:

Suggested change

	fake_prefix = "I74"
	shutter_name = "GrandOldShutter"
	hcs = HutchConditionalShutter(fake_prefix, hutch_controlling_shutter, shutter_name)
	test_case = TestCase()
	test_case.assertIsInstance(hcs, HutchConditionalShutter)
	fake_prefix = "I74"
	shutter_name = "GrandOldShutter"
	hcs = HutchConditionalShutter(fake_prefix, hutch_controlling_shutter, shutter_name)
	assert isinstance(hcs, HutchConditionalShutter)

[DominicOram]

Could: You can use @pytest.mark.parametrize to do this for you:

Suggested change

	async def test_shutter_created_for_eh_one_without_raising_errors():
	hutch_from_which_shutter_is_controlled = HutchState.EH1
	await unit_test_shutter_created_without_raising_errors(
	hutch_from_which_shutter_is_controlled
	)
	async def test_shutter_created_for_eh_two_without_raising_errors():
	hutch_from_which_shutter_is_controlled = HutchState.EH2
	await unit_test_shutter_created_without_raising_errors(
	hutch_from_which_shutter_is_controlled
	)
	@pytest.mark.parametrize(
	"hutch_from_which_shutter_is_controlled", [HutchState.EH1, HutchState.EH2]
	)
	async def test_shutter_created_without_raising_errors(
	hutch_from_which_shutter_is_controlled,
	):
	await unit_test_shutter_created_without_raising_errors(
	hutch_from_which_shutter_is_controlled
	)

This will create two tests called test_shutter_created_without_raising_errors[EH1] and test_shutter_created_without_raising_errors[EH2].

In the case of just having the two options it's neither here nor there but when there's like 3 or 4 it tidies up a lot of the copy/paste.

[DominicOram]

Could: This actually need not be an async function. To a first approximation an async function is only needed if you are calling other async functions from it. The constructor of HutchConditionalShutter is synchronous and so this isn't needed. If you were calling the set here it would be needed. Does no harm though so not the end of the world to leave it in.

[DominicOram]

Nit: It's a little odd that you're calling a function called unit_test_shutter_created_without_raising_errors and expecting to actually raise the error. But not the end of the world.

[DominicOram]

Great, thank you! Snake_case is the only comment I really care about, happy for you to take/leave the others.

[DominicOram]

Could: I would combine the common bits here:

async def make_mock_shutter(hutch: HutchState) -> HutchConditionalShutter:
    shutter = HutchConditionalShutter("", hutch, name="mock_shutter")
    await shutter.connect(mock=True)

    def set_status(value: ShutterDemand, *args, **kwargs):
        value_sta = ShutterState.OPEN if value == "Open" else ShutterState.CLOSED
        set_mock_value(shutter.shutter.status, value_sta)

    callback_on_mock_put(shutter.shutter.control, set_status)

    return shutter


@pytest.fixture
async def eh1_shutter(RE: RunEngine) -> HutchConditionalShutter:
    return await make_mock_shutter(HutchState.EH1)


@pytest.fixture
async def eh2_shutter(RE: RunEngine) -> HutchConditionalShutter:
    return await make_mock_shutter(HutchState.EH2)

[DominicOram]

Should: This should be snake case as a python function

Suggested change

	def assertIsInstance(an_instance, expected_type):
	assert isinstance(an_instance, expected_type)
	def assert_is_instance(an_instance, expected_type):
	assert isinstance(an_instance, expected_type)

[DominicOram]

Could: I would also personally argue that assert_is_instance(x, y) is no more readable than assert isinstance(x, y) and so feel this function is redundant but I don't feel that strongly.

[CoePaul]

Might well be "no more readable" but it is more ophiomorphic ( Greek ophis = snake )

[DominicOram]

Great, thank you!