changed cmd.split to schlex.split + made pre-commit changes + created…

… conftest.py to store fixtures

jdftxcov.xml

@@ -0,0 +1,64 @@
+<?xml version="1.0" ?>
+<coverage version="7.6.1" timestamp="1728692014899" lines-valid="45" lines-covered="45" line-rate="1" branches-covered="0" branches-valid="0" branch-rate="0" complexity="0">
+	<!-- Generated by coverage.py: https://coverage.readthedocs.io/en/7.6.1 -->
+	<!-- Based on https://raw.githubusercontent.com/cobertura/web/master/htdocs/xml/coverage-04.dtd -->
+	<sources>
+		<source>src/custodian/jdftx</source>
+	</sources>
+	<packages>
+		<package name="." line-rate="1" branch-rate="0" complexity="0">
+			<classes>
+				<class name="jobs.py" filename="jobs.py" complexity="0" line-rate="1" branch-rate="0">
+					<methods/>
+					<lines>
+						<line number="3" hits="1"/>
+						<line number="4" hits="1"/>
+						<line number="5" hits="1"/>
+						<line number="7" hits="1"/>
+						<line number="9" hits="1"/>
+						<line number="11" hits="1"/>
+						<line number="14" hits="1"/>
+						<line number="41" hits="1"/>
+						<line number="44" hits="1"/>
+						<line number="53" hits="1"/>
+						<line number="54" hits="1"/>
+						<line number="59" hits="1"/>
+						<line number="68" hits="1"/>
+						<line number="71" hits="1"/>
+						<line number="74" hits="1"/>
+						<line number="75" hits="1"/>
+						<line number="76" hits="1"/>
+						<line number="77" hits="1"/>
+						<line number="78" hits="1"/>
+						<line number="79" hits="1"/>
+						<line number="80" hits="1"/>
+						<line number="81" hits="1"/>
+						<line number="82" hits="1"/>
+						<line number="83" hits="1"/>
+						<line number="84" hits="1"/>
+						<line number="85" hits="1"/>
+						<line number="86" hits="1"/>
+						<line number="87" hits="1"/>
+						<line number="89" hits="1"/>
+						<line number="92" hits="1"/>
+						<line number="93" hits="1"/>
+						<line number="94" hits="1"/>
+						<line number="95" hits="1"/>
+						<line number="98" hits="1"/>
+						<line number="99" hits="1"/>
+						<line number="100" hits="1"/>
+						<line number="101" hits="1"/>
+						<line number="102" hits="1"/>
+						<line number="103" hits="1"/>
+						<line number="104" hits="1"/>
+						<line number="106" hits="1"/>
+						<line number="107" hits="1"/>
+						<line number="110" hits="1"/>
+						<line number="111" hits="1"/>
+						<line number="112" hits="1"/>
+					</lines>
+				</class>
+			</classes>
+		</package>
+	</packages>
+</coverage>

src/custodian/jdftx/jobs.py

@@ -3,9 +3,9 @@
 import logging
 import os
 import subprocess
+import shlex
 
 import psutil
-
 from custodian.custodian import Job
 
 logger = logging.getLogger(__name__)
@@ -57,7 +57,7 @@ def run(self, directory="./"):
         ):
             # use line buffering for stderr
             return subprocess.run(
-                cmd.split(),
+                shlex.split(),
                 cwd=directory,
                 stdout=f_std,
                 stderr=f_err,
@@ -70,7 +70,6 @@ def postprocess(self, directory="./") -> None:
 
     def terminate(self, directory="./") -> None:
         """Terminate JDFTx."""
-
         work_dir = directory
         logger.info(f"Killing JDFTx processes in {work_dir=}.")
         for proc in psutil.process_iter():
@@ -94,9 +93,9 @@ def terminate(self, directory="./") -> None:
         if "jdftx" in cmd:
             subprocess.run(["killall", f"{cmd}"], check=False)
 
-
     @staticmethod
     def terminate_process(proc, timeout=5):
+        """Terminate a process gracefully, then forcefully if necessary."""
         try:
             proc.terminate()
             try:
@@ -105,10 +104,8 @@ def terminate_process(proc, timeout=5):
                 # If process is still running after the timeout, kill it
                 logger.warning(f"Process {proc.pid} did not terminate gracefully, killing it.")
                 proc.kill()
-                #proc.wait()
+                # proc.wait()
             else:
                 logger.info(f"Process {proc.pid} terminated gracefully.")
         except (psutil.NoSuchProcess, psutil.AccessDenied) as e:
             logger.warning(f"Error while terminating process {proc.pid}: {e}")
-
-

src/custodian/vasp/jobs.py

@@ -249,11 +249,11 @@ def run(self, directory="./"):
                     cmd[-1] += ".gamma"
         logger.info(f"Running {' '.join(cmd)}")
         with (
-            open(os.path.join(directory, self.output_file), "w") as f_std,
-            open(os.path.join(directory, self.stderr_file), "w", buffering=1) as f_err,
+            open(os.path.join(directory, self.output_file), "w"),
+            open(os.path.join(directory, self.stderr_file), "w", buffering=1),
         ):
             # use line buffering for stderrsubprocess.Popen(cmd, cwd=directory, stdout=f_std, stderr=f_err, start_new_session=True)
-            return   # pylint: disable=R1732
+            return  # pylint: disable=R1732
 
     def postprocess(self, directory="./") -> None:
         """

tests/jdftx/conftest.py

@@ -0,0 +1,6 @@
+import pytest
+from custodian.jdftx.jobs import JDFTxJob
+
+@pytest.fixture()
+def jdftx_job():
+    return JDFTxJob(jdftx_cmd="jdftx", output_file="jdftx.out")

tests/jdftx/test_jobs.py

@@ -1,24 +1,29 @@
 import os
-import pytest
 from pathlib import Path
-from unittest.mock import patch, MagicMock, ANY
+from unittest import mock
+from unittest.mock import ANY, MagicMock, patch
+
 import psutil
+import pytest
 from custodian.jdftx.jobs import JDFTxJob
 
 TEST_DIR = Path(__file__).resolve().parent.parent
 TEST_FILES = f"{TEST_DIR}/files/jdftx"
 
-@pytest.fixture
-def jdftx_job():
-    return JDFTxJob(jdftx_cmd="jdftx")
+def create_mock_process(
+    name="jdftx", open_files=None, pid=12345, name_side_effect=None, wait_side_effect=None, terminate_side_effect=None
+):
+    if open_files is None:  # Set a default value if not provided
+        open_files = [MagicMock(path=os.path.join("default_path", "jdftx.out"))]
 
-@pytest.fixture
-def mock_process(tmp_path, jdftx_job):
-    process = MagicMock(spec=psutil.Process)
-    process.name.return_value = "jdftx"
-    process.pid = 12345
-    process.open_files.return_value = [MagicMock(path=os.path.join(str(tmp_path), jdftx_job.output_file))]
-    return process
+    mock_process = mock.Mock(spec=psutil.Process)
+    mock_process.name.return_value = name
+    mock_process.open_files.return_value = open_files
+    mock_process.pid = pid
+    mock_process.name.side_effect = name_side_effect
+    mock_process.wait.side_effect = wait_side_effect
+    mock_process.terminate.side_effect = terminate_side_effect
+    return mock_process
 
 
 def test_jdftx_job_init(jdftx_job):
@@ -27,10 +32,12 @@ def test_jdftx_job_init(jdftx_job):
     assert jdftx_job.output_file == "jdftx.out"
     assert jdftx_job.stderr_file == "std_err.txt"
 
+
 def test_jdftx_job_setup(jdftx_job, tmp_path):
     jdftx_job.setup(str(tmp_path))
     # Setup method doesn't do anything, so just checking that it doesn't raise an exception
 
+
 def test_jdftx_job_run(jdftx_job, tmp_path):
     with patch("subprocess.run") as mock_run:
         mock_process = MagicMock()
@@ -48,97 +55,119 @@ def test_jdftx_job_run(jdftx_job, tmp_path):
             check=False,
         )
 
-def test_jdftx_job_run_creates_output_files(jdftx_job, temp_dir):
+
+def test_jdftx_job_run_creates_output_files(jdftx_job, tmp_path):
     with patch("subprocess.run"):
-        jdftx_job.run(str(temp_dir))
+        jdftx_job.run(str(tmp_path))
+
+    assert os.path.exists(os.path.join(str(tmp_path), "jdftx.out"))
+    assert os.path.exists(os.path.join(str(tmp_path), "std_err.txt"))
 
-    assert os.path.exists(os.path.join(str(temp_dir), "jdftx.out"))
-    assert os.path.exists(os.path.join(str(temp_dir), "std_err.txt"))
 
 def test_jdftx_job_postprocess(jdftx_job, tmp_path):
     jdftx_job.postprocess(str(tmp_path))
     # Postprocess method doesn't do anything, so we just check that it doesn't raise an exception
 
-@patch("psutil.process_iter")
-@patch("psutil.pid_exists")
-@patch("subprocess.run")
-@patch.object(JDFTxJob, 'terminate_process', autospec=True)
-def test_jdftx_job_terminate(mock_terminate_process, mock_subprocess_run, mock_pid_exists, mock_process_iter, jdftx_job, mock_process, tmp_path):
-    jdftx_job.output_file = "jdftx.out"  # Ensure this matches the actual output file name
-    run_path = os.path.join(str(tmp_path), jdftx_job.output_file)
-    mock_process.open_files.return_value = [MagicMock(path=run_path)]
-    mock_process.name.return_value = "jdftx"
-    mock_process_iter.return_value = [mock_process]
 
-    # Test successful termination
-    mock_pid_exists.return_value = True
-    jdftx_job.terminate(str(tmp_path))
-    mock_terminate_process.assert_called_once_with(mock_process)
-    mock_subprocess_run.assert_not_called()
+@mock.patch("psutil.pid_exists")
+@mock.patch("subprocess.run")
+@mock.patch.object(JDFTxJob, "terminate_process", autospec=True)
+def test_jdftx_job_terminate(mock_terminate_process, mock_subprocess_run, mock_pid_exists, jdftx_job, tmp_path, caplog):
+    open_files = [MagicMock(path=os.path.join(str(tmp_path), jdftx_job.output_file))]
+    # Test when JDFTx process exists
+    mock_process = create_mock_process(name="jdftx", open_files=open_files, pid=12345)
+
+    with patch("psutil.process_iter", return_value=[mock_process]):
+        mock_pid_exists.return_value = True
+        jdftx_job.terminate(str(tmp_path))
+        mock_terminate_process.assert_called_once_with(mock_process)
+        mock_subprocess_run.assert_not_called()
 
     mock_terminate_process.reset_mock()
     mock_subprocess_run.reset_mock()
 
-    # Test when process doesn't exist
-    mock_process.name.return_value = "not_jdft"
-    jdftx_job.terminate(str(tmp_path))
-    mock_terminate_process.assert_not_called()
-    mock_subprocess_run.assert_called_once_with(["killall", "jdftx"], check=False)
+    # Test when no JDFTx process exists
+    mock_process = create_mock_process(name="vasp", open_files=open_files, pid=12345)
+
+    with patch("psutil.process_iter", return_value=[mock_process]):
+        jdftx_job.terminate(str(tmp_path))
+        mock_terminate_process.assert_not_called()
+        mock_subprocess_run.assert_called_once_with(["killall", "jdftx"], check=False)
 
     mock_terminate_process.reset_mock()
     mock_subprocess_run.reset_mock()
 
-    # Test when psutil raises exceptions
-    mock_pid_exists.return_value = "jdftx"
-    mock_process_iter.side_effect = psutil.NoSuchProcess(pid=12345)
-    jdftx_job.terminate(str(tmp_path))
-    mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False)
+    # Test when psutil.process_iter raises NoSuchProcess
+    mock_process = create_mock_process(
+        name="jdftx", open_files=open_files, pid=12345, name_side_effect=psutil.NoSuchProcess(pid=12345)
+    )
+
+    with caplog.at_level("WARNING"):
+        with patch("psutil.process_iter", return_value=[mock_process]):
+            jdftx_job.terminate(str(tmp_path))
+            mock_terminate_process.assert_not_called()
+            mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False)
+
+        assert "Exception" in caplog.text
+        assert "encountered while killing JDFTx" in caplog.text
 
     mock_terminate_process.reset_mock()
     mock_subprocess_run.reset_mock()
-    mock_pid_exists.side_effect = None
-    mock_process_iter.side_effect = None
-
-    #mock_pid_exists.return_value = True
-    mock_process_iter.side_effect = psutil.AccessDenied(pid=12345)
-    jdftx_job.terminate(str(tmp_path))
-    mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False)
-
-@patch("psutil.Process")
-def test_terminate_process(mock_process, jdftx_job):
-    process = mock_process.return_value
-
+
+    # Test when psutil.process_iter raises AccessDenied
+    with caplog.at_level("WARNING"):
+        mock_process = create_mock_process(
+            name="jdftx", open_files=open_files, pid=12345, name_side_effect=psutil.AccessDenied(pid=12345)
+        )
+        with patch("psutil.process_iter", return_value=[mock_process]):
+            jdftx_job.terminate(str(tmp_path))
+            mock_terminate_process.assert_not_called()
+            mock_subprocess_run.assert_called_with(["killall", "jdftx"], check=False)
+
+        assert "Exception" in caplog.text
+        assert "encountered while killing JDFTx" in caplog.text
+
+
+def test_terminate_process(jdftx_job, caplog):
     # Test successful termination
-    jdftx_job.terminate_process(process)
-    process.terminate.assert_called_once()
-    process.wait.assert_called_once_with(timeout=5)
+    mock_process = create_mock_process()
+    mock_process.terminate.return_value = None  # Simulate successful termination
+    mock_process.wait.return_value = None  # Simulate process finished immediately
+    with caplog.at_level("INFO"):
+        jdftx_job.terminate_process(mock_process)
+
+    mock_process.terminate.assert_called_once()
+    mock_process.wait.assert_called_once()
+
+    assert "Process" in caplog.text
+    assert "terminated gracefully" in caplog.text
 
-    process.reset_mock()
+    mock_process.reset_mock()
 
     # Test when process doesn't terminate gracefully
-    process.wait.side_effect = [psutil.TimeoutExpired(5), None]
-    jdftx_job.terminate_process(process)
-    process.kill.assert_called_once()
+    mock_process = create_mock_process(pid=12345, wait_side_effect=psutil.TimeoutExpired(seconds=5))
+    mock_process.terminate.return_value = None
+
+    jdftx_job.terminate_process(mock_process)
+    mock_process.terminate.assert_called_once()
+    mock_process.kill.assert_called_once()
+    mock_process.wait.assert_called()
+
+    mock_process.reset_mock()
 
     # Test when process raises NoSuchProcess
-    process.terminate.side_effect = psutil.NoSuchProcess(pid=12345)
-    jdftx_job.terminate_process(process)
+    mock_process = create_mock_process(pid=12345, terminate_side_effect=psutil.NoSuchProcess(pid=12345))
+    with caplog.at_level("WARNING"):
+        jdftx_job.terminate_process(mock_process)
+
+    assert "Error while terminating process" in caplog.text
 
-    process.reset_mock()
+    mock_process.reset_mock()
 
     # Test when process raises AccessDenied
-    process.terminate.side_effect = psutil.AccessDenied(pid=12345)
-    jdftx_job.terminate_process(process)
-
-def test_jdftx_job_with_custom_parameters():
-    custom_job = JDFTxJob(
-        jdftx_cmd="custom_jdftx",
-        input_file="custom.in",
-        output_file="custom.out",
-        stderr_file="custom_err.txt"
-    )
-
-    assert custom_job.jdftx_cmd == "custom_jdftx"
-    assert custom_job.input_file == "custom.in"
-    assert custom_job.output_file == "custom.out"
-    assert custom_job.stderr_file == "custom_err.txt"
+    mock_process = create_mock_process(pid=12345, terminate_side_effect=psutil.AccessDenied(pid=12345))
+
+    with caplog.at_level("WARNING"):
+        jdftx_job.terminate_process(mock_process)
+
+    assert "Error while terminating process" in caplog.text