From d82fe383d49e26364535b58136800fa4291d9f9d Mon Sep 17 00:00:00 2001 From: Ganesh Patil <7030871503ganeshpatil@gmail.com> Date: Tue, 24 Feb 2026 17:48:04 +0530 Subject: [PATCH 1/3] fix: replace concorekill.bat single-PID overwrite with safe PID registry (#391) Problem: On Windows, concore.py wrote a single PID to concorekill.bat at import time using 'w' mode. When multiple Python nodes started simultaneously (via makestudy), each overwrote the file. Only the last node's PID survived, making the kill script unable to terminate other nodes. Stale PIDs from crashed studies could also kill unrelated processes. Solution: - Replace single-PID overwrite with append-based PID registry (concorekill_pids.txt) storing one PID per line - Register atexit handler to remove current PID on graceful shutdown - Generate concorekill.bat dynamically with PID validation: each PID is checked via tasklist before taskkill executes - Clean up both files when last node exits - Backward compatible: users still run concorekill.bat Added 9 tests covering registration, cleanup, multi-node scenarios, missing registry handling, and kill script generation. Fixes #391 --- concore.py | 97 ++++++++++++++++++++++++++++-- tests/test_concore.py | 133 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 224 insertions(+), 6 deletions(-) diff --git a/concore.py b/concore.py index 2147e75..a9267d1 100644 --- a/concore.py +++ b/concore.py @@ -21,13 +21,98 @@ logging.getLogger('requests').setLevel(logging.WARNING) -# if windows, create script to kill this process -# because batch files don't provide easy way to know pid of last command -# ignored for posix != windows, because "concorepid" is handled by script -# ignored for docker (linux != windows), because handled by docker stop +# =================================================================== +# Windows PID Registry (Issue #391) +# =================================================================== +# Previous implementation wrote a single PID to concorekill.bat at +# import time, causing race conditions when multiple Python nodes +# started simultaneously (each overwrote the file). Only the last +# node's PID survived, and stale PIDs could kill unrelated processes. +# +# New approach: +# - Append each node's PID to concorekill_pids.txt (one per line) +# - Generate concorekill.bat that validates each PID before killing +# - Use atexit to remove the PID on graceful shutdown +# - Backward compatible: users still run concorekill.bat +# =================================================================== + +_PID_REGISTRY_FILE = "concorekill_pids.txt" +_KILL_SCRIPT_FILE = "concorekill.bat" + + +def _register_pid(): + """Append the current process PID to the registry file.""" + pid = os.getpid() + try: + with open(_PID_REGISTRY_FILE, "a") as f: + f.write(str(pid) + "\n") + except OSError: + pass # Non-fatal: best-effort registration + + +def _cleanup_pid(): + """Remove the current process PID from the registry on exit.""" + pid = str(os.getpid()) + try: + if not os.path.exists(_PID_REGISTRY_FILE): + return + with open(_PID_REGISTRY_FILE, "r") as f: + pids = [line.strip() for line in f if line.strip()] + remaining = [p for p in pids if p != pid] + if remaining: + with open(_PID_REGISTRY_FILE, "w") as f: + for p in remaining: + f.write(p + "\n") + else: + # No PIDs left — clean up both files + try: + os.remove(_PID_REGISTRY_FILE) + except OSError: + pass + try: + os.remove(_KILL_SCRIPT_FILE) + except OSError: + pass + except OSError: + pass # Non-fatal: best-effort cleanup + + +def _write_kill_script(): + """Generate concorekill.bat that validates PIDs before killing. + + The script reads concorekill_pids.txt and for each PID: + 1. Checks if the process still exists + 2. Verifies it is a Python process + 3. Only then issues taskkill + After killing, the registry file is deleted. + """ + try: + script = "@echo off\r\n" + script += "if not exist \"%~dp0" + _PID_REGISTRY_FILE + "\" (\r\n" + script += " echo No PID registry found. Nothing to kill.\r\n" + script += " exit /b 0\r\n" + script += ")\r\n" + script += "for /f \"tokens=*\" %%p in (%~dp0" + _PID_REGISTRY_FILE + ") do (\r\n" + script += " tasklist /FI \"PID eq %%p\" 2>nul | find /i \"python\" >nul\r\n" + script += " if not errorlevel 1 (\r\n" + script += " echo Killing Python process %%p\r\n" + script += " taskkill /F /PID %%p >nul 2>&1\r\n" + script += " ) else (\r\n" + script += " echo Skipping PID %%p - not a Python process or not running\r\n" + script += " )\r\n" + script += ")\r\n" + script += "del /q \"%~dp0" + _PID_REGISTRY_FILE + "\" 2>nul\r\n" + script += "del /q \"%~dp0" + _KILL_SCRIPT_FILE + "\" 2>nul\r\n" + with open(_KILL_SCRIPT_FILE, "w") as f: + f.write(script) + except OSError: + pass # Non-fatal: best-effort script generation + + if hasattr(sys, 'getwindowsversion'): - with open("concorekill.bat","w") as fpid: - fpid.write("taskkill /F /PID "+str(os.getpid())+"\n") + _register_pid() + _write_kill_script() + atexit.register(_cleanup_pid) ZeroMQPort = concore_base.ZeroMQPort convert_numpy_to_python = concore_base.convert_numpy_to_python diff --git a/tests/test_concore.py b/tests/test_concore.py index b1a980e..670d0d8 100644 --- a/tests/test_concore.py +++ b/tests/test_concore.py @@ -1,8 +1,141 @@ import pytest import os +import sys import numpy as np +# =================================================================== +# PID Registry Tests (Issue #391) +# =================================================================== + +class TestPidRegistry: + """Tests for the Windows PID registry mechanism that replaces the + old single-overwrite concorekill.bat approach.""" + + @pytest.fixture(autouse=True) + def use_temp_dir(self, temp_dir, monkeypatch): + """Run each test in an isolated temp directory.""" + self.temp_dir = temp_dir + monkeypatch.chdir(temp_dir) + + def test_register_pid_creates_registry_file(self): + """_register_pid should create concorekill_pids.txt with current PID.""" + from concore import _register_pid, _PID_REGISTRY_FILE + _register_pid() + assert os.path.exists(_PID_REGISTRY_FILE) + with open(_PID_REGISTRY_FILE) as f: + pids = [line.strip() for line in f if line.strip()] + assert str(os.getpid()) in pids + + def test_register_pid_appends_not_overwrites(self): + """Multiple calls to _register_pid should append, not overwrite.""" + from concore import _register_pid, _PID_REGISTRY_FILE + # Simulate two different PIDs by writing manually then registering + with open(_PID_REGISTRY_FILE, "w") as f: + f.write("11111\n") + f.write("22222\n") + _register_pid() + with open(_PID_REGISTRY_FILE) as f: + pids = [line.strip() for line in f if line.strip()] + assert "11111" in pids + assert "22222" in pids + assert str(os.getpid()) in pids + assert len(pids) == 3 + + def test_cleanup_pid_removes_current_pid(self): + """_cleanup_pid should remove only the current PID from the registry.""" + from concore import _cleanup_pid, _PID_REGISTRY_FILE + current_pid = str(os.getpid()) + with open(_PID_REGISTRY_FILE, "w") as f: + f.write("99999\n") + f.write(current_pid + "\n") + f.write("88888\n") + _cleanup_pid() + with open(_PID_REGISTRY_FILE) as f: + pids = [line.strip() for line in f if line.strip()] + assert current_pid not in pids + assert "99999" in pids + assert "88888" in pids + + def test_cleanup_pid_deletes_files_when_last_pid(self): + """When the current PID is the only one left, cleanup should + remove both the registry file and the kill script.""" + from concore import _cleanup_pid, _PID_REGISTRY_FILE, _KILL_SCRIPT_FILE + current_pid = str(os.getpid()) + with open(_PID_REGISTRY_FILE, "w") as f: + f.write(current_pid + "\n") + # Create a dummy kill script to verify it gets cleaned up + with open(_KILL_SCRIPT_FILE, "w") as f: + f.write("@echo off\n") + _cleanup_pid() + assert not os.path.exists(_PID_REGISTRY_FILE) + assert not os.path.exists(_KILL_SCRIPT_FILE) + + def test_cleanup_pid_handles_missing_registry(self): + """_cleanup_pid should not crash when registry file doesn't exist.""" + from concore import _cleanup_pid, _PID_REGISTRY_FILE + assert not os.path.exists(_PID_REGISTRY_FILE) + _cleanup_pid() # Should not raise + + def test_write_kill_script_generates_bat_file(self): + """_write_kill_script should create concorekill.bat with validation logic.""" + from concore import _write_kill_script, _KILL_SCRIPT_FILE, _PID_REGISTRY_FILE + _write_kill_script() + assert os.path.exists(_KILL_SCRIPT_FILE) + with open(_KILL_SCRIPT_FILE) as f: + content = f.read() + # Script should reference the PID registry file + assert _PID_REGISTRY_FILE in content + # Script should validate processes before killing + assert "tasklist" in content + assert "taskkill" in content + assert "python" in content.lower() + + def test_multi_node_registration(self): + """Simulate 3 nodes registering PIDs — all should be present.""" + from concore import _register_pid, _PID_REGISTRY_FILE + fake_pids = ["1204", "1932", "8120"] + with open(_PID_REGISTRY_FILE, "w") as f: + for pid in fake_pids: + f.write(pid + "\n") + _register_pid() # Current process is the 4th + with open(_PID_REGISTRY_FILE) as f: + pids = [line.strip() for line in f if line.strip()] + for pid in fake_pids: + assert pid in pids + assert str(os.getpid()) in pids + assert len(pids) == 4 + + def test_cleanup_preserves_other_pids(self): + """After cleanup, only the current process PID should be removed.""" + from concore import _cleanup_pid, _PID_REGISTRY_FILE + current_pid = str(os.getpid()) + other_pids = ["1111", "2222", "3333"] + with open(_PID_REGISTRY_FILE, "w") as f: + for pid in other_pids: + f.write(pid + "\n") + f.write(current_pid + "\n") + _cleanup_pid() + with open(_PID_REGISTRY_FILE) as f: + pids = [line.strip() for line in f if line.strip()] + assert len(pids) == 3 + assert current_pid not in pids + for pid in other_pids: + assert pid in pids + + @pytest.mark.skipif(not hasattr(sys, 'getwindowsversion'), + reason="Windows-only test") + def test_import_registers_pid_on_windows(self): + """On Windows, importing concore should register the PID.""" + from concore import _PID_REGISTRY_FILE + # The import already happened, so just verify the registry exists + # in our temp dir (we can't easily test the import-time side effect + # since concore was already imported — we test the functions directly) + from concore import _register_pid + _register_pid() + assert os.path.exists(_PID_REGISTRY_FILE) + + class TestSafeLiteralEval: def test_reads_dictionary_from_file(self, temp_dir): test_file = os.path.join(temp_dir, "config.txt") From 7fc7d538f0947c5c38be7471198254d009c33944 Mon Sep 17 00:00:00 2001 From: Ganesh Patil <7030871503ganeshpatil@gmail.com> Date: Mon, 2 Mar 2026 11:42:25 +0530 Subject: [PATCH 2/3] fix: address PR review - add file locking, fix path quoting, fix line endings, format with ruff - Add msvcrt.locking in _cleanup_pid() to prevent race conditions when multiple nodes exit concurrently (Windows file lock) - Use 'usebackq' and quote %~dp0 path in for /f loop so kill script works in directories with spaces/parentheses - Open kill script with newline='' to prevent double-CR (\r\r\n) - Improve test_import_registers_pid_on_windows to actually test import-time side effect by forcing module reimport - Run ruff format on test_concore.py and test_read_status.py --- concore.py | 104 ++++++++++++++++++++++++-------------- tests/test_concore.py | 42 +++++++++++---- tests/test_read_status.py | 46 ++++++++++++----- 3 files changed, 132 insertions(+), 60 deletions(-) diff --git a/concore.py b/concore.py index a9267d1..5b175f8 100644 --- a/concore.py +++ b/concore.py @@ -11,14 +11,14 @@ import concore_base -logger = logging.getLogger('concore') +logger = logging.getLogger("concore") logger.addHandler(logging.NullHandler()) -#these lines mute the noisy library -logging.getLogger('matplotlib').setLevel(logging.WARNING) -logging.getLogger('PIL').setLevel(logging.WARNING) -logging.getLogger('urllib3').setLevel(logging.WARNING) -logging.getLogger('requests').setLevel(logging.WARNING) +# these lines mute the noisy library +logging.getLogger("matplotlib").setLevel(logging.WARNING) +logging.getLogger("PIL").setLevel(logging.WARNING) +logging.getLogger("urllib3").setLevel(logging.WARNING) +logging.getLogger("requests").setLevel(logging.WARNING) # =================================================================== @@ -51,28 +51,40 @@ def _register_pid(): def _cleanup_pid(): - """Remove the current process PID from the registry on exit.""" + """Remove the current process PID from the registry on exit. + + Uses file locking on Windows (msvcrt.locking) to prevent race + conditions when multiple nodes exit concurrently. + """ pid = str(os.getpid()) try: if not os.path.exists(_PID_REGISTRY_FILE): return - with open(_PID_REGISTRY_FILE, "r") as f: + with open(_PID_REGISTRY_FILE, "r+") as f: + # Acquire an exclusive lock on Windows to prevent concurrent + # read-modify-write races between exiting nodes. + if hasattr(sys, "getwindowsversion"): + import msvcrt + + msvcrt.locking(f.fileno(), msvcrt.LK_LOCK, 1) pids = [line.strip() for line in f if line.strip()] - remaining = [p for p in pids if p != pid] - if remaining: - with open(_PID_REGISTRY_FILE, "w") as f: + remaining = [p for p in pids if p != pid] + if remaining: + f.seek(0) + f.truncate() for p in remaining: f.write(p + "\n") - else: - # No PIDs left — clean up both files - try: - os.remove(_PID_REGISTRY_FILE) - except OSError: - pass - try: - os.remove(_KILL_SCRIPT_FILE) - except OSError: - pass + else: + f.close() + # No PIDs left — clean up both files + try: + os.remove(_PID_REGISTRY_FILE) + except OSError: + pass + try: + os.remove(_KILL_SCRIPT_FILE) + except OSError: + pass except OSError: pass # Non-fatal: best-effort cleanup @@ -88,28 +100,34 @@ def _write_kill_script(): """ try: script = "@echo off\r\n" - script += "if not exist \"%~dp0" + _PID_REGISTRY_FILE + "\" (\r\n" + script += 'if not exist "%~dp0' + _PID_REGISTRY_FILE + '" (\r\n' script += " echo No PID registry found. Nothing to kill.\r\n" script += " exit /b 0\r\n" script += ")\r\n" - script += "for /f \"tokens=*\" %%p in (%~dp0" + _PID_REGISTRY_FILE + ") do (\r\n" - script += " tasklist /FI \"PID eq %%p\" 2>nul | find /i \"python\" >nul\r\n" + script += ( + 'for /f "usebackq tokens=*" %%p in ("%~dp0' + + _PID_REGISTRY_FILE + + '") do (\r\n' + ) + script += ' tasklist /FI "PID eq %%p" 2>nul | find /i "python" >nul\r\n' script += " if not errorlevel 1 (\r\n" script += " echo Killing Python process %%p\r\n" script += " taskkill /F /PID %%p >nul 2>&1\r\n" script += " ) else (\r\n" - script += " echo Skipping PID %%p - not a Python process or not running\r\n" + script += ( + " echo Skipping PID %%p - not a Python process or not running\r\n" + ) script += " )\r\n" script += ")\r\n" - script += "del /q \"%~dp0" + _PID_REGISTRY_FILE + "\" 2>nul\r\n" - script += "del /q \"%~dp0" + _KILL_SCRIPT_FILE + "\" 2>nul\r\n" - with open(_KILL_SCRIPT_FILE, "w") as f: + script += 'del /q "%~dp0' + _PID_REGISTRY_FILE + '" 2>nul\r\n' + script += 'del /q "%~dp0' + _KILL_SCRIPT_FILE + '" 2>nul\r\n' + with open(_KILL_SCRIPT_FILE, "w", newline="") as f: f.write(script) except OSError: pass # Non-fatal: best-effort script generation -if hasattr(sys, 'getwindowsversion'): +if hasattr(sys, "getwindowsversion"): _register_pid() _write_kill_script() atexit.register(_cleanup_pid) @@ -125,17 +143,19 @@ def _write_kill_script(): last_read_status = "SUCCESS" -s = '' -olds = '' +s = "" +olds = "" delay = 1 retrycount = 0 -inpath = "./in" #must be rel path for local +inpath = "./in" # must be rel path for local outpath = "./out" simtime = 0 + def _port_path(base, port_num): return base + str(port_num) + concore_params_file = os.path.join(_port_path(inpath, 1), "concore.params") concore_maxtime_file = os.path.join(_port_path(inpath, 1), "concore.maxtime") @@ -145,16 +165,19 @@ def _port_path(base, port_num): _mod = sys.modules[__name__] + # =================================================================== # ZeroMQ Communication Wrapper # =================================================================== def init_zmq_port(port_name, port_type, address, socket_type_str): concore_base.init_zmq_port(_mod, port_name, port_type, address, socket_type_str) + def terminate_zmq(): """Clean up all ZMQ sockets and contexts before exit.""" concore_base.terminate_zmq(_mod) + def signal_handler(sig, frame): """Handle interrupt signals gracefully.""" print(f"\nReceived signal {sig}, shutting down gracefully...") @@ -165,20 +188,23 @@ def signal_handler(sig, frame): concore_base.terminate_zmq(_mod) sys.exit(0) + # Register cleanup handlers atexit.register(terminate_zmq) -signal.signal(signal.SIGINT, signal_handler) # Handle Ctrl+C -if not hasattr(sys, 'getwindowsversion'): +signal.signal(signal.SIGINT, signal_handler) # Handle Ctrl+C +if not hasattr(sys, "getwindowsversion"): signal.signal(signal.SIGTERM, signal_handler) # Handle termination (Unix only) params = concore_base.load_params(concore_params_file) -#9/30/22 + +# 9/30/22 def tryparam(n, i): """Return parameter `n` from params dict, else default `i`.""" return params.get(n, i) -#9/12/21 + +# 9/12/21 # =================================================================== # Simulation Time Handling # =================================================================== @@ -187,12 +213,15 @@ def default_maxtime(default): global maxtime maxtime = safe_literal_eval(concore_maxtime_file, default) + default_maxtime(100) + def unchanged(): """Check if global string `s` is unchanged since last call.""" return concore_base.unchanged(_mod) + # =================================================================== # I/O Handling (File + ZMQ) # =================================================================== @@ -228,5 +257,6 @@ def read(port_identifier, name, initstr_val): def write(port_identifier, name, val, delta=0): concore_base.write(_mod, port_identifier, name, val, delta) -def initval(simtime_val_str): + +def initval(simtime_val_str): return concore_base.initval(_mod, simtime_val_str) diff --git a/tests/test_concore.py b/tests/test_concore.py index 670d0d8..2a85f43 100644 --- a/tests/test_concore.py +++ b/tests/test_concore.py @@ -8,6 +8,7 @@ # PID Registry Tests (Issue #391) # =================================================================== + class TestPidRegistry: """Tests for the Windows PID registry mechanism that replaces the old single-overwrite concorekill.bat approach.""" @@ -21,6 +22,7 @@ def use_temp_dir(self, temp_dir, monkeypatch): def test_register_pid_creates_registry_file(self): """_register_pid should create concorekill_pids.txt with current PID.""" from concore import _register_pid, _PID_REGISTRY_FILE + _register_pid() assert os.path.exists(_PID_REGISTRY_FILE) with open(_PID_REGISTRY_FILE) as f: @@ -30,6 +32,7 @@ def test_register_pid_creates_registry_file(self): def test_register_pid_appends_not_overwrites(self): """Multiple calls to _register_pid should append, not overwrite.""" from concore import _register_pid, _PID_REGISTRY_FILE + # Simulate two different PIDs by writing manually then registering with open(_PID_REGISTRY_FILE, "w") as f: f.write("11111\n") @@ -45,6 +48,7 @@ def test_register_pid_appends_not_overwrites(self): def test_cleanup_pid_removes_current_pid(self): """_cleanup_pid should remove only the current PID from the registry.""" from concore import _cleanup_pid, _PID_REGISTRY_FILE + current_pid = str(os.getpid()) with open(_PID_REGISTRY_FILE, "w") as f: f.write("99999\n") @@ -61,6 +65,7 @@ def test_cleanup_pid_deletes_files_when_last_pid(self): """When the current PID is the only one left, cleanup should remove both the registry file and the kill script.""" from concore import _cleanup_pid, _PID_REGISTRY_FILE, _KILL_SCRIPT_FILE + current_pid = str(os.getpid()) with open(_PID_REGISTRY_FILE, "w") as f: f.write(current_pid + "\n") @@ -74,12 +79,14 @@ def test_cleanup_pid_deletes_files_when_last_pid(self): def test_cleanup_pid_handles_missing_registry(self): """_cleanup_pid should not crash when registry file doesn't exist.""" from concore import _cleanup_pid, _PID_REGISTRY_FILE + assert not os.path.exists(_PID_REGISTRY_FILE) _cleanup_pid() # Should not raise def test_write_kill_script_generates_bat_file(self): """_write_kill_script should create concorekill.bat with validation logic.""" from concore import _write_kill_script, _KILL_SCRIPT_FILE, _PID_REGISTRY_FILE + _write_kill_script() assert os.path.exists(_KILL_SCRIPT_FILE) with open(_KILL_SCRIPT_FILE) as f: @@ -94,6 +101,7 @@ def test_write_kill_script_generates_bat_file(self): def test_multi_node_registration(self): """Simulate 3 nodes registering PIDs — all should be present.""" from concore import _register_pid, _PID_REGISTRY_FILE + fake_pids = ["1204", "1932", "8120"] with open(_PID_REGISTRY_FILE, "w") as f: for pid in fake_pids: @@ -109,6 +117,7 @@ def test_multi_node_registration(self): def test_cleanup_preserves_other_pids(self): """After cleanup, only the current process PID should be removed.""" from concore import _cleanup_pid, _PID_REGISTRY_FILE + current_pid = str(os.getpid()) other_pids = ["1111", "2222", "3333"] with open(_PID_REGISTRY_FILE, "w") as f: @@ -123,17 +132,30 @@ def test_cleanup_preserves_other_pids(self): for pid in other_pids: assert pid in pids - @pytest.mark.skipif(not hasattr(sys, 'getwindowsversion'), - reason="Windows-only test") + @pytest.mark.skipif( + not hasattr(sys, "getwindowsversion"), reason="Windows-only test" + ) def test_import_registers_pid_on_windows(self): - """On Windows, importing concore should register the PID.""" - from concore import _PID_REGISTRY_FILE - # The import already happened, so just verify the registry exists - # in our temp dir (we can't easily test the import-time side effect - # since concore was already imported — we test the functions directly) - from concore import _register_pid - _register_pid() - assert os.path.exists(_PID_REGISTRY_FILE) + """On Windows, importing concore should register the PID. + + We force a fresh import by removing the cached module so that + the module-level registration code runs inside our temp dir. + """ + import importlib + + # Remove cached modules so re-import triggers module-level code + for mod_name in ("concore", "concore_base"): + sys.modules.pop(mod_name, None) + + import concore # noqa: F811 – intentional reimport + + assert os.path.exists(concore._PID_REGISTRY_FILE) + with open(concore._PID_REGISTRY_FILE) as f: + pids = [line.strip() for line in f if line.strip()] + assert str(os.getpid()) in pids + + # Restore module for other tests + importlib.reload(concore) class TestSafeLiteralEval: diff --git a/tests/test_read_status.py b/tests/test_read_status.py index 54dc4b8..df3b7ef 100644 --- a/tests/test_read_status.py +++ b/tests/test_read_status.py @@ -13,6 +13,7 @@ # Helpers # --------------------------------------------------------------------------- + class DummyZMQPort: """Minimal stand-in for ZeroMQPort used in ZMQ read tests.""" @@ -33,14 +34,16 @@ def recv_json_with_retry(self): # File-based read tests # --------------------------------------------------------------------------- + class TestReadFileSuccess: """read() on a valid file returns (data, True) with SUCCESS status.""" @pytest.fixture(autouse=True) def setup(self, temp_dir, monkeypatch): import concore + self.concore = concore - monkeypatch.setattr(concore, 'delay', 0) + monkeypatch.setattr(concore, "delay", 0) # Create ./in1/ym with valid data: [simtime, value] in_dir = os.path.join(temp_dir, "in1") @@ -48,7 +51,7 @@ def setup(self, temp_dir, monkeypatch): with open(os.path.join(in_dir, "ym"), "w") as f: f.write("[10, 3.14]") - monkeypatch.setattr(concore, 'inpath', os.path.join(temp_dir, "in")) + monkeypatch.setattr(concore, "inpath", os.path.join(temp_dir, "in")) def test_returns_data_and_true(self): data, ok = self.concore.read(1, "ym", "[0, 0.0]") @@ -66,10 +69,11 @@ class TestReadFileMissing: @pytest.fixture(autouse=True) def setup(self, temp_dir, monkeypatch): import concore + self.concore = concore - monkeypatch.setattr(concore, 'delay', 0) + monkeypatch.setattr(concore, "delay", 0) # Point to a directory that does NOT have the file - monkeypatch.setattr(concore, 'inpath', os.path.join(temp_dir, "in")) + monkeypatch.setattr(concore, "inpath", os.path.join(temp_dir, "in")) def test_returns_default_and_false(self): data, ok = self.concore.read(1, "nonexistent", "[0, 0.0]") @@ -86,15 +90,16 @@ class TestReadFileParseError: @pytest.fixture(autouse=True) def setup(self, temp_dir, monkeypatch): import concore + self.concore = concore - monkeypatch.setattr(concore, 'delay', 0) + monkeypatch.setattr(concore, "delay", 0) in_dir = os.path.join(temp_dir, "in1") os.makedirs(in_dir, exist_ok=True) with open(os.path.join(in_dir, "ym"), "w") as f: f.write("NOT_VALID_PYTHON{{{") - monkeypatch.setattr(concore, 'inpath', os.path.join(temp_dir, "in")) + monkeypatch.setattr(concore, "inpath", os.path.join(temp_dir, "in")) def test_returns_default_and_false(self): data, ok = self.concore.read(1, "ym", "[0, 0.0]") @@ -111,8 +116,9 @@ class TestReadFileRetriesExceeded: @pytest.fixture(autouse=True) def setup(self, temp_dir, monkeypatch): import concore + self.concore = concore - monkeypatch.setattr(concore, 'delay', 0) + monkeypatch.setattr(concore, "delay", 0) # Create an empty file in_dir = os.path.join(temp_dir, "in1") @@ -120,7 +126,7 @@ def setup(self, temp_dir, monkeypatch): with open(os.path.join(in_dir, "ym"), "w") as f: pass # empty - monkeypatch.setattr(concore, 'inpath', os.path.join(temp_dir, "in")) + monkeypatch.setattr(concore, "inpath", os.path.join(temp_dir, "in")) def test_returns_default_and_false(self): data, ok = self.concore.read(1, "ym", "[0, 0.0]") @@ -135,12 +141,14 @@ def test_last_read_status_is_retries_exceeded(self): # ZMQ read tests # --------------------------------------------------------------------------- + class TestReadZMQSuccess: """Successful ZMQ read returns (data, True).""" @pytest.fixture(autouse=True) def setup(self, monkeypatch): import concore + self.concore = concore self.original_ports = concore.zmq_ports.copy() yield @@ -164,6 +172,7 @@ class TestReadZMQTimeout: @pytest.fixture(autouse=True) def setup(self, monkeypatch): import concore + self.concore = concore self.original_ports = concore.zmq_ports.copy() yield @@ -185,6 +194,7 @@ class TestReadZMQError: @pytest.fixture(autouse=True) def setup(self, monkeypatch): import concore + self.concore = concore self.original_ports = concore.zmq_ports.copy() yield @@ -193,6 +203,7 @@ def setup(self, monkeypatch): def test_zmq_error_returns_default_and_false(self): import zmq + dummy = DummyZMQPort(raise_on_recv=zmq.error.ZMQError("test error")) self.concore.zmq_ports["test_port"] = dummy @@ -205,21 +216,23 @@ def test_zmq_error_returns_default_and_false(self): # Backward compatibility # --------------------------------------------------------------------------- + class TestReadBackwardCompatibility: """Legacy callers can use isinstance check on the result.""" @pytest.fixture(autouse=True) def setup(self, temp_dir, monkeypatch): import concore + self.concore = concore - monkeypatch.setattr(concore, 'delay', 0) + monkeypatch.setattr(concore, "delay", 0) in_dir = os.path.join(temp_dir, "in1") os.makedirs(in_dir, exist_ok=True) with open(os.path.join(in_dir, "ym"), "w") as f: f.write("[10, 42.0]") - monkeypatch.setattr(concore, 'inpath', os.path.join(temp_dir, "in")) + monkeypatch.setattr(concore, "inpath", os.path.join(temp_dir, "in")) def test_legacy_unpack_pattern(self): """The recommended migration pattern works correctly.""" @@ -245,17 +258,24 @@ def test_tuple_unpack(self): # last_read_status exposed on module # --------------------------------------------------------------------------- + class TestLastReadStatusExposed: """concore.last_read_status is publicly accessible.""" def test_attribute_exists(self): import concore - assert hasattr(concore, 'last_read_status') + + assert hasattr(concore, "last_read_status") def test_initial_value_is_success(self): import concore + # Before any read, default is SUCCESS assert concore.last_read_status in ( - "SUCCESS", "FILE_NOT_FOUND", "TIMEOUT", - "PARSE_ERROR", "EMPTY_DATA", "RETRIES_EXCEEDED", + "SUCCESS", + "FILE_NOT_FOUND", + "TIMEOUT", + "PARSE_ERROR", + "EMPTY_DATA", + "RETRIES_EXCEEDED", ) From 130d581d916a14a497ad185ca5f58931a341ba09 Mon Sep 17 00:00:00 2001 From: Ganesh Patil <7030871503ganeshpatil@gmail.com> Date: Mon, 2 Mar 2026 11:45:02 +0530 Subject: [PATCH 3/3] fix: remove unused numpy import and unused variable in test_read_status.py (ruff F401, F841) --- tests/test_read_status.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/test_read_status.py b/tests/test_read_status.py index df3b7ef..8cc2c06 100644 --- a/tests/test_read_status.py +++ b/tests/test_read_status.py @@ -6,7 +6,6 @@ import os import pytest -import numpy as np # --------------------------------------------------------------------------- @@ -123,8 +122,7 @@ def setup(self, temp_dir, monkeypatch): # Create an empty file in_dir = os.path.join(temp_dir, "in1") os.makedirs(in_dir, exist_ok=True) - with open(os.path.join(in_dir, "ym"), "w") as f: - pass # empty + open(os.path.join(in_dir, "ym"), "w").close() # create empty file monkeypatch.setattr(concore, "inpath", os.path.join(temp_dir, "in"))