Ticket 3176 keithley 2400 #101
Conversation
Ticket_3176_Keithley_2400
tests/keithley2400.py
Outdated
| self.log.info("Message") | ||
|
|
||
| def calculate_resistance_range(self, value): | ||
| for r in [2.1 * pow(10, i) for i in range(1, 8)]: |
There was a problem hiding this comment.
Nit: Usually use 10**i as opposed to pow(10, i)
There was a problem hiding this comment.
2.1 is a somewhat arbitrary value. It would be helpful to add a docstring explaining the justification for this algorithm (e.g. is this what the Keithley uses internally or is it one we made up)
tests/keithley2400.py
Outdated
|
|
||
| @has_log |
There was a problem hiding this comment.
Surprised to find this here. This is a LeWIS function, not an IOC test framework function. I don't think it does anything here.
tests/keithley2400.py
Outdated
| class Keithley2400Tests(unittest.TestCase): | ||
| """ | ||
| Tests for the keithley 2400. | ||
| """ | ||
|
|
||
| def setUp(self): | ||
| self._lewis, self._ioc = get_running_lewis_and_ioc("keithley_2400", DEVICE_PREFIX) | ||
| self._ioc = IOCRegister.get_running(DEVICE_PREFIX) |
There was a problem hiding this comment.
Duplicate definition of self._ioc - I think you only need the first line, remove the second
tests/keithley2400.py
Outdated
| def test_WHEN_voltage_and_current_are_set_THEN_readback_returns_valid_resistance(self): | ||
| self.ca.set_pv_value("OUTPUT:MODE:SP", "On") | ||
| for volts in [4.5, 6.7]: | ||
| for amps in [6.7, 4.5]: |
There was a problem hiding this comment.
Nit: nested for loops are often better expressed using itertools:
for volts, amps in itertools.product([4.5, 6.7], [6.7, 4.5]):
...
tests/keithley2400.py
Outdated
| @skip_if_recsim("Recsim does not work with lewis backdoor tests") | ||
| def test_WHEN_voltage_value_is_set_THEN_readback_returns_value_just_set(self): | ||
| self.ca.set_pv_value("OUTPUT:MODE:SP", "On") | ||
| for val in [1.23, 456.789, 1e-3]: |
There was a problem hiding this comment.
Consider defining a constant, TEST_OUTPUTS, at the top of the file for this list.
tests/keithley2400.py
Outdated
| def test_WHEN_ioc_is_started_THEN_ioc_is_not_disabled(self): | ||
| self.ca.assert_that_pv_is("DISABLE", "COMMS ENABLED") | ||
|
|
||
| def test_WHEN_output_mode_is_set_THEN_readback_updates_with_the_value_just_set(self): | ||
| for mode in ["On", "Off"]: | ||
| self.ca.assert_setting_setpoint_sets_readback(mode, "OUTPUT:MODE") | ||
|
|
||
| @skip_if_recsim("Recsim does not work with lewis backdoor tests") | ||
| def test_WHEN_output_mode_is_unset_THEN_current_readback_does_not_update(self): | ||
| self.ca.set_pv_value("CURR:RAW", 1.0) |
There was a problem hiding this comment.
Shouldn't typically be putting to a "RAW" record - these are implementation details. If you have a good justification for why you need to use the raw record here, put it in a comment.
tests/keithley2400.py
Outdated
|
|
||
| self._lewis.backdoor_set_on_device("voltage", 5.0) | ||
|
|
||
| self.ca.assert_that_pv_is_number("VOLT:RAW", 1.0) |
There was a problem hiding this comment.
Currently this test might sometimes pass even when it shouldn't, as the VOLT:RAW pv will take some time to update. To avoid the race condition here, use assert_that_pv_value_is_unchanged.
| def test_WHEN_range_is_set_THEN_readback_updates_with_the_value_just_set(self): | ||
| for val in [1.23, 456.789]: | ||
| self.ca.assert_setting_setpoint_sets_readback(val, "RES:RANGE") | ||
| @skip_if_recsim("Banded record behaviour too complex for recsim") |
tests/keithley2400.py
Outdated
|
|
||
| def test_WHEN_voltage_compliance_is_set_THEN_readback_updates_with_the_value_just_set(self): | ||
| for val in [1.23, 456.789]: | ||
| self.ca.assert_setting_setpoint_sets_readback(val, "V:COMPLIANCE") | ||
| for val in [1.23, 456.789, 1e-5, -1e-5]: |
There was a problem hiding this comment.
Why 1e-5 here and 1e-6 in the current compliance test above? Is this important?
I think these lists should be refactored out as a constant at the top of the file.
tests/keithley2400.py
Outdated
| @@ -12,30 +14,85 @@ | |||
| { | |||
| "name": DEVICE_PREFIX, | |||
| "directory": get_default_ioc_dir("KHLY2400"), | |||
| "emulator": "keithley_2400" | |||
There was a problem hiding this comment.
I also needed to add the following macros to get the tests to pass.
"macros": {
"IEOS": r"\\r\\n",
"OEOS": r"\\r\\n",
}
This is because the device uses variable terminators, if they are not specified in the test framework then it uses whatever is in a config (which may be wrong).
See ISISComputingGroup/IBEX#3176