Properly test ase not installed error, use single skip mark for module level test skip (#4107)

* use a single skip mark

* use a single skip mark

* properly test no ase installed

* use specific import error

* check None instead of truthy

* use single skip mark

* separate two skip conditions

* use module level skip mark

* more descriptive var name

* try to remove ignore code

* simplify conditional skip for package

* use is not None to instead of Falsy

* enable unit test

* importorskip might be even cleaner

* NEED CONFIRM: redirect emmet graph_hashing import

* correctly test module not available

* fix mock

---------

Signed-off-by: Shyue Ping Ong <[email protected]>
Co-authored-by: Shyue Ping Ong <[email protected]>

Author: DanielYang59

src/pymatgen/io/ase.py

@@ -23,6 +23,7 @@
     from ase.spacegroup import Spacegroup
 
     NO_ASE_ERR = None
+
 except ImportError:
     NO_ASE_ERR = PackageNotFoundError("AseAtomsAdaptor requires the ASE package. Use `pip install ase`")
     encode = decode = FixAtoms = SinglePointDFTCalculator = Spacegroup = None
@@ -94,7 +95,7 @@ def get_atoms(structure: SiteCollection, msonable: bool = True, **kwargs) -> MSO
         Returns:
             Atoms: ASE Atoms object
         """
-        if NO_ASE_ERR:
+        if NO_ASE_ERR is not None:
             raise NO_ASE_ERR
         if not structure.is_ordered:
             raise ValueError("ASE Atoms only supports ordered structures")

tests/electronic_structure/test_boltztrap2.py

@@ -7,6 +7,7 @@
 from monty.serialization import loadfn
 from pytest import approx
 
+from pymatgen.electronic_structure.boltztrap import BoltztrapError
 from pymatgen.electronic_structure.core import OrbitalType, Spin
 from pymatgen.io.vasp import Vasprun
 from pymatgen.util.testing import TEST_FILES_DIR
@@ -21,9 +22,8 @@
         VasprunLoader,
     )
 
-    BOLTZTRAP2_PRESENT = True
-except Exception:
-    BOLTZTRAP2_PRESENT = False
+except BoltztrapError:
+    pytest.skip("No boltztrap2.", allow_module_level=True)
 
 
 TEST_DIR = f"{TEST_FILES_DIR}/electronic_structure/boltztrap2"
@@ -40,7 +40,6 @@
 BZT_TRANSP_FN = f"{TEST_DIR}/bztTranspProps.json.gz"
 
 
-@pytest.mark.skipif(not BOLTZTRAP2_PRESENT, reason="No boltztrap2, skipping tests.")
 class TestVasprunBSLoader(TestCase):
     def setUp(self):
         self.loader = VasprunBSLoader(VASP_RUN)
@@ -80,7 +79,6 @@ def test_get_volume(self):
         assert self.loader.get_volume() == approx(477.6256714925874, abs=1e-5)
 
 
-@pytest.mark.skipif(not BOLTZTRAP2_PRESENT, reason="No boltztrap2, skipping tests.")
 class TestBandstructureLoader(TestCase):
     def setUp(self):
         self.loader = BandstructureLoader(BAND_STRUCT, VASP_RUN.structures[-1])
@@ -108,7 +106,6 @@ def test_set_upper_lower_bands(self):
         assert self.loader_sp_dn.ebands.shape == (14, 198)
 
 
-@pytest.mark.skipif(not BOLTZTRAP2_PRESENT, reason="No boltztrap2, skipping tests.")
 class TestVasprunLoader(TestCase):
     def setUp(self):
         self.loader = VasprunLoader(VASP_RUN)
@@ -128,7 +125,6 @@ def test_from_file(self):
         assert self.loader is not None
 
 
-@pytest.mark.skipif(not BOLTZTRAP2_PRESENT, reason="No boltztrap2, skipping tests.")
 class TestBztInterpolator(TestCase):
     def setUp(self):
         self.loader = VasprunBSLoader(VASP_RUN)
@@ -206,7 +202,6 @@ def test_tot_proj_dos(self):
         assert pdos == approx(272.194174, abs=1e-5)
 
 
-@pytest.mark.skipif(not BOLTZTRAP2_PRESENT, reason="No boltztrap2, skipping tests.")
 class TestBztTransportProperties(TestCase):
     def setUp(self):
         loader = VasprunBSLoader(VASP_RUN)
@@ -307,7 +302,6 @@ def test_compute_properties_doping(self):
             assert self.bztTransp_sp.contain_props_doping
 
 
-@pytest.mark.skipif(not BOLTZTRAP2_PRESENT, reason="No boltztrap2, skipping tests.")
 class TestBztPlotter(TestCase):
     def test_plot(self):
         loader = VasprunBSLoader(VASP_RUN)

tests/ext/test_matproj.py

@@ -32,17 +32,21 @@
     MP_URL = "https://api.materialsproject.org"
 else:
     MP_URL = "https://materialsproject.org"
+
 try:
     skip_mprester_tests = requests.get(MP_URL, timeout=60).status_code != 200
 
 except (ModuleNotFoundError, ImportError, requests.exceptions.ConnectionError):
     # Skip all MPRester tests if some downstream problem on the website, mp-api or whatever.
     skip_mprester_tests = True
 
+if skip_mprester_tests:
+    pytest.skip("MP API is down", allow_module_level=True)
+
 
 @pytest.mark.skipif(
-    skip_mprester_tests or (not 10 < len(PMG_MAPI_KEY) <= 20),
-    reason="Legacy PMG_MAPI_KEY environment variable not set or MP API is down.",
+    not 10 < len(PMG_MAPI_KEY) <= 20,
+    reason="Legacy PMG_MAPI_KEY environment variable not set.",
 )
 class TestMPResterOld(PymatgenTest):
     def setUp(self):
@@ -515,8 +519,8 @@ def test_api_key_is_none(self):
 
 
 @pytest.mark.skipif(
-    skip_mprester_tests or (not len(PMG_MAPI_KEY) > 20),
-    reason="PMG_MAPI_KEY environment variable not set or MP API is down.",
+    not len(PMG_MAPI_KEY) > 20,
+    reason="PMG_MAPI_KEY environment variable not set.",
 )
 class TestMPResterNewBasic(PymatgenTest):
     def setUp(self):

tests/ext/test_optimade.py

@@ -8,9 +8,9 @@
 
 try:
     # 403 is returned when server detects bot-like behavior
-    website_down = requests.get(OptimadeRester.aliases["mp"], timeout=60).status_code not in (200, 403)
+    mp_website_down = requests.get(OptimadeRester.aliases["mp"], timeout=60).status_code not in (200, 403)
 except requests.exceptions.ConnectionError:
-    website_down = True
+    mp_website_down = True
 
 try:
     optimade_providers_down = requests.get("https://providers.optimade.org", timeout=60).status_code not in (200, 403)
@@ -37,7 +37,7 @@
 
 
 class TestOptimade(PymatgenTest):
-    @pytest.mark.skipif(website_down, reason="MP OPTIMADE is down.")
+    @pytest.mark.skipif(mp_website_down, reason="MP OPTIMADE is down.")
     def test_get_structures_mp(self):
         with OptimadeRester("mp") as optimade:
             structs = optimade.get_structures(elements=["Ga", "N"], nelements=2)
@@ -55,7 +55,7 @@ def test_get_structures_mp(self):
                     raw_filter_structs["mp"]
                 ), f"Raw filter {_filter} did not return the same number of results as the query builder."
 
-    @pytest.mark.skipif(website_down, reason="MP OPTIMADE is down.")
+    @pytest.mark.skipif(mp_website_down, reason="MP OPTIMADE is down.")
     def test_get_snls_mp(self):
         base_query = dict(elements=["Ga", "N"], nelements=2, nsites=[2, 6])
         with OptimadeRester("mp") as optimade:

tests/io/pwmat/test_inputs.py

@@ -1,9 +1,13 @@
 from __future__ import annotations
 
+import importlib
+from unittest import mock
+
 import pytest
 from monty.io import zopen
 from numpy.testing import assert_allclose
 
+import pymatgen
 from pymatgen.core import Composition, Structure
 from pymatgen.io.pwmat.inputs import (
     ACExtractor,
@@ -129,10 +133,16 @@ def test_write_file(self):
         assert tmp_high_symmetry_points_str == high_symmetry_points.get_str()
 
 
-def test_err_msg_on_seekpath_not_installed(monkeypatch):
+def test_err_msg_on_seekpath_not_installed():
     """Simulate and test error message when seekpath is not installed."""
-    try:
-        import seekpath  # noqa: F401
-    except ImportError:
+
+    with mock.patch.dict("sys.modules", {"seekpath": None}):
+        # As the import error is raised during init of KPathSeek,
+        # have to import it as well (order matters)
+        importlib.reload(pymatgen.symmetry.kpath)
+        importlib.reload(pymatgen.io.pwmat.inputs)
+
+        from pymatgen.io.pwmat.inputs import GenKpt
+
         with pytest.raises(RuntimeError, match="SeeK-path needs to be installed to use the convention of Hinuma et al"):
             GenKpt.from_structure(Structure.from_file(f"{TEST_DIR}/atom.config"), dim=2, density=0.01)

tests/io/test_ase.py

@@ -1,6 +1,8 @@
 from __future__ import annotations
 
+import importlib
 from importlib.metadata import PackageNotFoundError
+from unittest import mock
 
 import numpy as np
 import pytest
@@ -11,18 +13,12 @@
 from pymatgen.io.ase import AseAtomsAdaptor, MSONAtoms
 from pymatgen.util.testing import TEST_FILES_DIR, VASP_IN_DIR, VASP_OUT_DIR
 
-try:
-    import ase
-except ImportError:
-    ase = None
+ase = pytest.importorskip("ase", reason="ase not installed")
 
 STRUCTURE = Structure.from_file(f"{VASP_IN_DIR}/POSCAR")
 XYZ_STRUCTURE = f"{TEST_FILES_DIR}/io/xyz/acetylene.xyz"
 
-skip_if_no_ase = pytest.mark.skipif(ase is None, reason="ase not installed")
 
-
-@skip_if_no_ase
 def test_get_atoms_from_structure():
     atoms = AseAtomsAdaptor.get_atoms(STRUCTURE)
     ase_composition = Composition(atoms.get_chemical_formula())
@@ -42,7 +38,6 @@ def test_get_atoms_from_structure():
     assert atoms.get_array("prop").tolist() == prop
 
 
-@skip_if_no_ase
 def test_get_atoms_from_structure_mags():
     mags = [1.0] * len(STRUCTURE)
     STRUCTURE.add_site_property("final_magmom", mags)
@@ -64,7 +59,6 @@ def test_get_atoms_from_structure_mags():
     assert atoms.get_magnetic_moments().tolist(), mags
 
 
-@skip_if_no_ase
 def test_get_atoms_from_structure_charge():
     charges = [1.0] * len(STRUCTURE)
     STRUCTURE.add_site_property("final_charge", charges)
@@ -86,22 +80,19 @@ def test_get_atoms_from_structure_charge():
     assert atoms.get_charges().tolist(), charges
 
 
-@skip_if_no_ase
 def test_get_atoms_from_structure_oxi_states():
     oxi_states = [1.0] * len(STRUCTURE)
     STRUCTURE.add_oxidation_state_by_site(oxi_states)
     atoms = AseAtomsAdaptor.get_atoms(STRUCTURE)
     assert atoms.get_array("oxi_states").tolist() == oxi_states
 
 
-@skip_if_no_ase
 def test_get_atoms_from_structure_dyn():
     STRUCTURE.add_site_property("selective_dynamics", [[False] * 3] * len(STRUCTURE))
     atoms = AseAtomsAdaptor.get_atoms(STRUCTURE)
     assert atoms.constraints[0].get_indices().tolist() == [atom.index for atom in atoms]
 
 
-@skip_if_no_ase
 def test_get_atoms_from_molecule():
     mol = Molecule.from_file(XYZ_STRUCTURE)
     atoms = AseAtomsAdaptor.get_atoms(mol)
@@ -113,7 +104,6 @@ def test_get_atoms_from_molecule():
     assert not atoms.has("initial_magmoms")
 
 
-@skip_if_no_ase
 def test_get_atoms_from_molecule_mags():
     molecule = Molecule.from_file(XYZ_STRUCTURE)
     atoms = AseAtomsAdaptor.get_atoms(molecule)
@@ -139,15 +129,13 @@ def test_get_atoms_from_molecule_mags():
     assert atoms.spin_multiplicity == 3
 
 
-@skip_if_no_ase
 def test_get_atoms_from_molecule_dyn():
     molecule = Molecule.from_file(XYZ_STRUCTURE)
     molecule.add_site_property("selective_dynamics", [[False] * 3] * len(molecule))
     atoms = AseAtomsAdaptor.get_atoms(molecule)
     assert atoms.constraints[0].get_indices().tolist() == [atom.index for atom in atoms]
 
 
-@skip_if_no_ase
 def test_get_structure():
     atoms = ase.io.read(f"{VASP_IN_DIR}/POSCAR")
     struct = AseAtomsAdaptor.get_structure(atoms)
@@ -170,7 +158,6 @@ def test_get_structure():
         struct = AseAtomsAdaptor.get_structure(atoms, validate_proximity=True)
 
 
-@skip_if_no_ase
 def test_get_structure_mag():
     atoms = ase.io.read(f"{VASP_IN_DIR}/POSCAR")
     mags = [1.0] * len(atoms)
@@ -187,7 +174,6 @@ def test_get_structure_mag():
     assert "initial_magmoms" not in structure.site_properties
 
 
-@skip_if_no_ase
 @pytest.mark.parametrize(
     "select_dyn",
     [[True, True, True], [False, False, False], np.array([True, True, True]), np.array([False, False, False])],
@@ -212,7 +198,6 @@ def test_get_structure_dyn(select_dyn):
     assert len(ase_atoms) == len(structure)
 
 
-@skip_if_no_ase
 def test_get_molecule():
     atoms = ase.io.read(XYZ_STRUCTURE)
     molecule = AseAtomsAdaptor.get_molecule(atoms)
@@ -240,7 +225,6 @@ def test_get_molecule():
     assert molecule.spin_multiplicity == 3
 
 
-@skip_if_no_ase
 @pytest.mark.parametrize("filename", ["io/vasp/outputs/OUTCAR.gz", "cif/V2O3.cif"])
 def test_back_forth(filename):
     # Atoms --> Structure --> Atoms --> Structure
@@ -259,7 +243,6 @@ def test_back_forth(filename):
         assert str(atoms_back.todict()[key]) == str(val)
 
 
-@skip_if_no_ase
 def test_back_forth_v2():
     # Structure --> Atoms --> Structure --> Atoms
     structure = Structure.from_file(f"{VASP_IN_DIR}/POSCAR")
@@ -281,7 +264,6 @@ def test_back_forth_v2():
     MontyDecoder().process_decoded(dct)
 
 
-@skip_if_no_ase
 def test_back_forth_v3():
     # Atoms --> Molecule --> Atoms --> Molecule
     atoms = ase.io.read(XYZ_STRUCTURE)
@@ -299,7 +281,6 @@ def test_back_forth_v3():
     assert molecule_back == molecule
 
 
-@skip_if_no_ase
 def test_back_forth_v4():
     # Molecule --> Atoms --> Molecule --> Atoms
     molecule = Molecule.from_file(XYZ_STRUCTURE)
@@ -317,7 +298,6 @@ def test_back_forth_v4():
     MontyDecoder().process_decoded(dct)
 
 
-@skip_if_no_ase
 def test_back_forth_v5():
     # Structure --> Atoms --> Structure --> Atoms
     structure = Structure.from_file(f"{VASP_IN_DIR}/POSCAR")
@@ -331,7 +311,6 @@ def test_back_forth_v5():
         assert str(atoms_back.todict()[key]) == str(val)
 
 
-@skip_if_no_ase
 def test_msonable_atoms():
     structure = Structure.from_file(f"{VASP_IN_DIR}/POSCAR")
 
@@ -369,10 +348,12 @@ def test_msonable_atoms():
     assert isinstance(atoms, ase.Atoms)
 
 
-@pytest.mark.skipif(ase is not None, reason="ase is present")
 def test_no_ase_err():
     import pymatgen.io.ase
 
-    expected_msg = str(pymatgen.io.ase.NO_ASE_ERR)
-    with pytest.raises(PackageNotFoundError, match=expected_msg):
-        pymatgen.io.ase.MSONAtoms()
+    with mock.patch.dict("sys.modules", {"ase.atoms": None}):
+        importlib.reload(pymatgen.io.ase)
+        from pymatgen.io.ase import MSONAtoms
+
+        with pytest.raises(PackageNotFoundError, match="AseAtomsAdaptor requires the ASE package."):
+            MSONAtoms()