Migrating unittest to pytest 6

[prady0t]

I've tried using fixture and parameterized in this PR. Do let me know places I've missed (inside this PR).

#4156

[agriyakhetarpal]

We initialise the geometry variable below, do we need it in the arguments of the function?

[agriyakhetarpal]

Also, this test can be refactored since we are testing the same thing repeatedly. Please add this to the tracking issue.

[prady0t]

Now sure how to refactor. Could you help me more?

[agriyakhetarpal]

Usually, in such situations, it's better to make the test shorter and distribute it into multiple tests. For example, here, you could rewrite test_geometry into an array of smaller tests (haven't added the self keyword here for brevity):

def test_battery_geometry_current_collector_tabs():
    geometry = pybamm.battery_geometry(
        options={
            "particle size": "distribution",
            "dimensionality": 1,
        },
    )
    assert "tabs" in geometry["current collector"]

and

def test_battery_geometry_cylindrical_dimensionality():
    geo = pybamm.geometric_parameters
    geometry = pybamm.battery_geometry(
        form_factor="cylindrical", options={"dimensionality": 1}
    )
    assert geometry["current collector"]["r_macro"]["min"] == geo.r_inner
    assert geometry["current collector"]["r_macro"]["max"] == 1

However, readability is important. So, if refactoring introduces too many conditions or makes the code hard to read, it is best to not bother and keep the test as is.

Another aspect is to remove the loops by parametrizing. For example, the loop for cc_dimension in [0, 1, 2]: can be refactored into

@pytest.mark.parametrize("cc_dimension", [0, 1, 2])
def test_battery_geometry_basics(cc_dimension):

and similarly

@pytest.mark.parametrize("electrode, particle_type", [
    ("negative", "primary"),
    ("negative", "secondary"),
    ("positive", "primary"),
    ("positive", "secondary"),
])
def test_battery_geometry_particle_phases(electrode, particle_type):
    options = {"particle phases": "2"}
    geometry = pybamm.battery_geometry(options=options)
    geo = pybamm.GeometricParameters(options=options)
# followed by the rest of the test

and so on. I hope this can get you going!

[agriyakhetarpal]

I'm happy to write more examples if needed, it's easier than you would think :)

[arjxn-py]

These examples looks enough to me. Thanks :)
I guess we don't need case or if/else then as we using parametrize marker?

[agriyakhetarpal]

In some cases, we might still need them.

@pytest.mark.parametrize("foo, bar", [0, 1, 2, 3, 4])
def test_something(foo, bar):
    # Some logic for initialising "foo" and "bar" here
    # and setting attributes, etc.
    # [...]

    # Conditional logic
    match foo.value:
        case 1:
            # do something
            assert foo in bar
        case 3.value:
            # do something else
            assert bar in foo

    # not (1) and not (3) – i.e., for (0) or (2) or (4)
    # Some other logic that applies to the rest of the cases
    assert foo.value + bar.value > 30

but it depends on the test. It might be better to handle such cases without parameterizing them

[agriyakhetarpal]

This test can be refactored, too.

[prady0t]

Not sure why the error is. I was getting another regex error locally can you run CI jobs again?

[agriyakhetarpal]

Since this PR seems to have taken a bit of time (for all the right reasons), let's nudge this towards a merge asap and work on the parameterization in further, dedicated PRs in your coming weeks – once the pytest migration gets finished.

[prady0t]

I reverted the changes in test_OKane2022.py I still need to figure out the regex error. In the meantime, I'm also working on the last migration. I'll try to open PR by today or tomorrow morning.

[prady0t]

I've removed pytest conversion from test_base_battery_model.py. This should work

[agriyakhetarpal]

Looks nice, thanks, @prady0t! Just one more change, and we can merge this if the tests pass.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.41%. Comparing base (7f263e4) to head (5c1e293).
Report is 182 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4354   +/-   ##
========================================
  Coverage    99.41%   99.41%           
========================================
  Files          292      292           
  Lines        22223    22223           
========================================
  Hits         22093    22093           
  Misses         130      130           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.