Merge pull request #22 from niklashoelter/extensive-test-suite-10598339394247217714

Extensive Test Suite Implementation

Author: niklashoelter

tests/test_cli.py

@@ -1,4 +1,5 @@
 from unittest.mock import patch
+import pytest
 
 from gpuma.cli import main
 
@@ -119,3 +120,10 @@ def test_cli_verbose(tmp_path, caplog):
     # Check for debug logs if any (depends on what's logged)
     # The config logic sets logging level to DEBUG
     pass
+
+def test_cli_help(capsys):
+    with patch("sys.argv", ["gpuma", "--help"]):
+        with pytest.raises(SystemExit):
+            main()
+        captured = capsys.readouterr()
+        assert "GPUMA" in captured.out or "usage:" in captured.out

tests/test_config.py

@@ -77,6 +77,24 @@ def test_validate_config():
     with pytest.raises(ValueError, match="Device must be"):
         Config({"optimization": {"device": "invalid_device"}})
 
+def test_validate_config_convergence():
+    # Test invalid convergence criteria
+    with pytest.raises(ValueError, match="force_convergence_criterion must be a positive float"):
+        Config({"optimization": {"force_convergence_criterion": -0.01}})
+
+    with pytest.raises(ValueError, match="force_convergence_criterion must be a positive float"):
+        Config({"optimization": {"force_convergence_criterion": "invalid"}})
+
+    with pytest.raises(ValueError, match="energy_convergence_criterion must be a positive float"):
+        Config({"optimization": {"energy_convergence_criterion": 0.0}})
+
+def test_validate_config_device_empty():
+    with pytest.raises(ValueError, match="Device string in config cannot be empty"):
+        Config({"optimization": {"device": ""}})
+
+    with pytest.raises(ValueError, match="Device string in config cannot be empty"):
+        Config({"optimization": {"device": "   "}})
+
 def test_huggingface_token(tmp_path, monkeypatch):
     # Case 1: Token in config
     cfg = Config({"optimization": {"huggingface_token": "token_in_config"}})

tests/test_io.py

@@ -1,4 +1,5 @@
 import pytest
+from unittest.mock import patch
 
 from gpuma.io_handler import (
     file_exists,
@@ -28,6 +29,25 @@ def test_read_xyz_not_found():
     with pytest.raises(FileNotFoundError):
         read_xyz("non_existent.xyz")
 
+def test_read_xyz_empty_file(tmp_path):
+    f = tmp_path / "empty.xyz"
+    f.touch()
+
+    with pytest.raises(ValueError):
+        read_xyz(str(f))
+
+def test_read_xyz_malformed(tmp_path):
+    f = tmp_path / "malformed.xyz"
+    # First line not integer
+    f.write_text("invalid\ncomment\nC 0 0 0")
+    with pytest.raises(ValueError, match="First line must contain the number of atoms"):
+        read_xyz(str(f))
+
+    # Missing coordinates
+    f.write_text("1\ncomment\nC 0 0")
+    with pytest.raises(ValueError, match="must contain at least 4 elements"):
+        read_xyz(str(f))
+
 def test_read_multi_xyz(tmp_path, sample_multi_xyz_content):
     f = tmp_path / "multi.xyz"
     f.write_text(sample_multi_xyz_content)
@@ -39,6 +59,25 @@ def test_read_multi_xyz(tmp_path, sample_multi_xyz_content):
     assert structs[1].comment == "Methane"
     assert len(structs[1].symbols) == 5
 
+def test_read_multi_xyz_malformed(tmp_path):
+    f = tmp_path / "multi_malformed.xyz"
+    content = """1
+Good
+H 0 0 0
+1
+Bad
+H 0 0
+1
+Good2
+H 1 1 1
+"""
+    f.write_text(content)
+    structs = read_multi_xyz(str(f))
+    # It should skip the middle one
+    assert len(structs) == 2
+    assert structs[0].comment == "Good"
+    assert structs[1].comment == "Good2"
+
 def test_read_xyz_directory(tmp_path, sample_xyz_content):
     d = tmp_path / "xyz_dir"
     d.mkdir()
@@ -58,17 +97,21 @@ def test_save_xyz_file(tmp_path, sample_structure):
     assert "Methane | Energy: -10.500000 eV | Charge: 0 | Multiplicity: 1" in content
     assert "C 0.000000" in content
 
+def test_save_xyz_permission_error(tmp_path, sample_structure):
+    f = tmp_path / "protected.xyz"
+
+    # Mock open to raise PermissionError
+    with patch("builtins.open", side_effect=PermissionError("Denied")):
+        with pytest.raises(PermissionError):
+            save_xyz_file(sample_structure, str(f))
+
 def test_save_multi_xyz(tmp_path, sample_structure):
     f = tmp_path / "output_multi.xyz"
     s1 = sample_structure
     s2 = sample_structure.with_energy(-20.0)
     save_multi_xyz([s1, s2], str(f), comments=["First", "Second"])
 
     content = f.read_text()
-    # Energy might persist on s1 if mutated
-    assert "First | Energy: -20.000000" in content or "First | Energy: -10.500000" not in content
-    # wait, s2 = sample_structure.with_energy(-20.0) mutates sample_structure!
-    # Because s2 is s1.
     assert "Second | Energy: -20.000000" in content
 
 def test_smiles_to_xyz():

tests/test_logging.py

@@ -0,0 +1,38 @@
+import logging
+from unittest.mock import patch
+
+from gpuma.logging_utils import configure_logging
+
+
+def test_configure_logging_root():
+    # Reset root logger handlers
+    logging.getLogger().handlers = []
+
+    configure_logging(level=logging.DEBUG)
+
+    logger = logging.getLogger()
+    assert logger.level == logging.DEBUG
+    assert len(logger.handlers) >= 1
+    assert isinstance(logger.handlers[0], logging.StreamHandler)
+
+def test_configure_logging_named():
+    logger_name = "test_logger"
+    # Ensure fresh start
+    logging.getLogger(logger_name).handlers = []
+
+    configure_logging(level=logging.WARNING, logger_name=logger_name)
+
+    logger = logging.getLogger(logger_name)
+    assert logger.level == logging.WARNING
+    assert len(logger.handlers) >= 1
+
+def test_configure_logging_idempotent():
+    # Test that calling it twice doesn't add multiple handlers if not needed
+    # (The implementation checks `if not logger.handlers`)
+    logging.getLogger().handlers = []
+
+    configure_logging(level=logging.INFO)
+    handlers_count = len(logging.getLogger().handlers)
+
+    configure_logging(level=logging.INFO)
+    assert len(logging.getLogger().handlers) == handlers_count

tests/test_models.py

@@ -1,4 +1,4 @@
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock, patch, ANY
 
 import pytest
 import torch
@@ -37,12 +37,17 @@ def test_device_for_torch():
         assert dev.type == "cuda"
         assert dev.index == 0
 
+    # Test invalid device fallback
+    with patch("torch.cuda.is_available", return_value=False):
+        # Even if we ask for cuda, if not available it might raise or fallback depending on implementation
+        # _device_for_torch calls _parse_device_string which checks availability.
+        # If _parse_device_string returns cpu, _device_for_torch returns cpu device.
+        dev = _device_for_torch("cuda")
+        assert dev.type == "cpu"
+
 def test_load_model_fairchem_logic(mock_hf_token):
     config = Config({"optimization": {"model_name": "test_model"}})
 
-    # We need to patch fairchem.core inside the function or pre-import it
-    # Since it is a local import, patching the module where it lives (fairchem.core) works
-    # if we can import it first.
     try:
         import fairchem.core as _  # noqa: F401
     except ImportError:
@@ -75,3 +80,93 @@ def test_load_model_torchsim_logic(mock_hf_token):
 
         mock_model_cls.assert_called()
         assert model is mock_model_cls.return_value
+
+def test_load_model_fairchem_path(mock_hf_token, tmp_path):
+    model_file = tmp_path / "model.pt"
+    model_file.touch()
+
+    config = Config({"optimization": {"model_path": str(model_file)}})
+
+    try:
+        import fairchem.core as _
+    except ImportError:
+        pytest.skip("fairchem.core not installed")
+
+    with patch("fairchem.core.pretrained_mlip.load_predict_unit") as mock_load_unit, \
+         patch("fairchem.core.FAIRChemCalculator") as mock_calc_cls:
+
+        mock_load_unit.return_value = MagicMock()
+        mock_calc_cls.return_value = MagicMock()
+
+        calc = load_model_fairchem(config)
+
+        mock_load_unit.assert_called_with(path=model_file, device=ANY)
+        mock_calc_cls.assert_called()
+
+def test_load_model_torchsim_path(mock_hf_token, tmp_path):
+    model_file = tmp_path / "model.pt"
+    model_file.touch()
+
+    config = Config({"optimization": {"model_path": str(model_file)}})
+
+    try:
+        import torch_sim.models.fairchem as _
+    except ImportError:
+        pytest.skip("torch_sim not installed")
+
+    with patch("torch_sim.models.fairchem.FairChemModel") as mock_model_cls:
+        mock_model_cls.return_value = MagicMock()
+
+        model = load_model_torchsim(config)
+
+        call_kwargs = mock_model_cls.call_args.kwargs
+        assert call_kwargs["model"] == model_file
+
+def test_model_cache_creation_failure(mock_hf_token, caplog):
+    # Test fails to create cache dir
+    config = Config({"optimization": {"model_cache_dir": "/invalid/path/cache", "model_name": "uma"}})
+
+    try:
+        import fairchem.core as _
+    except ImportError:
+        pytest.skip("fairchem.core not installed")
+
+    with patch("os.makedirs", side_effect=OSError("Permission denied")), \
+         patch("fairchem.core.pretrained_mlip.get_predict_unit") as mock_get_unit, \
+         patch("fairchem.core.FAIRChemCalculator"):
+
+        load_model_fairchem(config)
+
+        # Verify get_predict_unit called without cache_dir
+        call_kwargs = mock_get_unit.call_args.kwargs
+        assert "cache_dir" not in call_kwargs
+
+        # Check that warning was logged
+        assert "Could not create model cache directory" in caplog.text
+
+def test_model_path_not_exists(mock_hf_token):
+    # Path doesn't exist, should fall back to name
+    config = Config({"optimization": {"model_path": "/non/existent/path", "model_name": "fallback"}})
+
+    try:
+        import fairchem.core as _
+    except ImportError:
+        pytest.skip("fairchem.core not installed")
+
+    with patch("fairchem.core.pretrained_mlip.get_predict_unit") as mock_get_unit, \
+         patch("fairchem.core.FAIRChemCalculator"):
+
+         load_model_fairchem(config)
+
+         # Should call get_predict_unit (name based)
+         mock_get_unit.assert_called()
+         args, _ = mock_get_unit.call_args
+         assert args[0] == "fallback"
+
+def test_missing_model_name(mock_hf_token):
+    # Config without model_name (and no valid path) should raise ValueError
+    # Config default has model_name, so we must explicitly set it to empty
+    config = Config({"optimization": {"model_name": ""}})
+
+    with pytest.raises(ValueError, match="Model name must be specified"):
+        load_model_fairchem(config)