First draft of tests on the ensemble_tools

[dleggat]

A first draft of the tests for ensemble_tools.

The tests use /tmp/ to create dummy sweep directories with a range of templated parameters, and check the directory structure and xml output for validity.

The tests work with pytest, but do not currently run in tox. I get the following error:

=========================================================================================================================================================== ERRORS ===========================================================================================================================================================
_______________________________________________________________________________________________________________________________________ ERROR collecting tests/test_ensemble_tools.py ________________________________________________________________________________________________________________________________________
ImportError while importing test module '/home/duncan/Work/aquifer/fabnesoPlugin/paraOverride/tests/test_ensemble_tools.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
FabNESO/tasks.py:13: in <module>
    from fabsim.base import fab
E   ModuleNotFoundError: No module named 'fabsim'

During handling of the above exception, another exception occurred:
/usr/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
tests/test_ensemble_tools.py:13: in <module>
    from FabNESO.ensemble_tools import create_dict_sweep, create_dir_tree, edit_parameters
FabNESO/__init__.py:3: in <module>
    from .tasks import neso, neso_ensemble
FabNESO/tasks.py:15: in <module>
    from base import fab
E   ModuleNotFoundError: No module named 'base'

Which I'm assuming means something in the .toml needs to be changed to allow pytest access to the base fabsim classes, but adding in the fabsim pythonpath argument to the pytest init hook hasn't solved the problem as I hoped it would.

Resolves #12

[matt-graham]

Ah yes this is going to be a pain. I think the root of the issue that tox by design creates a new isolated environment and install the package and its dependencies in it before running tests (or other commands). So while when you run pytest directly from an environment in which FabSim3 has been installed it will be able to import successfully, but running pytest from within tox, the manual manipulations of PYTHONPATH than the FabSim3 install performs do not have any effect and the isolated environment won't be able to import from fabsim.

There are a few routes we could go

1. Make the import from .tasks in FabNESO/__init__.py optional by wrapping in some try: ... except ImportError: logic. This will then allow Pytest to import FabNESO even when fabsim cannot be imported, and if we are only testing the ensemble_tools module for now, should suffice.
2. See if we can manually have a FabSim install made available within the isolated tox environment by manipulating PYTHONPATH in the tox environment definition. This is likely to be brittle and will require having FabSim3 set up in advance before being able to use tox, which means we'd also need to figure out how to get it installed on GitHub Actions runners.
3. Try and see if we can get FabSim3 to use a standard approach for installation as a package so we could directly specific it as dependency as normal for the plugin and have it automatically installed in the isolated environment.

Of these, (3) is I'd say the 'nicest' solution in terms of adhering to good practice but I suspect will be non-trivial to get done. I think for now (1) is probably the best option as it should hopefully be relatively simple to implement and I think is less brittle than (2).

[dleggat]

Agreed that (3) sounds ideal, but less immediately practical. I was trying to get something to work with (2), but couldn't work out how to get the right paths in the right places, and like you say it would have ended up very brittle either way. So I've put in a suppression for ImportError, that hopefully won't cause too many problems down the road?

[matt-graham]

Thanks @dleggat this is looking good - I've made some suggestions about separating out some of the checks in to different test functions (generally tests which check one or a few related conditions rather than lots of logically distinct conditions are preferred from a perspective of failure in test reports then being more granular and helping to identify the specific issue needing to be addressed) and also in parameterizing some tests to cover a wide range of cases.

[matt-graham]

Thanks @dleggat for the updates. I've made a few small further suggestions but none are essential. I'll open issues for the points I made about factoring out some functions and adding unit tests for these.