test: new test suite (pytest-based) by Ken Lauer#59
test: new test suite (pytest-based) by Ken Lauer#59ralphlange merged 9 commits intoepics-extensions:masterfrom
Conversation
ralphlange
force-pushed
the
switch_to_pytest
branch
from
ce7145a to
1d77b06
Compare
ralphlange
force-pushed
the
switch_to_pytest
branch
2 times, most recently
from
c2e3a01 to
0559840
Compare
ralphlange
changed the title
ralphlange
force-pushed
the
switch_to_pytest
branch
from
0559840 to
4f4f1ae
Compare
ralphlange
force-pushed
the
switch_to_pytest
branch
3 times, most recently
from
07f369f to
e4a03a3
Compare
- based on Ken Lauer's PR at https://github.com/klauer/ca-gateway/tree/ref_switch_to_pytest (closes epics-extensions#48) Co-authored-by: Ralph Lange <ralph.lange@gmx.de>
ralphlange
force-pushed
the
switch_to_pytest
branch
2 times, most recently
from
749f723 to
833dd02
Compare
ralphlange
force-pushed
the
switch_to_pytest
branch
from
833dd02 to
fdecd12
Compare
- Update to Ubuntu 24.04 as Access Security tests fail on Ubuntu 22.04 for unknown reasons - Add caPutLog to 3.14 and 3.15 setup - Use caPutLog "master" for 3.15 and 7, R4.0 for 3.14
- let the system choose an arbitrary port
ralphlange
force-pushed
the
switch_to_pytest
branch
from
fdecd12 to
d5a3f98
Compare
|
Ken Lauer's farewell gift to the CA Gateway. Please have a look - the level of Python is way over my head... |
mdavidsaver
left a comment
mdavidsaver
left a comment
There was a problem hiding this comment.
I have read as far as testTop/pyTestsApp/gateway_tests/test_cs_studio.py.
@ralphlange I have tried to note some of the less familiar bits of python. Specifically the use of pytest fixtures. While I can not argue with their expediency, I find this feature to be quite non-obvious.
| textwrap.indent(str(stdout), " "), | ||
| ) | ||
|
|
||
| threading.Thread(daemon=True, target=read_stdout).start() |
There was a problem hiding this comment.
This thread is never joined. Since the function waits on the child, misbehavior on interpreter exit is unlikely. Still, this could instead be:
slirp = threading.Thread(target=read_stdout)
slirp.start()
... yield proc
... proc.wait()
slirp.join()| ioc, mask=dbr.DBE_PROPERTY, use_ctrl=True, callback=on_change_ioc | ||
| ) | ||
|
|
||
| time.sleep(0.1) |
There was a problem hiding this comment.
I'm not sure if these sleeps are actually necessary, so maybe not a big deal. Still, fixed sleeps with no explanation have a certain code smell.
There was a problem hiding this comment.
Agree for this instance.
In general (based on experience with the old Gateway tests), I found such naps necessary because it's a three-body setup (IOC, Gateway, test) and the test can't easily see the connection between Gateway and IOC.
| @pytest.fixture(scope="function") | ||
| def standard_env() -> Generator[EnvironmentInfo, None, None]: |
There was a problem hiding this comment.
I note usage of pytest fixtures as I found these super confusing when I first saw eg.
def test_cs_studio_value_and_prop_monitor(standard_env: conftest.EnvironmentInfo):without understanding where this magical standard_env argument comes from.
| ca.put(ioc_hihi, 123.0, wait=True) | ||
| time.sleep(0.1) | ||
|
|
||
| ca.put(ioc_value, 11.0, wait=True) | ||
| time.sleep(0.1) | ||
|
|
||
| assert events_received_ioc == events_received_gw, ( |
There was a problem hiding this comment.
I think this sort of sequence will be prone to timing related erroneous failures. These sleep(0.1) are effectively settling times. 100ms is probably fine for local testing, but not so for shared GHA runners.
One way to explicitly synchronize would be to use a threading.Condition to wait for an expected end state. Although even then there could be problems if the expected end state is not the actual end state.
Something like:
done = threading.Condition()
def on_change_ioc(pvname=None, **kws):
with done:
... update ioc_struct
done.notify_all()
def on_change_gw(pvname=None, **kws):
with done:
... update gw_start
done.notify_all()
... # setup subscriptions and put
with done:
while gw_struct.get('value')!=11 or ioc_struct.get('value')!=11: # I'm guessing at the key names
assert done.wait(timeout=10.0)
assert not done.wait(timeout=1.0) # ensure no unexpected event
... # full test of result
|
I have a branch which updates https://github.com/mdavidsaver/ca-gateway/tree/refs/heads/pr-59 |
as suggested by M. Davidsaver in epics-extensions#59 (epics-extensions#59 (comment))
ralphlange
force-pushed
the
switch_to_pytest
branch
from
6df44f0 to
1cc11c5
Compare
|
Like this, I assume? Disclaimer: I had Jules (Google's agentic AI) apply the pattern from your example to all tests. |
|
Fails on Base 3.14 - need to investigate. |
Some runners on GitHub Actions seem to be heavily overbooked.
As suggested by @mdavidsaver in his review of PR 59, replace fixed time.sleep() calls with threading.Condition synchronization. This makes the tests more reliable and potentially faster by waiting exactly as long as needed for asynchronous events. - Systematic use of Condition and while loops for synchronization. - Generous 10s timeouts for stability. - Retained or added small fixed sleeps (0.1s - 0.3s) for clean shutdown, especially to prevent race conditions during gateway termination. Co-authored-by: google-labs-jules[bot]
- Replace immediate `return` and `exit(0)` calls with a loop control flag in `gateServer::mainLoop` to allow graceful resource cleanup - Explicitly clean up `gateRateStatsTimer` and `epicsTimerQueueActive` at the end of the server loop - Add a 1-second quiescence period in tests for EPICS 3.14 before terminating to prevent race condition crashes during shutdown - Update `test_cs_studio_value_and_prop_monitor` to skip property update checks if `DBE_PROPERTY` is not supported (e.g., on EPICS 3.14). Co-authored-by: google-labs-jules[bot]
ralphlange
force-pushed
the
switch_to_pytest
branch
from
1cc11c5 to
d338ef8
Compare
|
I think this is in good shape now. Any last words?! |
as suggested by M. Davidsaver in #59 (#59 (comment))
3 participants
(closes #48)