Test failure in test_config.py if SATPY_CONFIG_PATH set

[gerritholl]

Describe the bug

If the environment SATPY_CONFIG_PATH is set, test_deprecated_env_vars in test_config.py fails.

To Reproduce

SATPY_CONFIG_PATH=seitan pytest test_config.py

Expected behaviour

I expect all tests to pass independent of environment variables.

Actual results

==================================================================== test session starts =====================================================================
platform linux -- Python 3.9.7, pytest-6.2.5, py-1.10.0, pluggy-1.0.0
rootdir: /data/gholl/checkouts/satpy
plugins: lazy-fixture-0.6.3
collected 8 items

test_config.py ....F...                                                                                                                                [100%]

========================================================================== FAILURES ==========================================================================
_________________________________________________________ TestConfigObject.test_deprecated_env_vars __________________________________________________________

self = <satpy.tests.test_config.TestConfigObject object at 0x7f366433d2e0>

    def test_deprecated_env_vars(self):
        """Test that deprecated variables are mapped to new config."""
        from importlib import reload
        import satpy
        old_vars = {
            'PPP_CONFIG_DIR': '/my/ppp/config/dir',
            'SATPY_ANCPATH': '/my/ancpath',
        }

        with mock.patch.dict('os.environ', old_vars):
            reload(satpy._config)
            reload(satpy)
            assert satpy.config.get('data_dir') == '/my/ancpath'
>           assert satpy.config.get('config_path') == ['/my/ppp/config/dir']
E           AssertionError: assert ['seitan'] == ['/my/ppp/config/dir']
E             At index 0 diff: 'seitan' != '/my/ppp/config/dir'
E             Use -v to get the full diff

test_config.py:150: AssertionError
--------------------------------------------------------------------- Captured log call ----------------------------------------------------------------------
WARNING  satpy._config:_config.py:91 'SATPY_ANCPATH' is deprecated. Please use 'SATPY_DATA_DIR' instead.
====================================================================== warnings summary ======================================================================
satpy/tests/test_config.py::TestBuiltinAreas::test_areas_pyproj
satpy/tests/test_config.py::TestBuiltinAreas::test_areas_pyproj
satpy/tests/test_config.py::TestBuiltinAreas::test_areas_rasterio
satpy/tests/test_config.py::TestBuiltinAreas::test_areas_rasterio
  /data/gholl/mambaforge/envs/py39/lib/python3.9/site-packages/pyproj/crs/crs.py:131: FutureWarning: '+init=<authority>:<code>' syntax is deprecated. '<authority>:<code>' is the preferred initialization method. When making the change, be mindful of axis order changes: https://pyproj4.github.io/pyproj/stable/gotchas.html#axis-order-changes-in-proj-6
    in_crs_string = _prepare_from_proj_string(in_crs_string)

satpy/tests/test_config.py: 113 warnings
  /data/gholl/mambaforge/envs/py39/lib/python3.9/site-packages/pyproj/crs/crs.py:1256: UserWarning: You will likely lose important projection information when converting to a PROJ string from another format. See: https://proj.org/faq.html#what-is-the-best-format-for-describing-coordinate-reference-systems
    return self._crs.to_proj4(version=version)

-- Docs: https://docs.pytest.org/en/stable/warnings.html
================================================================== short test summary info ===================================================================
FAILED test_config.py::TestConfigObject::test_deprecated_env_vars - AssertionError: assert ['seitan'] == ['/my/ppp/config/dir']
========================================================= 1 failed, 7 passed, 117 warnings in 0.70s ==========================================================

Environment Info:

• OS: openSUSE 15.3
• Satpy Version: v0.30.1

Additional context

This bug falls in the same category as #1763.

[djhoese]

I just realized, now that we use pytest we could configure/setup all tests in a conftest.py in the tests directory and make it unset these environment variables (it would have to re-set them afterward).

[gerritholl]

There exists monkeypatch.delenv for that, which could be set in a global autouse fixture, I think.

[djhoese]

Doing it that way would unset/set the env var for every test right?

I was hoping something like this could be put in conftest.py:

https://docs.pytest.org/en/6.2.x/reference.html#pytest.hookspec.pytest_configure

I don't know if I've ever done it though.

[gerritholl]

Doing it that way would unset/set the env var for every test right?

Depends on the fixture scope, I think. With the default (function) scope, it would run for every test. Isn't that what we want? That way, we also protect against tests which aren't cleaning up after themselves properly. I've seen more than once that a test would pass in isolation, but fail when running the whole suite (and I've even seen the other way around). But I think if the autouse fixture scope was class, module, or session, it would run only once per scope. I might have used that for creating files, but for something as fast as (un)setting an environment variable, I wouldn't expect a noticeable difference.

I'm not familiar with initialisation hooks or plugins in pytest, and I don't understand how it would be different from an autouse fixture with session scope.

[djhoese]

True. If you use per-function fixtures in your session-scoped fixtures (monkeypatch in our custom session fixture) it will raise an error.

I'm a little torn on what's best here. Is it better to write code to make it harder for tests to interfere with each other (resetting env every test) or let bad tests mess up other tests environments? I lean towards the latter as we (the maintainers) may not be able to defend against all interference between tests. By hiding side effects of a test we may be also hiding bugs in the actual user-facing code.