From 52a4aaa2fd127815b8239b3de64a663ac5668eba Mon Sep 17 00:00:00 2001 From: Michael Abbott Date: Thu, 3 Feb 2022 10:38:59 +0000 Subject: [PATCH 1/4] Allow NELM and length as aliases for waveform length Fixes issue #37 --- softioc/builder.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/softioc/builder.py b/softioc/builder.py index 679c4c6c..23b435cf 100644 --- a/softioc/builder.py +++ b/softioc/builder.py @@ -134,6 +134,19 @@ def Action(name, **fields): } +def _get_length(fields, default = None): + '''Helper function for getting 'length' or 'NELM' from arguments''' + + if 'length' in fields: + assert 'NELM' not in fields, 'Cannot specify NELM and length together' + return fields.pop('length') + elif 'NELM' in fields: + return fields.pop('NELM') + else: + assert default is not None, 'Must specify waveform length' + return default + + def _waveform(value, fields): '''Helper routine for waveform construction. If a value is given it is interpreted as an initial value and used to configure length and datatype @@ -165,7 +178,7 @@ def _waveform(value, fields): # If a value is specified it should be the *only* non keyword argument. value, = value initial_value = device._require_waveform(value, datatype) - length = fields.pop('length', len(initial_value)) + length = _get_length(fields, len(initial_value)) # Special case for [u]int64: if the initial value comes in as 64 bit # integers we cannot represent that, so recast it as [u]int32 @@ -176,7 +189,7 @@ def _waveform(value, fields): initial_value = numpy.require(initial_value, numpy.uint32) else: initial_value = numpy.array([], dtype = datatype) - length = fields.pop('length') + length = _get_length(fields) datatype = initial_value.dtype assert length > 0, 'Array cannot be of zero length' From 5f14ca3efc650875a42362f57134f599cb328555 Mon Sep 17 00:00:00 2001 From: AlexWells Date: Thu, 3 Feb 2022 11:54:06 +0000 Subject: [PATCH 2/4] Add tests for 53f107d, related to issues #46 & #66 --- tests/conftest.py | 8 ++- tests/test_record_values.py | 106 ++++++++++++++++++++++++++++++++---- tests/test_records.py | 34 ++---------- 3 files changed, 106 insertions(+), 42 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 29b1c035..b5220f93 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -15,6 +15,10 @@ sys.platform.startswith("win"), reason="Cothread doesn't work on windows" ) +# Default length used to initialise Waveform and longString records. +# Length picked to match string record length, so we can re-use test strings. +WAVEFORM_LENGTH = 40 + class SubprocessIOC: def __init__(self, ioc_py): self.pv_prefix = "".join( @@ -76,9 +80,9 @@ def _clear_records(): # https://github.com/dls-controls/pythonSoftIOC/issues/56 RecordLookup._RecordDirectory.clear() -@pytest.fixture +@pytest.fixture(autouse=True) def clear_records(): - """Fixture to delete all records before and after a test.""" + """Deletes all records before and after every test""" _clear_records() yield _clear_records() diff --git a/tests/test_record_values.py b/tests/test_record_values.py index e9c6a166..2eb242e1 100644 --- a/tests/test_record_values.py +++ b/tests/test_record_values.py @@ -6,7 +6,7 @@ from enum import Enum from math import isnan, inf, nan -from conftest import requires_cothread +from conftest import requires_cothread, WAVEFORM_LENGTH from softioc import asyncio_dispatcher, builder, softioc from softioc.pythonSoftIoc import RecordWrapper @@ -18,6 +18,9 @@ DEVICE_NAME = "RECORD-VALUE-TESTS" TIMEOUT = 5 # Seconds +# The maximum length string for StringIn/Out records +MAX_LEN_STR = "a 39 char string exactly maximum length" + VERY_LONG_STRING = "This is a fairly long string, the kind that someone " \ "might think to put into a record that can theoretically hold a huge " \ "string and so lets test it and prove that shall we?" @@ -101,6 +104,8 @@ def record_values_names(fixture_value): ("mbbOut_int", builder.mbbOut, 1, 1, int), ("strIn_abc", builder.stringIn, "abc", "abc", str), ("strOut_abc", builder.stringOut, "abc", "abc", str), + ("strIn_39chars", builder.stringIn, MAX_LEN_STR, MAX_LEN_STR, str), + ("strOut_39chars", builder.stringOut, MAX_LEN_STR, MAX_LEN_STR, str), ("strIn_empty", builder.stringIn, "", "", str), ("strOut_empty", builder.stringOut, "", "", str), ("strin_utf8", builder.stringIn, "%a€b", "%a€b", str), # Valid UTF-8 @@ -310,7 +315,7 @@ def run_ioc(record_configurations: list, conn, set_enum, get_enum): if set_enum == SetValueEnum.INITIAL_VALUE: kwarg.update({"initial_value": initial_value}) elif creation_func in [builder.WaveformIn, builder.WaveformOut]: - kwarg = {"length": 50} # Required when no value on creation + kwarg = {"length": WAVEFORM_LENGTH} # Must specify when no value # Related to this issue: # https://github.com/dls-controls/pythonSoftIOC/issues/37 @@ -493,7 +498,7 @@ class TestGetValue: """Tests that use .get() to check whether values applied with .set(), initial_value, or caput return the expected value""" - def test_value_pre_init_set(self, clear_records, record_values): + def test_value_pre_init_set(self, record_values): """Test that records provide the expected values on get calls when using .set() and .get() before IOC initialisation occurs""" @@ -507,7 +512,7 @@ def test_value_pre_init_set(self, clear_records, record_values): kwarg = {} if creation_func in [builder.WaveformIn, builder.WaveformOut]: - kwarg = {"length": 50} # Required when no value on creation + kwarg = {"length": WAVEFORM_LENGTH} # Must specify when no value out_rec = creation_func(record_name, **kwarg) out_rec.set(initial_value) @@ -678,7 +683,7 @@ class TestDefaultValue: ], ) def test_value_default_pre_init( - self, creation_func, expected_value, expected_type, clear_records + self, creation_func, expected_value, expected_type ): """Test that the correct default values are returned from .get() (before record initialisation) when no initial_value or .set() is done""" @@ -686,7 +691,7 @@ def test_value_default_pre_init( kwarg = {} if creation_func in [builder.WaveformIn, builder.WaveformOut]: - kwarg = {"length": 50} # Required when no value on creation + kwarg = {"length": WAVEFORM_LENGTH} # Must specify when no value out_rec = creation_func("out-record", **kwarg) record_value_asserts( @@ -728,7 +733,7 @@ def record_func_reject_none(self, record_func): return record_func def test_value_none_rejected_initial_value( - self, clear_records, record_func_reject_none + self, record_func_reject_none ): """Test setting \"None\" as the initial_value raises an exception""" @@ -737,7 +742,7 @@ def test_value_none_rejected_initial_value( builder.WaveformIn, builder.WaveformOut, ]: - kwarg = {"length": 50} # Required when no value on creation + kwarg = {"length": WAVEFORM_LENGTH} # Must specify when no value with pytest.raises(self.expected_exceptions): record_func_reject_none("SOME-NAME", initial_value=None, **kwarg) @@ -749,7 +754,7 @@ def test_value_none_rejected_set_before_init( kwarg = {} if record_func_reject_none in [builder.WaveformIn, builder.WaveformOut]: - kwarg = {"length": 50} # Required when no value on creation + kwarg = {"length": WAVEFORM_LENGTH} # Must specify when no value with pytest.raises(self.expected_exceptions): record = record_func_reject_none("SOME-NAME", **kwarg) @@ -759,7 +764,7 @@ def none_value_test_func(self, record_func, queue): """Start the IOC and catch the expected exception""" kwarg = {} if record_func in [builder.WaveformIn, builder.WaveformOut]: - kwarg = {"length": 50} # Required when no value on creation + kwarg = {"length": WAVEFORM_LENGTH} # Must specify when no value record = record_func("SOME-NAME", **kwarg) @@ -793,3 +798,84 @@ def test_value_none_rejected_set_after_init(self, record_func_reject_none): finally: process.terminate() process.join(timeout=3) + + +class TestInvalidValues: + """Tests for values that records should reject""" + + def test_string_rejects_overlong_strings(self): + """Test that stringIn & stringOut records reject strings >=39 chars""" + + OVERLONG_STR = MAX_LEN_STR + "A" + + with pytest.raises(ValueError): + builder.stringIn("STRIN1", initial_value=OVERLONG_STR) + + with pytest.raises(ValueError): + builder.stringOut("STROUT1", initial_value=OVERLONG_STR) + + with pytest.raises(ValueError): + si = builder.stringIn("STRIN2") + si.set(OVERLONG_STR) + + with pytest.raises(ValueError): + so = builder.stringOut("STROUT2", initial_value=OVERLONG_STR) + so.set(OVERLONG_STR) + + def test_long_string_rejects_overlong_strings(self): + """Test that longStringIn & longStringOut records reject + strings >=39 chars""" + OVERLONG_STR = MAX_LEN_STR + "A" + + with pytest.raises(AssertionError): + builder.longStringIn( + "LSTRIN1", + initial_value=OVERLONG_STR, + length=WAVEFORM_LENGTH) + + with pytest.raises(AssertionError): + builder.longStringOut( + "LSTROUT1", + initial_value=OVERLONG_STR, + length=WAVEFORM_LENGTH) + + with pytest.raises(AssertionError): + lsi = builder.longStringIn("LSTRIN2", length=WAVEFORM_LENGTH) + lsi.set(OVERLONG_STR) + + with pytest.raises(AssertionError): + lso = builder.longStringIn("LSTROUT2", length=WAVEFORM_LENGTH) + lso.set(OVERLONG_STR) + + # And a different way to initialise the records to trigger same behaviour: + with pytest.raises(AssertionError): + lsi = builder.longStringIn("LSTRIN3", initial_value="ABC") + lsi.set(OVERLONG_STR) + + with pytest.raises(AssertionError): + lso = builder.longStringOut("LSTROUT3", initial_value="ABC") + lso.set(OVERLONG_STR) + + + def test_waveform_rejects_zero_length(self): + """Test that WaveformIn/Out and longStringIn/Out records throw an + exception when being initialized with a zero length array""" + with pytest.raises(AssertionError): + builder.WaveformIn("W_IN", []) + with pytest.raises(AssertionError): + builder.WaveformOut("W_OUT", []) + with pytest.raises(AssertionError): + builder.longStringIn("L_IN", length=0) + with pytest.raises(AssertionError): + builder.longStringOut("L_OUT", length=0) + + def test_waveform_rejects_overlonglong_values(self): + """Test that Waveform records throw an exception when an overlong + value is written""" + w_in = builder.WaveformIn("W_IN", [1, 2, 3]) + w_out = builder.WaveformOut("W_OUT", [1, 2, 3]) + + with pytest.raises(AssertionError): + w_in.set([1, 2, 3, 4]) + with pytest.raises(AssertionError): + w_out.set([1, 2, 3, 4]) diff --git a/tests/test_records.py b/tests/test_records.py index 54c3d532..af39cb49 100644 --- a/tests/test_records.py +++ b/tests/test_records.py @@ -4,7 +4,7 @@ import pytest import asyncio -from conftest import requires_cothread, _clear_records +from conftest import requires_cothread, _clear_records, WAVEFORM_LENGTH from softioc import asyncio_dispatcher, builder, softioc @@ -14,7 +14,7 @@ DEVICE_NAME = "RECORD-TESTS" TIMEOUT = 5 # Seconds -def test_records(tmp_path, clear_records): +def test_records(tmp_path): # Ensure we definitely unload all records that may be hanging over from # previous tests, then create exactly one instance of expected records. from sim_records import create_records @@ -85,32 +85,6 @@ def test_DISP_can_be_overridden(): # Note: DISP attribute won't exist if field not specified assert record.DISP.Value() == 0 -def test_waveform_disallows_zero_length(clear_records): - """Test that WaveformIn/Out records throw an exception when being - initialized with a zero length array""" - with pytest.raises(AssertionError): - builder.WaveformIn("W_IN", []) - with pytest.raises(AssertionError): - builder.WaveformOut("W_OUT", []) - -def test_waveform_disallows_too_long_values(clear_records): - """Test that Waveform and longString records throw an exception when - an overlong value is written""" - w_in = builder.WaveformIn("W_IN", [1, 2, 3]) - w_out = builder.WaveformOut("W_OUT", [1, 2, 3]) - - ls_in = builder.longStringIn("LS_IN", initial_value="ABC") - ls_out = builder.longStringOut("LS_OUT", initial_value="ABC") - - with pytest.raises(AssertionError): - w_in.set([1, 2, 3, 4]) - with pytest.raises(AssertionError): - w_out.set([1, 2, 3, 4]) - with pytest.raises(AssertionError): - ls_in.set("ABCD") - with pytest.raises(AssertionError): - ls_out.set("ABCD") - def validate_fixture_names(params): """Provide nice names for the out_records fixture in TestValidate class""" return params[0].__name__ @@ -149,7 +123,7 @@ def validate_ioc_test_func(self, record_func, queue, validate_pass: bool): kwarg = {} if record_func in [builder.WaveformIn, builder.WaveformOut]: - kwarg = {"length": 50} # Required when no value on creation + kwarg = {"length": WAVEFORM_LENGTH} # Must specify when no value kwarg.update( { @@ -275,7 +249,7 @@ def on_update_func(new_val): kwarg = {} if record_func is builder.WaveformOut: - kwarg = {"length": 50} # Required when no value on creation + kwarg = {"length": WAVEFORM_LENGTH} # Must specify when no value record_func( "ON-UPDATE-RECORD", From 4014707c4f21dd07cb11843e8764505b849a2dd7 Mon Sep 17 00:00:00 2001 From: AlexWells Date: Thu, 3 Feb 2022 16:07:34 +0000 Subject: [PATCH 3/4] Add tests for issue #37 and fix other test fails Other fails appear to be the spawned Validate Process not correctly updating, or possibly not being removed from previous test run, on each iteration. Workaround is to give each one a unique device prefix and add logging to work out what it might be for future issues. --- tests/conftest.py | 8 +++-- tests/test_record_values.py | 6 ++-- tests/test_records.py | 66 ++++++++++++++++++++++++++++++++----- 3 files changed, 65 insertions(+), 15 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index b5220f93..92282756 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -19,11 +19,13 @@ # Length picked to match string record length, so we can re-use test strings. WAVEFORM_LENGTH = 40 +def create_random_prefix(): + """Create 12-character random string, for generating unique Device Names""" + return "".join(random.choice(string.ascii_uppercase) for _ in range(12)) + class SubprocessIOC: def __init__(self, ioc_py): - self.pv_prefix = "".join( - random.choice(string.ascii_uppercase) for _ in range(12) - ) + self.pv_prefix = create_random_prefix() sim_ioc = os.path.join(os.path.dirname(__file__), ioc_py) cmd = [sys.executable, sim_ioc, self.pv_prefix] self.proc = subprocess.Popen( diff --git a/tests/test_record_values.py b/tests/test_record_values.py index 2eb242e1..b7172601 100644 --- a/tests/test_record_values.py +++ b/tests/test_record_values.py @@ -824,7 +824,7 @@ def test_string_rejects_overlong_strings(self): def test_long_string_rejects_overlong_strings(self): """Test that longStringIn & longStringOut records reject - strings >=39 chars""" + strings >=`WAVEFORM_LENGTH` chars""" OVERLONG_STR = MAX_LEN_STR + "A" with pytest.raises(AssertionError): @@ -847,7 +847,7 @@ def test_long_string_rejects_overlong_strings(self): lso = builder.longStringIn("LSTROUT2", length=WAVEFORM_LENGTH) lso.set(OVERLONG_STR) - # And a different way to initialise the records to trigger same behaviour: + # And a different way to initialise records to trigger same behaviour: with pytest.raises(AssertionError): lsi = builder.longStringIn("LSTRIN3", initial_value="ABC") lsi.set(OVERLONG_STR) @@ -869,7 +869,7 @@ def test_waveform_rejects_zero_length(self): with pytest.raises(AssertionError): builder.longStringOut("L_OUT", length=0) - def test_waveform_rejects_overlonglong_values(self): + def test_waveform_rejects_overlong_values(self): """Test that Waveform records throw an exception when an overlong value is written""" w_in = builder.WaveformIn("W_IN", [1, 2, 3]) diff --git a/tests/test_records.py b/tests/test_records.py index af39cb49..2edff309 100644 --- a/tests/test_records.py +++ b/tests/test_records.py @@ -2,9 +2,13 @@ import numpy import os import pytest -import asyncio -from conftest import requires_cothread, _clear_records, WAVEFORM_LENGTH +from conftest import ( + create_random_prefix, + requires_cothread, + _clear_records, + WAVEFORM_LENGTH, +) from softioc import asyncio_dispatcher, builder, softioc @@ -85,6 +89,44 @@ def test_DISP_can_be_overridden(): # Note: DISP attribute won't exist if field not specified assert record.DISP.Value() == 0 + +def test_waveform_construction(): + """Test the various ways to construct a Waveform records all produce + correct results""" + + wi = builder.WaveformIn("WI1", [1, 2, 3]) + assert wi.NELM.Value() == 3 + + wi = builder.WaveformIn("WI2", initial_value=[1, 2, 3, 4]) + assert wi.NELM.Value() == 4 + + wi = builder.WaveformIn("WI3", length=5) + assert wi.NELM.Value() == 5 + + wi = builder.WaveformIn("WI4", NELM=6) + assert wi.NELM.Value() == 6 + + wi = builder.WaveformIn("WI5", [1, 2, 3], length=7) + assert wi.NELM.Value() == 7 + + wi = builder.WaveformIn("WI6", initial_value=[1, 2, 3], length=8) + assert wi.NELM.Value() == 8 + + wi = builder.WaveformIn("WI7", [1, 2, 3], NELM=9) + assert wi.NELM.Value() == 9 + + wi = builder.WaveformIn("WI8", initial_value=[1, 2, 3], NELM=10) + assert wi.NELM.Value() == 10 + + # Specifying neither value nor length should produce an error + with pytest.raises(AssertionError): + builder.WaveformIn("WI9") + + # Specifying both value and initial_value should produce an error + with pytest.raises(AssertionError): + builder.WaveformIn("WI10", [1, 2, 4], initial_value=[5, 6]) + + def validate_fixture_names(params): """Provide nice names for the out_records fixture in TestValidate class""" return params[0].__name__ @@ -238,14 +280,15 @@ def out_records(self, request): """The list of Out records to test """ return request.param - def on_update_test_func(self, record_func, queue, always_update): - + def on_update_test_func( + self, device_name, record_func, queue, always_update + ): def on_update_func(new_val): """Increments li record each time main out record receives caput""" nonlocal li li.set(li.get() + 1) - builder.SetDeviceName(DEVICE_NAME) + builder.SetDeviceName(device_name) kwarg = {} if record_func is builder.WaveformOut: @@ -271,9 +314,11 @@ def on_update_func(new_val): def on_update_runner(self, creation_func, always_update, put_same_value): queue = multiprocessing.Queue() + device_name = create_random_prefix() + process = multiprocessing.Process( target=self.on_update_test_func, - args=(creation_func, queue, always_update), + args=(device_name, creation_func, queue, always_update), ) process.start() @@ -293,17 +338,20 @@ def on_update_runner(self, creation_func, always_update, put_same_value): while count < 4: put_ret = caput( - DEVICE_NAME + ":ON-UPDATE-RECORD", + device_name + ":ON-UPDATE-RECORD", 9 if put_same_value else count, wait=True, ) - assert put_ret.ok, "caput did not succeed" + assert put_ret.ok, f"caput did not succeed: {put_ret.errorcode}" count += 1 ret_val = caget( - DEVICE_NAME + ":ON-UPDATE-COUNTER-RECORD", + device_name + ":ON-UPDATE-COUNTER-RECORD", timeout=3, ) + assert ret_val.ok, \ + f"caget did not succeed: {ret_val.errorcode}, {ret_val}" + # Expected value is either 3 (incremented once per caput) # or 1 (incremented on first caput and not subsequent ones) From 0115f4023e5afe71cd74dc0d700edcee8d2a4f83 Mon Sep 17 00:00:00 2001 From: AlexWells Date: Mon, 7 Feb 2022 09:22:47 +0000 Subject: [PATCH 4/4] Refactor tests to avoid most race conditions These tests have passed 10 consecutive runs on CI with no failures. There were race conditions regarding processing of record callbacks, so we swap to a model of using a "DONE" record that, when processed, signals that the IOC process has finished all of its callbacks. Logging also added to some tests that had weird failures on CI, so if they appear again we'll have more information. --- tests/conftest.py | 21 +++++++ tests/test_record_values.py | 66 +++++++++++---------- tests/test_records.py | 115 +++++++++++++++++++++++++++--------- 3 files changed, 144 insertions(+), 58 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 92282756..16adb94f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -19,6 +19,9 @@ # Length picked to match string record length, so we can re-use test strings. WAVEFORM_LENGTH = 40 +# Default timeout for many operations across testing +TIMEOUT = 10 # Seconds + def create_random_prefix(): """Create 12-character random string, for generating unique Device Names""" return "".join(random.choice(string.ascii_uppercase) for _ in range(12)) @@ -99,3 +102,21 @@ def enable_code_coverage(): pass else: cleanup_on_sigterm() + +def select_and_recv(conn, expected_char = None): + """Wait for the given Connection to have data to receive, and return it. + If a character is provided check its correct before returning it. + This function imports Cothread, and so must NOT be called before any + multiprocessing sub-processes are spawned.""" + from cothread import select + rrdy, _, _ = select([conn], [], [], TIMEOUT) + if rrdy: + val = conn.recv() + else: + pytest.fail("Did not receive expected char before TIMEOUT expired") + + if expected_char: + assert val == expected_char, \ + "Expected character did not match" + + return val diff --git a/tests/test_record_values.py b/tests/test_record_values.py index b7172601..198c0a82 100644 --- a/tests/test_record_values.py +++ b/tests/test_record_values.py @@ -6,7 +6,12 @@ from enum import Enum from math import isnan, inf, nan -from conftest import requires_cothread, WAVEFORM_LENGTH +from conftest import ( + requires_cothread, + WAVEFORM_LENGTH, + select_and_recv, + TIMEOUT +) from softioc import asyncio_dispatcher, builder, softioc from softioc.pythonSoftIoc import RecordWrapper @@ -16,7 +21,6 @@ # Test parameters DEVICE_NAME = "RECORD-VALUE-TESTS" -TIMEOUT = 5 # Seconds # The maximum length string for StringIn/Out records MAX_LEN_STR = "a 39 char string exactly maximum length" @@ -329,7 +333,7 @@ def run_ioc(record_configurations: list, conn, set_enum, get_enum): dispatcher = asyncio_dispatcher.AsyncioDispatcher() builder.LoadDatabase() softioc.iocInit(dispatcher) - conn.send("IOC started") + conn.send("R") # "Ready" # Record list and record config list should always be in line for record, configuration in zip(records, record_configurations): @@ -351,18 +355,19 @@ def run_ioc(record_configurations: list, conn, set_enum, get_enum): if set_enum == SetValueEnum.CAPUT: if conn.poll(TIMEOUT): val = conn.recv() - if val is not None: + if val == "G": conn.send(record.get()) + else: + pytest.fail(f"Received unexpected character {val}") # Keep process alive while main thread works. # This is most applicable to CAGET tests. while (True): if conn.poll(TIMEOUT): val = conn.recv() - if val == "EXIT": + if val == "D": # "Done" break - def run_test_function( record_configurations: list, set_enum: SetValueEnum, get_enum: GetValueEnum ): @@ -379,19 +384,17 @@ def run_test_function( ioc_process.start() + # Wait for message that IOC has started - if parent_conn.poll(TIMEOUT): - parent_conn.recv() - else: - pytest.fail("IOC process did not start before TIMEOUT expired") + select_and_recv(parent_conn, "R") - try: - # Cannot do these imports before the subprocess starts, as cothread - # isn't threadsafe (in the way we require) - from cothread import Yield - from cothread.catools import caget, caput, _channel_cache - from cothread.dbr import DBR_CHAR_STR + # Cannot do these imports before the subprocess starts, as the subprocess + # would inherit cothread's internal state which would break things! + from cothread import Yield + from cothread.catools import caget, caput, _channel_cache + from cothread.dbr import DBR_CHAR_STR + try: # cothread remembers connected IOCs. As we potentially restart the same # named IOC multiple times, we have to purge the cache else the # result from caget/caput cache would be a DisconnectError during the @@ -424,10 +427,7 @@ def run_test_function( if set_enum == SetValueEnum.CAPUT: if get_enum == GetValueEnum.GET: - if parent_conn.poll(TIMEOUT): - parent_conn.recv() - else: - pytest.fail("IOC did not provide initial record value") + select_and_recv(parent_conn) caput( DEVICE_NAME + ":" + record_name, initial_value, @@ -437,7 +437,8 @@ def run_test_function( ) if get_enum == GetValueEnum.GET: - parent_conn.send("Do another get!") + parent_conn.send("G") # "Get" + # Ensure IOC process has time to execute. # I saw failures on MacOS where it appeared the IOC had not # processed the put'ted value as the caget returned the same @@ -445,10 +446,7 @@ def run_test_function( Yield(timeout=TIMEOUT) if get_enum == GetValueEnum.GET: - if parent_conn.poll(TIMEOUT): - rec_val = parent_conn.recv() - else: - pytest.fail("IOC did not provide record value in queue") + rec_val = select_and_recv(parent_conn) else: rec_val = caget( DEVICE_NAME + ":" + record_name, @@ -477,10 +475,8 @@ def run_test_function( # Purge cache to suppress spurious "IOC disconnected" exceptions _channel_cache.purge() + parent_conn.send("D") # "Done" - parent_conn.send("EXIT") - - # ioc_process.terminate() ioc_process.join(timeout=TIMEOUT) if ioc_process.exitcode is None: pytest.fail("Process did not terminate") @@ -771,11 +767,16 @@ def none_value_test_func(self, record_func, queue): builder.LoadDatabase() softioc.iocInit() + print("CHILD: Soft IOC started, about to .set(None)") + try: record.set(None) + print("CHILD: Uh-OH! No exception thrown when setting None!") except Exception as e: queue.put(e) + print("CHILD: We really should never get here...") + queue.put(Exception("FAIL:Test did not raise exception during .set()")) @requires_cothread @@ -791,13 +792,18 @@ def test_value_none_rejected_set_after_init(self, record_func_reject_none): process.start() + print("PARENT: Child process started, waiting for returned exception") + try: - exception = queue.get(timeout=5) + exception = queue.get(timeout=TIMEOUT) assert isinstance(exception, self.expected_exceptions) finally: + print("PARENT: Issuing terminate to child process") process.terminate() - process.join(timeout=3) + process.join(timeout=TIMEOUT) + if process.exitcode is None: + pytest.fail("Process did not terminate") class TestInvalidValues: diff --git a/tests/test_records.py b/tests/test_records.py index 2edff309..be24dd97 100644 --- a/tests/test_records.py +++ b/tests/test_records.py @@ -8,6 +8,8 @@ requires_cothread, _clear_records, WAVEFORM_LENGTH, + TIMEOUT, + select_and_recv ) from softioc import asyncio_dispatcher, builder, softioc @@ -16,7 +18,6 @@ # Test parameters DEVICE_NAME = "RECORD-TESTS" -TIMEOUT = 5 # Seconds def test_records(tmp_path): # Ensure we definitely unload all records that may be hanging over from @@ -158,10 +159,15 @@ def validate_always_fail(self, record, new_val): """Validation method that always rejects changes""" return False - def validate_ioc_test_func(self, record_func, queue, validate_pass: bool): + def validate_ioc_test_func( + self, + device_name, + record_func, + child_conn, + validate_pass: bool): """Start the IOC with the specified validate method""" - builder.SetDeviceName(DEVICE_NAME) + builder.SetDeviceName(device_name) kwarg = {} if record_func in [builder.WaveformIn, builder.WaveformOut]: @@ -181,10 +187,12 @@ def validate_ioc_test_func(self, record_func, queue, validate_pass: bool): builder.LoadDatabase() softioc.iocInit(dispatcher) - queue.put("IOC ready") + child_conn.send("R") # Keep process alive while main thread runs CAGET - queue.get(timeout=5) + if child_conn.poll(TIMEOUT): + val = child_conn.recv() + assert val == "D", "Did not receive expected Done character" def validate_test_runner( self, @@ -193,19 +201,23 @@ def validate_test_runner( expected_value, validate_pass: bool): - queue = multiprocessing.Queue() + parent_conn, child_conn = multiprocessing.Pipe() + + device_name = create_random_prefix() process = multiprocessing.Process( target=self.validate_ioc_test_func, - args=(creation_func, queue, validate_pass), + args=(device_name, creation_func, child_conn, validate_pass), ) process.start() + from cothread.catools import caget, caput, _channel_cache + try: - queue.get(timeout=5) # Get the expected IOC initialised message + # Wait for message that IOC has started + select_and_recv(parent_conn, "R") - from cothread.catools import caget, caput, _channel_cache # Suppress potential spurious warnings _channel_cache.purge() @@ -216,7 +228,7 @@ def validate_test_runner( kwargs.update({"datatype": DBR_CHAR_STR}) put_ret = caput( - DEVICE_NAME + ":VALIDATE-RECORD", + device_name + ":VALIDATE-RECORD", new_value, wait=True, **kwargs, @@ -224,8 +236,8 @@ def validate_test_runner( assert put_ret.ok, "caput did not succeed" ret_val = caget( - DEVICE_NAME + ":VALIDATE-RECORD", - timeout=3, + device_name + ":VALIDATE-RECORD", + timeout=TIMEOUT, **kwargs ) @@ -237,8 +249,8 @@ def validate_test_runner( finally: # Suppress potential spurious warnings _channel_cache.purge() - queue.put("EXIT") - process.join(timeout=3) + parent_conn.send("D") # "Done" + process.join(timeout=TIMEOUT) @requires_cothread @@ -281,15 +293,17 @@ def out_records(self, request): return request.param def on_update_test_func( - self, device_name, record_func, queue, always_update + self, device_name, record_func, conn, always_update ): + + builder.SetDeviceName(device_name) + + li = builder.longIn("ON-UPDATE-COUNTER-RECORD", initial_value=0) + def on_update_func(new_val): """Increments li record each time main out record receives caput""" - nonlocal li li.set(li.get() + 1) - builder.SetDeviceName(device_name) - kwarg = {} if record_func is builder.WaveformOut: kwarg = {"length": WAVEFORM_LENGTH} # Must specify when no value @@ -300,33 +314,49 @@ def on_update_func(new_val): always_update=always_update, **kwarg) - li = builder.longIn("ON-UPDATE-COUNTER-RECORD", initial_value=0) + def on_update_done(_): + conn.send("C") # "Complete" + # Put to the action record after we've done all other Puts, so we know + # all the callbacks have finished processing + builder.Action("ON-UPDATE-DONE", on_update=on_update_done) dispatcher = asyncio_dispatcher.AsyncioDispatcher() builder.LoadDatabase() softioc.iocInit(dispatcher) - queue.put("IOC ready") + conn.send("R") # "Ready" + + print("CHILD: Sent R over Connection to Parent") # Keep process alive while main thread runs CAGET - queue.get(timeout=TIMEOUT) + if conn.poll(TIMEOUT): + val = conn.recv() + assert val == "D", "Did not receive expected Done character" + + print("CHILD: Received exit command, child exiting") def on_update_runner(self, creation_func, always_update, put_same_value): - queue = multiprocessing.Queue() + parent_conn, child_conn = multiprocessing.Pipe() device_name = create_random_prefix() process = multiprocessing.Process( target=self.on_update_test_func, - args=(device_name, creation_func, queue, always_update), + args=(device_name, creation_func, child_conn, always_update), ) process.start() + print("PARENT: Child started, waiting for R command") + + from cothread.catools import caget, caput, _channel_cache + try: - queue.get(timeout=5) # Get the expected IOC initialised message + # Wait for message that IOC has started + select_and_recv(parent_conn, "R") + + print("PARENT: received R command") - from cothread.catools import caget, caput, _channel_cache # Suppress potential spurious warnings _channel_cache.purge() @@ -336,6 +366,8 @@ def on_update_runner(self, creation_func, always_update, put_same_value): # value to force processing to occur count = 1 + print("PARENT: begining While loop") + while count < 4: put_ret = caput( device_name + ":ON-UPDATE-RECORD", @@ -343,15 +375,37 @@ def on_update_runner(self, creation_func, always_update, put_same_value): wait=True, ) assert put_ret.ok, f"caput did not succeed: {put_ret.errorcode}" + + print(f"PARENT: completed caput with count {count}") + count += 1 + print("PARENT: Put'ing to DONE record") + + caput( + device_name + ":ON-UPDATE-DONE", + 1, + wait=True, + ) + + print("PARENT: Waiting for C command") + + # Wait for action record to process, so we know all the callbacks + # have finished processing (This assumes record callbacks are not + # re-ordered, and will run in the same order as the caputs we sent) + select_and_recv(parent_conn, "C") + + print("PARENT: Received C command") + ret_val = caget( device_name + ":ON-UPDATE-COUNTER-RECORD", - timeout=3, + timeout=TIMEOUT, ) assert ret_val.ok, \ f"caget did not succeed: {ret_val.errorcode}, {ret_val}" + print(f"PARENT: Received val from COUNTER: {ret_val}") + # Expected value is either 3 (incremented once per caput) # or 1 (incremented on first caput and not subsequent ones) @@ -364,8 +418,13 @@ def on_update_runner(self, creation_func, always_update, put_same_value): finally: # Suppress potential spurious warnings _channel_cache.purge() - queue.put("EXIT") - process.join(timeout=3) + + print("PARENT:Sending Done command to child") + parent_conn.send("D") # "Done" + process.join(timeout=TIMEOUT) + print(f"PARENT: Join completed with exitcode {process.exitcode}") + if process.exitcode is None: + pytest.fail("Process did not terminate") @requires_cothread def test_on_update_false_false(self, out_records):