Fix failing tests after CMIP6 climate patterns merge

[ehogan]

I noticed that the tests are failing since the CMIP6 climate patterns PR (#2785) was merged:

 _ ERROR collecting esmvaltool/diag_scripts/climate_patterns/climate_patterns.py _
ImportError while importing test module '/home/runner/work/ESMValTool/ESMValTool/esmvaltool/diag_scripts/climate_patterns/climate_patterns.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
../../../miniconda3/envs/esmvaltool-fromlock/lib/python3.11/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
esmvaltool/diag_scripts/climate_patterns/climate_patterns.py:41: in <module>
    from plotting import (
E   ImportError: cannot import name 'plot_timeseries' from 'plotting' (/home/runner/work/ESMValTool/ESMValTool/esmvaltool/diag_scripts/arctic_ocean/plotting.py)
____ ERROR collecting esmvaltool/diag_scripts/climate_patterns/plotting.py _____
import file mismatch:
imported module 'plotting' has this __file__ attribute:
  /home/runner/work/ESMValTool/ESMValTool/esmvaltool/diag_scripts/arctic_ocean/plotting.py
which is not the same as the test file we want to collect:
  /home/runner/work/ESMValTool/ESMValTool/esmvaltool/diag_scripts/climate_patterns/plotting.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules

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 😊

[ehogan]

I have been looking into this issue and have discovered the following:

• For historical reasons, 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.
• Due to the specification of the --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).
• A workaround is to add __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.)
• However, given 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:

• Absolute import fix:

    import esmvaltool.diag_scripts.climate_patterns.sub_functions as sf

• Relative import fix:

    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? 🤔

[bouweandela]

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

[ehogan]

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? 😊

[bouweandela]

Yes, that would be great!