Pytest for unit 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.

Addresses a part of #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

PyBaMM/tests/unit/test_expression_tree/test_operations/test_evaluate_python.py

Line 96 in 8056d22

expr = pybamm.Function(test_function, a)

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:

PyBaMM/tests/unit/test_expression_tree/test_functions.py

Line 42 in 8056d22

self.assertEqual(funca.name, "function (test_function)")

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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           

☔ View full report in Codecov by Sentry.
📢 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 #3857 (comment) 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 #3617 (comment).

[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:

PyBaMM/pybamm/expression_tree/operations/latexify.py

Line 82 in 39cff38

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

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.

[kratman]

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

[kratman]

Minor stuff

[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.

[agriyakhetarpal]

To fix the failing Windows tests, let's use the Agg backend for Matplotlib to not open up a GUI. We can either set the MPLBACKEND environment variable to "Agg" in the PYBAMM_ENV dictionary in noxfile.py, or we can do

from matplotlib import use
use("Agg")

wherever we have matplotlib being imported in the files of the tests/ folder. The latter would be preferable because it would available without nox (i.e., with just run-tests.py invocations too).

Edit: I noticed that we set MPLBACKEND to "Template" in run-tests.py, which would have set that backend back again when running the tests. Let's switch that to Agg as well.

[agriyakhetarpal]

This looks quite good on my end, and I have just a few additional comments. Let's merge after either @kratman or @Saransh-cpp can review again.

I noticed that we set MPLBACKEND to "Template" in run-tests.py, which would have set that backend back again when running the tests. Let's switch that environment variable to Agg as well.

This got lost in the conversation, probably – could you please do this too?

[kratman]

I don't see any major issues here, however @agriyakhetarpal has an open discussion, so lets make sure to resolve that before we merge the changes from @prady0t

[agriyakhetarpal]

Almost all good from my end, just the comment about removing the "Template" Matplotlib backend from run-tests.py needs to be addressed and a final CI run to be triggered, should be ready to merge after that.

[arjxn-py]

Nice work @prady0t, everything looks nice to me.

[agriyakhetarpal]

Thanks for all the fixes, @prady0t – let's merge when all of the unit tests pass (which will be when all of the integration tests pass, they run in alphabetical order)

[prady0t]

Thanks for all the fixes, @prady0t – let's merge when all of the unit tests pass (which will be when all of the integration tests pass, they run in alphabetical order)

Just few more months and i'll speed up the integration tests as well. 😉