Closed ehogan closed 1 week ago
I have been looking into this issue and have discovered the following:
pytest
defaults to the prepend
import mode instead of the importlib
import mode pytest
recommends for new projects. This results in a drawback; test files must have unique names, see https://docs.pytest.org/en/7.1.x/explanation/goodpractices.html#choosing-an-import-mode.--doctest-modules
option for pytest
in ESMValTool's setup.cfg
file, pytest
imports the code, meaning our code files must also have unique names (so the above situation, where a diag_scripts/arctic_ocean/plotting.py
module exists alongside a diag_scripts/climate_patterns/plotting.py
module, would not be possible). __init__.py
files to your tests folder and subfolders (in our case that would be the diag_scripts
folder and subfolders), changing them to packages. (Note there is also a diag_scripts/autoassess/stratosphere/plotting.py
module, but the diag_scripts/autoassess/stratosphere/
directory contains an __init__.py
file, so the plotting.py
module here retains its namespace.)pytest
recommends importlib
for new projects, and that they did intend for this mode to be the default mode, see https://docs.pytest.org/en/7.1.x/explanation/pythonpath.html#import-modes, I would suggest making the following change to enable this:diff --git a/setup.cfg b/setup.cfg
index c738c5d71..e28f8079a 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -1,5 +1,6 @@
[tool:pytest]
addopts =
+ --import-mode=importlib
--doctest-modules
--ignore=doc/sphinx/source/conf.py
--cov=esmvaltool
This causes different import errors, e.g.:
__________________ ERROR collecting esmvaltool/diag_scripts/climate_patterns/climate_patterns.py ___________________
ImportError while importing test module '/ESMValTool/esmvaltool/diag_scripts/climate_patterns/climate_patterns.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
esmvaltool/diag_scripts/climate_patterns/climate_patterns.py:40: in <module>
import sub_functions as sf
E ModuleNotFoundError: No module named 'sub_functions'
which will need updating. I would be happy to do this, but I would like some input from the @ESMValGroup/technical-lead-development-team whether you would prefer absolute or relative imports. For example:
import esmvaltool.diag_scripts.climate_patterns.sub_functions as sf
import .sub_functions as sf
I like absolute imports, as it's really clear what is being imported, but the relative imports make it easy to relocate subpackages without needing to edit import statements in modules within the subpackage.
What are your preferences? 🤔
Pep8 recommends absolute imports, so those should be preferred: https://peps.python.org/pep-0008/#imports
Maybe you could use some of the changes from #3646
Pep8 recommends absolute imports, so those should be preferred: https://peps.python.org/pep-0008/#imports
Great!
Maybe you could use some of the changes from #3646
Yes, I see about 15-20 import changes within the diag_scripts
directory that appear to align with changes from #3646; would you be happy for me to capture those in a new PR against this issue so we can solve the failing tests on main
, please? 😊
Yes, that would be great!
I noticed that the tests are failing since the CMIP6 climate patterns PR (#2785) was merged:
All the checks were passing on the PR. Is it possible to update the PR checks to catch issues like this?
I will open a PR now with a fix 😊