PyBaMM icon indicating copy to clipboard operation
PyBaMM copied to clipboard

Use `pytest`'s fixtures to reduce repetition across tests

Open prady0t opened this issue 1 year ago • 10 comments

Pytest offers various features, one of them being the use of fixtures. This helps us better manage the tests and setup preconditions. Using the pytest.mark.parameterize fixture helps us give multiple inputs to test functions. Learn more : https://docs.pytest.org/en/stable/how-to/parametrize.html.

We can take for example

        assert isinstance(processed[1][0][0], np.ndarray)
        assert isinstance(processed[1][0][1], np.ndarray)
        assert isinstance(processed[1][1], np.ndarray)

inside a fixture and work up things accordingly.

prady0t avatar Oct 09 '24 04:10 prady0t

@agriyakhetarpal We can put this for hacktoberfest.

prady0t avatar Oct 09 '24 04:10 prady0t

I added the labels – additionally, could you please link to some code examples (a few low-hanging fruits) where you've noticed that this is the case, so that it's easier for potential contributors to know what to start with?

agriyakhetarpal avatar Oct 09 '24 18:10 agriyakhetarpal

Hey would love to pick up this issue ! I understand that we must set up fixtures to handle multiple cases here and thereby consolidating tests for different inputs into a single, reusable function, therefore making the code more concise. Can I start working on this ?

Aswinr24 avatar Oct 10 '24 18:10 Aswinr24

Sure @Aswinr24, feel free to work on this and also don't hesitate to let us know if you need help at any point :)

arjxn-py avatar Oct 11 '24 09:10 arjxn-py

Could You Please Review my PR and suggest if any changes are required

Aswinr24 avatar Oct 12 '24 07:10 Aswinr24

Hi @agriyakhetarpal, hope you're doing well! I've opened this PR as part of a series where I aim to parameterize tests wherever possible, following your suggestions. Could you please review this PR and let me know if any changes are needed?

Thank you!

Aswinr24 avatar Dec 25 '24 18:12 Aswinr24

Hi! I noticed some PRs already linked to this issue, is there still scope for additional refactoring using pytest fixtures in other test modules? I’d be happy to help with that.

swastim01 avatar Oct 30 '25 10:10 swastim01

Hi, @swastim01! Indeed, there is still scope for additional refactoring. There is #4401 that @prady0t previously started, but it wasn't touched for a while and was later closed. The idea is that we can move most of our shared.py test harnesses into conftest.py. I am open to hearing ideas on how this could be done. I suppose you could draw some inspiration from that PR in a PR of your own (while including @prady0t's previous commits, of course!)

agriyakhetarpal avatar Nov 13 '25 03:11 agriyakhetarpal

@agriyakhetarpal, thanks for the guidance! I had a look at #4401, the full removal of shared.py was a bit too broad, but the idea of moving the reusable test setup into conftest.py seems workable.

I can take this forward in smaller steps by extracting the commonly used parts of shared.py into fixtures and updating tests gradually. Before I begin, could you confirm the preferred scope? Should I start with tests/unit/parameters/ or widen it to other modules where repetition is obvious?

swastim01 avatar Nov 24 '25 17:11 swastim01

Thanks for thinking through this. I think the preferred scope is broad but the improvements are limited; we've done a lot of improvements for the test suite over the years. Starting with the parameters tests sounds best. Smaller but multiple PRs with obvious repetition removed will be easier for us to review – limiting to a few hundred lines of code and a few files at a time. We can keep the shared.py file to whatever extent its contents can't be removed.

agriyakhetarpal avatar Nov 24 '25 17:11 agriyakhetarpal