Pytest for testing

[prady0t]

Description

Still a WIP. Since most of the integration tests were failing, they need to be rewritten completely before we can switch to pytest. For now we are using pytest just for unit tests. We still reduce testing time while running nox by a considerable factor.

Fixes #3617

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

• [ ] New feature (non-breaking change which adds functionality)
• [ ] Optimization (back-end change that speeds up the code)
• [ ] Bug fix (non-breaking change which fixes an issue)

Key checklist:

• [ ] No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
• [ ] All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• [ ] The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s quick).

Further checks:

• [ ] Code is commented, particularly in hard-to-understand areas
• [ ] Tests added that prove fix is effective or that feature works

[prady0t]

Reason why tests are failing is because pytest recognises arg, arg1, arg2 from across different function's arguments as pytest fixtures. I tried using lambda function in place of

def test_function(arg):
     return arg + arg

using:

expr = pybamm.Function(lambda arg: arg+arg, a)

insted of https://github.com/pybamm-team/PyBaMM/blob/8056d224e75b3926743319a4743abb7c6ab0dac2/tests/unit/test_expression_tree/test_operations/test_evaluate_python.py#L96

But assertion fails (in unittest and pytest):

AssertionError: 'function (<lambda>)' != 'function (test_function)'
- function (<lambda>)
+ function (test_function)

Maybe if we define arg fixture we could solve this.
@agriyakhetarpal what do you think?

[prady0t]

One easy fix is to rename functions like test_function to maybe function1 and others respectively. Due to pytest naming convention, it recognises these as test functions.

Edit: I just noticed I did not change assertion statement here while using lambda functions: https://github.com/pybamm-team/PyBaMM/blob/8056d224e75b3926743319a4743abb7c6ab0dac2/tests/unit/test_expression_tree/test_functions.py#L42 I can still try solving this with them but that would mean completely getting rid of test_function type functions.

[agriyakhetarpal]

Does renaming them from test_function to something like function_for_testing help? Using a lambda function is not only a bit hard on readability but also it would not be able to cover every use case (returns a single expression and you might need to define a different one everywhere)

[prady0t]

yes we can also do that.

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.60%. Comparing base (a3db966) to head (ed1a1a5).

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #3857   +/-   ##
========================================
  Coverage    99.60%   99.60%           
========================================
  Files          259      259           
  Lines        21347    21347           
========================================
  Hits         21263    21263           
  Misses          84       84           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[prady0t]

Why are the CI tasks failing?

[agriyakhetarpal]

Why are the CI tasks failing?

It's probably coming from Matplotlib opening too many figures/plots when running the tests. We should close them after they are opened, I guess. There might be a few other failures due to differences in how unittest and pytest handle test cases, so you might have to fix the tests.

Also, according to https://github.com/pybamm-team/PyBaMM/pull/3857#issuecomment-2001808198 that the coverage is 19.40%, which means that the coverage percentage is not being reported correctly. We would need to add pytest-cov and configure it with the unit tests, as noted by @Saransh-cpp in https://github.com/pybamm-team/PyBaMM/issues/3617#issuecomment-1854702640.

[prady0t]

These are mostly the failures we have to deal with :

FAILED tests/unit/test_expression_tree/test_operations/test_latexify.py::TestLatexify::test_latexify -   File "/Users/runner/work/PyBaMM/PyBaMM/pybamm/expression_tree/operations/latexify.py", line 82
    geo_latex = f"\quad {rng_min} < {name} < {rng_max}"
                                                      ^
SyntaxError: invalid escape sequence '\q'

FAILED tests/unit/test_expression_tree/test_operations/test_latexify.py::TestLatexify::test_latexify_other_variables -   File "/Users/runner/work/PyBaMM/PyBaMM/pybamm/expression_tree/operations/latexify.py", line 82
    geo_latex = f"\quad {rng_min} < {name} < {rng_max}"
                                                      ^
SyntaxError: invalid escape sequence '\q'

FAILED tests/unit/test_expression_tree/test_operations/test_latexify.py::TestLatexify::test_sympy_preview -   File "/home/runner/work/PyBaMM/PyBaMM/pybamm/expression_tree/operations/latexify.py", line 82
    geo_latex = f"\quad {rng_min} < {name} < {rng_max}"
                                                      ^
SyntaxError: invalid escape sequence '\q'

FAILED tests/unit/test_util.py::TestUtil::test_import_optional_dependency - AssertionError: "Optional dependency anytree is not available." does not match "Optional dependency anytree.exporter is not available. See https://docs.pybamm.org/en/latest/source/user_guide/installation/index.html#optional-dependencies for more details."

FAILED tests/unit/test_simulation.py::TestSimulation::test_create_gif - _tkinter.TclError: Can't find a usable init.tcl in the following directories: 
    {C:\hostedtoolcache\windows\Python\3.11.8\x64\tcl\tcl8.6}

C:/hostedtoolcache/windows/Python/3.11.8/x64/tcl/tcl8.6/init.tcl: couldn't read file "C:/hostedtoolcache/windows/Python/3.11.8/x64/tcl/tcl8.6/init.tcl": No error
couldn't read file "C:/hostedtoolcache/windows/Python/3.11.8/x64/tcl/tcl8.6/init.tcl": No error
    while executing
"source C:/hostedtoolcache/windows/Python/3.11.8/x64/tcl/tcl8.6/init.tcl"
    ("uplevel" body line 1)
    invoked from within
"uplevel #0 [list source $tclfile]"


This probably means that Tcl wasn't installed properly.

FAILED tests/unit/test_plotting/test_plot.py::TestPlot::test_plot

I don't understand failures with tcl and anytree. They fail randomly on different envs.

[agriyakhetarpal]

I think the anytree error in import_optional_dependency is related to an incorrect error message for the dependency (use use anytree.exporter specifically, but we import anytree). Could you try that test with changes from #3892 applied? It should get merged soon.

Re: the Matplotlib error, I'm not sure either. It looks like an unrelated error and is probably coming from actions/setup-python? Does the test work for you locally?

[prady0t]

I did encounter anytree but not the test_plot error. So that might be the case. Also we can remove invalid escape sequence failure by using raw string, instead of:

https://github.com/pybamm-team/PyBaMM/blob/39cff38cc7c424285761101c3e046e0d04af5dff/pybamm/expression_tree/operations/latexify.py#L82

we can do:

        geo_latex = fr"\quad {rng_min} < {name} < {rng_max}"

Can we do that without any breaking changes?

[agriyakhetarpal]

Can we do that without any breaking changes?

I think so, yes. To verify that the change doesn't break anything, you can try out the latexify notebook and ensure that the outputs remain the same

[prady0t]

We should have more number of checks with this.

[prady0t]

How to get covergae report? I tried running Running

 python -m pytest --cov tests/unit  

but it's unable to find tests. Also running

coverage run -m unittest discover tests/unit report  

only gives me 20% coverage. What am I doing wrong?

[Saransh-cpp]

The command should be:

pytest --cov=pybamm tests/unit

More on coverage with pytest here - https://learn.scientific-python.org/development/guides/coverage/

[prady0t]

The command should be:

pytest --cov=pybamm tests/unit

More on coverage with pytest here - https://learn.scientific-python.org/development/guides/coverage/

Oh wrong command. This was embarrassing. 🙃 I'm now at 96% coverage.

[prady0t]

Is the doctests failure related to changes in this PR? Do I need to integrate xdoctest for it to pass?

[kratman]

Is the doctests failure related to changes in this PR? Do I need to integrate xdoctest for it to pass?

The tests appear to be passing on other branches, so most likely it is something in your PR. If you really get stuck then I can try to help out

[prady0t]

Is the doctests failure related to changes in this PR? Do I need to integrate xdoctest for it to pass?

The tests appear to be passing on other branches, so most likely it is something in your PR. If you really get stuck then I can try to help out

Let me try integrating pytest plugins for doctest and coverage. This might solve it.

[agriyakhetarpal]

The import pytest error should get fixed if you add the [dev] set of optional dependencies to be installed in the doctests and the docs nox sessions.

[prady0t]

Reason for failing tests in py3.12 https://www.python.org/downloads/release/python-3122/#:~:text=Invalid%20backslash%20escape,in%20the%20future.)

[prady0t]

Most of the tests are passing now. 😄

[agriyakhetarpal]

This JAX test has previously failed for us sometimes and looks to be flaky – in a separate PR later on, we can explore some of the available plugins and decorate those with @pytest.mark.flaky(). For now, I re-triggered the failing job

[prady0t]

This JAX test has previously failed for us sometimes and looks to be flaky – in a separate PR later on, we can explore some of the available plugins and decorate those with @pytest.mark.flaky(). For now, I re-triggered the failing job

Should I open an issue regarding this?

[agriyakhetarpal]

The parallel testing looks great – GitHub Actions runners do not have a lot of resources so I guess running locally will reveal the speedups more prominently. I can review this in a while – for now, we will also require updates to the Contributing guide and source installation instructions wherever applicable (while mentioning that the unit tests use pytest and the integration tests still use unittest)

P.S. the failing Lychee job looks fixable, could you replace it with a different link, i.e., CasADi 2.0.0 isn't really relevant anymore and we could add links to the stable documentation?

[agriyakhetarpal]

Should I open an issue regarding this?

Sure! It would be good to track that

[prady0t]

The parallel testing looks great – GitHub Actions runners do not have a lot of resources so I guess running locally will reveal the speedups more prominently. I can review this in a while – for now, we will also require updates to the Contributing guide and source installation instructions wherever applicable (while mentioning that the unit tests use pytest and the integration tests still use unittest)

P.S. the failing Lychee job looks fixable, could you replace it with a different link, i.e., CasADi 2.0.0 isn't really relevant anymore and we could add links to the stable documentation?

Let me look into it. Also is there a reason why running tests on macOS based runners are generally slower? (for me running on macos, py 3.10 locally takes about 12-14 min which is still faster than the time taking by runners but is significantly lot when compared to linux and windows )

[agriyakhetarpal]

Also is there a reason why running tests on macOS based runners are generally slower? (for me running on macos, py 3.10 locally takes about 12-14 min which is still faster than the time taking by runners but is significantly lot when compared to linux and windows )

I ~don't~ do think I'm able to reproduce that – up to 94% completion or so on macOS locally the test suite is quite fast, but then I face some heavy slowdowns when running the tests for the IDAKLU solver, IDAKLUJax tests, and the JAX BDF solver tests – and then with some test cases in tests/unit/test_meshes/test_one_dimensional_submesh.py and the scikits.odes solvers. But since these tests work fine in the serial implementation, could you try this solution in the meantime: https://github.com/pytest-dev/pytest-xdist/issues/385#issuecomment-1304877301? This is the same thread as the one I had shared earlier on Slack in the infrastructure channel when we were discussing pytest-profiling. I did see later on that JAX itself is using pytest-xdist, so it likely has something to do with our solver rather than being an upstream bug.

Also, it looks like the macOS Python 3.12 tests are failing for a different reason now, oops! :) Could you try fixing that? I have triggered another run just now to confirm it wasn't a one-off failure. Edit: okay, it passes now, but we have to ensure that the order of the tests is deterministic (i.e., test_util.py::test_import_optional_dependency and related tests should always be tested at the last).

[kratman]

okay, it passes now, but we have to ensure that the order of the tests is deterministic (i.e., test_util.py::test_import_optional_dependency and related tests should always be tested at the last).

The tests are parallel by default in PyTest, shouldn't run order be irrelevant? I would expect that PyTest is sort of sandboxing the tests.

If the optional dependencies tests are a problem, then they should not be run with the regular tests. That is more of an integration test than a unit test anyway.

[kratman]

I am going to merge develop into this to see if the test failures happen again

[kratman]

P.S. the failing Lychee job looks fixable, could you replace it with a different link, i.e., CasADi 2.0.0 isn't really relevant anymore and we could add links to the stable documentation?

Yeah we should just have a link to the main docs page instead of a versioned one.