Mock all the optional dependencies to testing for better coverage

[arjxn-py]

Currently we run all the tests with pybamm[all] with all optional dependencies installed on CI but as we also maintain pybamm without optional dependencies it is necessary to cover that in tests too.

We might need to introduce a new step in the job matrix with pybamm without optional dependencies installed to run tests (a number of ModuleNotFound failures are expected which should be refactored on the go)

[agriyakhetarpal]

Even a separate job in the test_on_push.yml workflow for a single Python version should suffice – we can install and test PyBaMM with custom steps there rather than using nox (or even just run a single file or the function itself with pytest – this could be tied in with #3617 later on).

Cross-posting #3848 (review) where a solution is available for Python 3.10 and later.

[lorenzofavaro]

I'd be happy to work on this issue if possible 🙋‍♂️

[agriyakhetarpal]

Hello @lorenzofavaro, thanks – I can assign you to this issue. Just one job on Linux where no optional dependencies are installed with the dev session would be okay in this case. We can do this without altering noxfile.py.

[lorenzofavaro]

Thanks @agriyakhetarpal.

By using the nox dev session do we not also install optional dependencies?

# noxfile.py

@nox.session(name="dev")
def set_dev(session):
  # ...
  if sys.platform == "linux":
    # ...
    session.run(
        python,
        "-m",
        "pip",
        "install",
        "-e",
        ".[all,dev,jax,odes]",
        external=True,
    )

Shouldn't we create a new function to install just dev dependencies to run the tests?

session.run(
    python,
    "-m",
    "pip",
    "install",
    "-e",
    ".[dev]",
    external=True,
)

Then we could configure a new job in the workflow test_on_push.yml so that it launches the refactored unit & integration tests.

[agriyakhetarpal]

We actually use the dev session for local development, but not in CI (it is used to set up a virtual environment in a folder venv/ in the root directory that a user can activate directly).

I was going to suggest that we can write these commands directly in a job for Python 3.11 (or 3.12) running on Linux in test_on_push.yml, since providing a nox session for this feels too boilerplate.

[lorenzofavaro]

Alright, so running all the tests with the core PyBaMM gives us 68 errors, as expected.

But how do we want to handle these errors for optional dependencies?
Should we:

• edit all the affected tests (to expect a ModuleNotFoundError if the extra library isn't installed, and use importorskip when migration to pytest has been performed), and launch it with:

  python run-tests.py --all

  or
• edit just the test_import_optional_dependency function (mocking all the optional dependencies tests inside it) and launch it with:

  pytest tests/unit/test_util.py TestUtil.test_import_optional_dependency

  or
• create and launch a whole new test module specifically for the optional dependencies

By the way, right now the test_import_optional_dependency includes a test for anytree that, if I'm not wrong, is actually a core dependency, not an optional one.

[agriyakhetarpal]

I would say the second one "edit just the test_import_optional_dependency function" sounds better to me, it's a lot less work at the time in comparison to modifying all of the test cases (there are several places where changes would be required).

Having a separate Python 3.11 job + running just this test with pytest (or with unittest) would not take long and should be great, xref #3617 for future reference.

By the way, right now the test_import_optional_dependency includes a test for anytree that, if I'm not wrong, is actually a core dependency, not an optional one.

That is right – I believe this was overlooked in #3475 and we didn't realise fixing this even in #3848 (cc: @arjxn-py). Could you resolve that? Perhaps #3848 (review) might be useful (but if there's a better API to parse optional dependencies then we should go ahead with that).

[lorenzofavaro]

That is right – I believe this was overlooked in #3475 and we didn't realise fixing this even in #3848 (cc: @arjxn-py). Could you resolve that? Perhaps #3848 (review) might be useful (but if there's a better API to parse optional dependencies then we should go ahead with that).

Yes of course, I'll solve it in my next PR.