From 229872cbd225bc43494e6de4fc1f9f550817df0a Mon Sep 17 00:00:00 2001 From: Ludovico Bianchi Date: Tue, 10 Sep 2024 20:04:01 -0500 Subject: [PATCH 1/9] Add pytest plugin to verify that test modules clean up any generated file --- idaes/conftest.py | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/idaes/conftest.py b/idaes/conftest.py index fa8724eb04..8e4983d333 100644 --- a/idaes/conftest.py +++ b/idaes/conftest.py @@ -16,7 +16,9 @@ import importlib.abc import importlib.machinery +import subprocess import sys +from pathlib import Path from typing import Dict from typing import Iterable from typing import List @@ -240,6 +242,47 @@ def pytest_report_collectionfinish(self) -> List[str]: return lines +class VerifyCleanup: + def __init__(self): + self._added_by_mod = {} + + def _get_files(self): + try: + text = subprocess.check_output([ + "git", "ls-files", "--others", "--exclude-standard", + ], text=True).strip() + except subprocess.CalledProcessError as e: + text = str(e) + return text.splitlines() + + @pytest.fixture(scope="module", autouse=True) + def _inspect_generated_files(self, request): + mod = request.module.__name__ + before = set(self._get_files()) + yield + after = set(self._get_files()) + added = after - before + if added: + self._added_by_mod[mod] = sorted(added) + + @pytest.hookimpl(trylast=True) + def pytest_terminal_summary(self, terminalreporter): + tr = terminalreporter + tr.section("Files added (and not cleaned up) by test modules") + for mod, files in self._added_by_mod.items(): + tr.write_line(mod) + for file in files: + tr.write_line(f"\t{file}") + if self._added_by_mod: + tr.write_line(f"{len(self._added_by_mod)} test modules did not clean up after themselves") + tr.write_line("The exit status of the test session will be set to failed") + + @pytest.hookimpl(trylast=True) + def pytest_sessionfinish(self, session, exitstatus): + if self._added_by_mod: + session.exitstatus = pytest.ExitCode.TESTS_FAILED + + def pytest_addhooks(pluginmanager: pytest.PytestPluginManager): skipper_plugin = Importorskipper( { @@ -248,6 +291,7 @@ def pytest_addhooks(pluginmanager: pytest.PytestPluginManager): ) pluginmanager.register(skipper_plugin, name="importorskipper") + pluginmanager.register(VerifyCleanup()) #### From 3122eee9711f778f8679acb6582bb075d5470505 Mon Sep 17 00:00:00 2001 From: Ludovico Bianchi Date: Tue, 10 Sep 2024 20:05:05 -0500 Subject: [PATCH 2/9] Format with Black and remove unused import --- idaes/conftest.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/idaes/conftest.py b/idaes/conftest.py index 8e4983d333..fd3b370918 100644 --- a/idaes/conftest.py +++ b/idaes/conftest.py @@ -18,7 +18,6 @@ import importlib.machinery import subprocess import sys -from pathlib import Path from typing import Dict from typing import Iterable from typing import List @@ -248,9 +247,15 @@ def __init__(self): def _get_files(self): try: - text = subprocess.check_output([ - "git", "ls-files", "--others", "--exclude-standard", - ], text=True).strip() + text = subprocess.check_output( + [ + "git", + "ls-files", + "--others", + "--exclude-standard", + ], + text=True, + ).strip() except subprocess.CalledProcessError as e: text = str(e) return text.splitlines() @@ -274,7 +279,9 @@ def pytest_terminal_summary(self, terminalreporter): for file in files: tr.write_line(f"\t{file}") if self._added_by_mod: - tr.write_line(f"{len(self._added_by_mod)} test modules did not clean up after themselves") + tr.write_line( + f"{len(self._added_by_mod)} test modules did not clean up after themselves" + ) tr.write_line("The exit status of the test session will be set to failed") @pytest.hookimpl(trylast=True) From 779ac789097ce16985946b520e7929f509b7bfb0 Mon Sep 17 00:00:00 2001 From: Ludovico Bianchi Date: Tue, 10 Sep 2024 22:19:34 -0500 Subject: [PATCH 3/9] Add docstring to make Pylint happy --- idaes/conftest.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/idaes/conftest.py b/idaes/conftest.py index fd3b370918..50b7f0fde0 100644 --- a/idaes/conftest.py +++ b/idaes/conftest.py @@ -242,6 +242,9 @@ def pytest_report_collectionfinish(self) -> List[str]: class VerifyCleanup: + """ + A pytest plugin to verify that files generated by tests are cleaned up properly. + """ def __init__(self): self._added_by_mod = {} From 4c486a98b0a1654e672c537208f9c2ae013ed5db Mon Sep 17 00:00:00 2001 From: Ludovico Bianchi Date: Tue, 10 Sep 2024 22:52:21 -0500 Subject: [PATCH 4/9] Format with Black --- idaes/conftest.py | 1 + 1 file changed, 1 insertion(+) diff --git a/idaes/conftest.py b/idaes/conftest.py index 50b7f0fde0..a775de1c74 100644 --- a/idaes/conftest.py +++ b/idaes/conftest.py @@ -245,6 +245,7 @@ class VerifyCleanup: """ A pytest plugin to verify that files generated by tests are cleaned up properly. """ + def __init__(self): self._added_by_mod = {} From 384585b13314b72544ef004d1940263b932c96a0 Mon Sep 17 00:00:00 2001 From: Ludovico Bianchi Date: Wed, 18 Sep 2024 17:11:46 -0400 Subject: [PATCH 5/9] Switch to per-test file checks and Python 3.9+ type hinting --- idaes/conftest.py | 112 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 83 insertions(+), 29 deletions(-) diff --git a/idaes/conftest.py b/idaes/conftest.py index a775de1c74..871d25a3fa 100644 --- a/idaes/conftest.py +++ b/idaes/conftest.py @@ -16,11 +16,12 @@ import importlib.abc import importlib.machinery +import os import subprocess import sys -from typing import Dict -from typing import Iterable -from typing import List +from collections.abc import Iterable +from pathlib import Path +from typing import Union import pytest @@ -196,7 +197,7 @@ class ImportorskipFinder(importlib.abc.MetaPathFinder): a custom Loader to be used for registered modules. """ - def __init__(self, registry: Dict[ModuleName, List[ModuleName]]): + def __init__(self, registry: dict[ModuleName, list[ModuleName]]): self._registry = registry def find_spec(self, *args, **kwargs): @@ -218,7 +219,7 @@ class Importorskipper: instead of having to call importorskip() in each test module for each possibly missing module. """ - def __init__(self, registry: Dict[ModuleName, List[ModuleName]]): + def __init__(self, registry: dict[ModuleName, list[ModuleName]]): self._registry = dict(registry) self._finder = ImportorskipFinder(self._registry) @@ -228,7 +229,7 @@ def pytest_configure(self): def pytest_sessionfinish(self): sys.meta_path.remove(self._finder) - def pytest_report_collectionfinish(self) -> List[str]: + def pytest_report_collectionfinish(self) -> list[str]: preamble = [ "The following modules are registered in the importorskipper plugin", " and will cause tests to be skipped if any of the registered modules is not found: ", @@ -241,56 +242,107 @@ def pytest_report_collectionfinish(self) -> List[str]: return lines +@pytest.fixture(scope="function") +def run_in_tmp_path(tmp_path: Path): + prev_workdir = os.getcwd() + try: + os.chdir(tmp_path) + yield + finally: + os.chdir(prev_workdir) + + +@pytest.fixture(scope="class") +def run_class_in_tmp_path( + request: pytest.FixtureRequest, tmp_path_factory: pytest.TempPathFactory +): + prev_workdir = os.getcwd() + + modname = request.module.__name__ + clsname = request.__class__.__name__ + dirname = f"{modname}-{clsname}" + + tmp_path = tmp_path_factory.mktemp(dirname, numbered=True) + try: + os.chdir(tmp_path) + yield + finally: + os.chdir(prev_workdir) + + +def _get_repo_root_dir() -> Union[Path, None]: + try: + text = subprocess.check_output( + ["git", "rev-parse", "--show-toplevel"], + text=True, + ).strip() + except subprocess.CalledProcessError: + # no Git directory found + return None + return Path(text).resolve() + + class VerifyCleanup: """ A pytest plugin to verify that files generated by tests are cleaned up properly. """ - def __init__(self): - self._added_by_mod = {} - - def _get_files(self): + def __init__(self, repo_root_dir: Path): + self._repo_root_dir = Path(repo_root_dir).resolve() + self._added_by_test = {} + + def _get_files(self) -> list[str]: + args = [ + "git", + "-C", + self._repo_root_dir, + "ls-files", + "--others", + "--exclude-standard", + ] try: text = subprocess.check_output( - [ - "git", - "ls-files", - "--others", - "--exclude-standard", - ], + args, text=True, ).strip() except subprocess.CalledProcessError as e: text = str(e) return text.splitlines() - @pytest.fixture(scope="module", autouse=True) - def _inspect_generated_files(self, request): - mod = request.module.__name__ + def pytest_report_collectionfinish(self) -> list[str]: + return [ + f"Looking for untracked files being created by tests for repo: {self._repo_root_dir}" + ] + + @pytest.hookimpl(hookwrapper=True) + def pytest_runtest_protocol(self, item): before = set(self._get_files()) yield after = set(self._get_files()) added = after - before if added: - self._added_by_mod[mod] = sorted(added) + key = item.nodeid + self._added_by_test[key] = sorted(added) @pytest.hookimpl(trylast=True) def pytest_terminal_summary(self, terminalreporter): + to_report = dict(self._added_by_test) + if not to_report: + return + tr = terminalreporter - tr.section("Files added (and not cleaned up) by test modules") - for mod, files in self._added_by_mod.items(): - tr.write_line(mod) + tr.section("Files added (and not cleaned up) by tests") + for item, files in to_report.items(): + tr.write_line(item) for file in files: tr.write_line(f"\t{file}") - if self._added_by_mod: - tr.write_line( - f"{len(self._added_by_mod)} test modules did not clean up after themselves" - ) + if to_report: + tr.write_line(f"{len(to_report)} test(s) did not clean up after themselves") tr.write_line("The exit status of the test session will be set to failed") @pytest.hookimpl(trylast=True) def pytest_sessionfinish(self, session, exitstatus): - if self._added_by_mod: + if self._added_by_test: session.exitstatus = pytest.ExitCode.TESTS_FAILED @@ -302,7 +354,9 @@ def pytest_addhooks(pluginmanager: pytest.PytestPluginManager): ) pluginmanager.register(skipper_plugin, name="importorskipper") - pluginmanager.register(VerifyCleanup()) + repo_root_dir = _get_repo_root_dir() + if repo_root_dir is not None: + pluginmanager.register(VerifyCleanup(repo_root_dir)) #### From 9933e0fe3b6c176a900ee3df9fff410b358bd12d Mon Sep 17 00:00:00 2001 From: Ludovico Bianchi Date: Wed, 18 Sep 2024 17:13:13 -0400 Subject: [PATCH 6/9] Apply fixtures to offending tests --- idaes/core/solvers/tests/test_solvers.py | 1 + idaes/core/surrogate/pysmo/tests/test_kriging.py | 1 + idaes/core/surrogate/pysmo/tests/test_polynomial_regression.py | 1 + idaes/core/surrogate/pysmo/tests/test_radial_basis_function.py | 1 + 4 files changed, 4 insertions(+) diff --git a/idaes/core/solvers/tests/test_solvers.py b/idaes/core/solvers/tests/test_solvers.py index 15d43c77cf..49c100fb76 100644 --- a/idaes/core/solvers/tests/test_solvers.py +++ b/idaes/core/solvers/tests/test_solvers.py @@ -105,6 +105,7 @@ def test_ipopt_idaes_solve(): assert pytest.approx(x) == pyo.value(m.x) +@pytest.mark.usefixtures("run_in_tmp_path") @pytest.mark.unit @pytest.mark.skipif( not pyo.SolverFactory("ipopt_l1").available(False), reason="solver not available" diff --git a/idaes/core/surrogate/pysmo/tests/test_kriging.py b/idaes/core/surrogate/pysmo/tests/test_kriging.py index c688383683..0f398b833c 100644 --- a/idaes/core/surrogate/pysmo/tests/test_kriging.py +++ b/idaes/core/surrogate/pysmo/tests/test_kriging.py @@ -22,6 +22,7 @@ import pytest +@pytest.mark.usefixtures("run_class_in_tmp_path") class TestKrigingModel: y = np.array( [ diff --git a/idaes/core/surrogate/pysmo/tests/test_polynomial_regression.py b/idaes/core/surrogate/pysmo/tests/test_polynomial_regression.py index b5300c0ee3..55cb55019d 100644 --- a/idaes/core/surrogate/pysmo/tests/test_polynomial_regression.py +++ b/idaes/core/surrogate/pysmo/tests/test_polynomial_regression.py @@ -153,6 +153,7 @@ def test_data_unscaling_06(self, array_type): FeatureScaling.data_unscaling(output_1, min_array, max_array) +@pytest.mark.usefixtures("run_class_in_tmp_path") class TestPolynomialRegression: y = np.array( [ diff --git a/idaes/core/surrogate/pysmo/tests/test_radial_basis_function.py b/idaes/core/surrogate/pysmo/tests/test_radial_basis_function.py index 93a29116ac..610795a06b 100644 --- a/idaes/core/surrogate/pysmo/tests/test_radial_basis_function.py +++ b/idaes/core/surrogate/pysmo/tests/test_radial_basis_function.py @@ -152,6 +152,7 @@ def test_data_unscaling_minmax_06(self, array_type): FeatureScaling.data_unscaling_minmax(output_1, min_array, max_array) +@pytest.mark.usefixtures("run_class_in_tmp_path") class TestRadialBasisFunction: y = np.array( [ From c7116f5d1c86d9d5700b26b108cd4fb3b0401c86 Mon Sep 17 00:00:00 2001 From: Ludovico Bianchi Date: Wed, 18 Sep 2024 17:31:29 -0400 Subject: [PATCH 7/9] Try resolving next batch of failures --- idaes/core/surrogate/tests/test_pysmo_surrogate.py | 1 + .../subcritical_power_plant/tests/test_subcritical_flowsheets.py | 1 + idaes/tests/test_docs.py | 1 + 3 files changed, 3 insertions(+) diff --git a/idaes/core/surrogate/tests/test_pysmo_surrogate.py b/idaes/core/surrogate/tests/test_pysmo_surrogate.py index 0bc603af31..e1738d15c0 100644 --- a/idaes/core/surrogate/tests/test_pysmo_surrogate.py +++ b/idaes/core/surrogate/tests/test_pysmo_surrogate.py @@ -285,6 +285,7 @@ def test_init(self): assert init_func._model == None assert init_func.expression_str == "" + @pytest.mark.usefixtures("run_in_tmp_path") @pytest.mark.unit def test_model_poly(self, pysmo_output_pr): out1, vars = pysmo_output_pr diff --git a/idaes/models_extra/power_generation/flowsheets/subcritical_power_plant/tests/test_subcritical_flowsheets.py b/idaes/models_extra/power_generation/flowsheets/subcritical_power_plant/tests/test_subcritical_flowsheets.py index 90cab63c16..a91542911b 100644 --- a/idaes/models_extra/power_generation/flowsheets/subcritical_power_plant/tests/test_subcritical_flowsheets.py +++ b/idaes/models_extra/power_generation/flowsheets/subcritical_power_plant/tests/test_subcritical_flowsheets.py @@ -174,6 +174,7 @@ def test_dynamic_steam_cycle(): assert degrees_of_freedom(m) == 0 +@pytest.mark.usefixtures("run_in_tmp_path") @pytest.mark.skipif(not helmholtz_available(), reason="General Helmholtz not available") @pytest.mark.component def test_subcritical_recirculation_system(): diff --git a/idaes/tests/test_docs.py b/idaes/tests/test_docs.py index 7685c1b467..291af97425 100644 --- a/idaes/tests/test_docs.py +++ b/idaes/tests/test_docs.py @@ -109,6 +109,7 @@ def _have_sphinx(): return have_sphinx +@pytest.mark.usefixtures("run_in_tmp_path") @pytest.mark.component def test_doctests(docs_path): if _have_sphinx(): From ac4b833f5132bf4f68dea68c9ec76afaa77ed1d4 Mon Sep 17 00:00:00 2001 From: Ludovico Bianchi Date: Thu, 19 Sep 2024 00:40:17 -0400 Subject: [PATCH 8/9] Try addressing two failures --- idaes/core/surrogate/tests/test_pysmo_surrogate.py | 2 +- .../power_generation/flowsheets/test/test_subcritical_boiler.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/idaes/core/surrogate/tests/test_pysmo_surrogate.py b/idaes/core/surrogate/tests/test_pysmo_surrogate.py index e1738d15c0..fe1228331c 100644 --- a/idaes/core/surrogate/tests/test_pysmo_surrogate.py +++ b/idaes/core/surrogate/tests/test_pysmo_surrogate.py @@ -228,6 +228,7 @@ ) +@pytest.mark.usefixtures("run_class_in_tmp_path") class TestSurrogateTrainingResult: @pytest.fixture def pysmo_output_pr(self): @@ -285,7 +286,6 @@ def test_init(self): assert init_func._model == None assert init_func.expression_str == "" - @pytest.mark.usefixtures("run_in_tmp_path") @pytest.mark.unit def test_model_poly(self, pysmo_output_pr): out1, vars = pysmo_output_pr diff --git a/idaes/models_extra/power_generation/flowsheets/test/test_subcritical_boiler.py b/idaes/models_extra/power_generation/flowsheets/test/test_subcritical_boiler.py index 9bfd3c8b77..ed227a1d49 100644 --- a/idaes/models_extra/power_generation/flowsheets/test/test_subcritical_boiler.py +++ b/idaes/models_extra/power_generation/flowsheets/test/test_subcritical_boiler.py @@ -40,6 +40,7 @@ def model(): return main() +@pytest.mark.usefixtures("run_in_tmp_path") @pytest.mark.skipif(not helmholtz_available(), reason="General Helmholtz not available") @pytest.mark.component def test_basic_build(model): From 33edfd69f551d4f0e4b031145c418cac6fa2e860 Mon Sep 17 00:00:00 2001 From: Ludovico Bianchi Date: Thu, 19 Sep 2024 08:44:23 -0400 Subject: [PATCH 9/9] Switch to using module-level fixture for tmp_path workdir --- idaes/conftest.py | 16 ++++++++++++++++ .../core/surrogate/tests/test_pysmo_surrogate.py | 3 +-- .../flowsheets/test/test_subcritical_boiler.py | 4 +++- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/idaes/conftest.py b/idaes/conftest.py index 871d25a3fa..57d9103b04 100644 --- a/idaes/conftest.py +++ b/idaes/conftest.py @@ -270,6 +270,22 @@ def run_class_in_tmp_path( os.chdir(prev_workdir) +@pytest.fixture(scope="module") +def run_module_in_tmp_path( + request: pytest.FixtureRequest, tmp_path_factory: pytest.TempPathFactory +): + prev_workdir = os.getcwd() + + modname = request.module.__name__ + + tmp_path = tmp_path_factory.mktemp(modname, numbered=True) + try: + os.chdir(tmp_path) + yield + finally: + os.chdir(prev_workdir) + + def _get_repo_root_dir() -> Union[Path, None]: try: text = subprocess.check_output( diff --git a/idaes/core/surrogate/tests/test_pysmo_surrogate.py b/idaes/core/surrogate/tests/test_pysmo_surrogate.py index fe1228331c..cc22ac9cbc 100644 --- a/idaes/core/surrogate/tests/test_pysmo_surrogate.py +++ b/idaes/core/surrogate/tests/test_pysmo_surrogate.py @@ -46,7 +46,7 @@ from idaes.core.surrogate.metrics import compute_fit_metrics -dirpath = Path(__file__).parent.resolve() +pytestmark = pytest.mark.usefixtures("run_module_in_tmp_path") # String representation of json output for testing @@ -228,7 +228,6 @@ ) -@pytest.mark.usefixtures("run_class_in_tmp_path") class TestSurrogateTrainingResult: @pytest.fixture def pysmo_output_pr(self): diff --git a/idaes/models_extra/power_generation/flowsheets/test/test_subcritical_boiler.py b/idaes/models_extra/power_generation/flowsheets/test/test_subcritical_boiler.py index ed227a1d49..de56d59ed6 100644 --- a/idaes/models_extra/power_generation/flowsheets/test/test_subcritical_boiler.py +++ b/idaes/models_extra/power_generation/flowsheets/test/test_subcritical_boiler.py @@ -35,12 +35,14 @@ solver = get_solver() +pytestmark = pytest.mark.usefixtures("run_module_in_tmp_path") + + @pytest.fixture(scope="module") def model(): return main() -@pytest.mark.usefixtures("run_in_tmp_path") @pytest.mark.skipif(not helmholtz_available(), reason="General Helmholtz not available") @pytest.mark.component def test_basic_build(model):