Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: use pytest fixtures to parameterize tests for lead acid battery model #4723

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Aswinr24
Copy link
Contributor

Description

Refactored the lead-acid model test cases to use pytest fixtures, reducing code duplication and improving test clarity and maintainability.

Related to #4502

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.

  • Code refactor (test improvements to reduce repetition)

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 $ 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

Copy link

codecov bot commented Dec 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.69%. Comparing base (ef8571c) to head (4ef92a6).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4723   +/-   ##
========================================
  Coverage    98.69%   98.69%           
========================================
  Files          303      303           
  Lines        23226    23226           
========================================
  Hits         22923    22923           
  Misses         303      303           

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

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't gone through the tests in detail, just skimmed them for now – I think this looks wrong because tests for options such as {"current collector": "potential pair", "dimensionality": 1} that were previously present have been removed, apparently. The tests for the lumped thermal model are not present either (and others). Is this PR ready for review, @Aswinr24, or is it in progress and you're writing more tests? Apologies if it's the latter (and please mark it as a draft if it is!)

@Aswinr24 Aswinr24 marked this pull request as draft December 30, 2024 16:26
@prady0t
Copy link
Contributor

prady0t commented Dec 31, 2024

@Aswinr24 Thanks for adding fixtures across tests! I suggest covering multiple files per PR so we don't have too many PRs covering single files. While you are working with this file, try to find more tests that can be parameterized using fixtures and cover those in this PR as well.

Once you have enough tests, you can mark this for review. Also, make a list of files (for yourself, no need to open any tracking issue) to keep track of the files you went through and the ones that are pending.

I'll add my reviews once it's marked as ready.

@Aswinr24
Copy link
Contributor Author

Thanks for the feedback @prady0t @agriyakhetarpal !
Sure will try adding multiple files in this PR and cover the tests that I believe can be parameterized using fixtures

@Aswinr24
Copy link
Contributor Author

Aswinr24 commented Jan 4, 2025

@prady0t @agriyakhetarpal Could You Please Review this.
I have tried to cover most of the tests which can be parameterized using fixtures, particularly in case of the Lead Acid Battery Model.
Would like to hear your feedback on this

@Aswinr24 Aswinr24 marked this pull request as ready for review January 4, 2025 09:53
@Aswinr24 Aswinr24 changed the title refactor: use pytest fixtures to reduce repetition in tests refactor: use pytest fixtures to parameterize tests for lead acid battery model Jan 4, 2025
import pybamm
import tests

import pytest
import numpy as np


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are the classes removed across all files? My suggestion is to add them back and put all fixtures outside the class (for a better code structure).

Copy link
Contributor

@prady0t prady0t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just by looking at the changes, I think we can use parametrize fixture here. @Aswinr24 can you maybe look into ways you can parametrize these tests?
Before that please add the class structure back, it would be easier to review with a cleaner diff.

@Aswinr24
Copy link
Contributor Author

Aswinr24 commented Jan 5, 2025

@prady0t Thanks for the review, added back the class structure and tried to use the parameterize fixture wherever feasible.
Could you check this now

@prady0t
Copy link
Contributor

prady0t commented Jan 7, 2025

It looks good. The tests look clean! The CI failure is unrelated to this PR, as the tests pass locally for me. Maybe we should re-run. @agriyakhetarpal would love your thoughts on these changes.

@Aswinr24
Copy link
Contributor Author

@agriyakhetarpal could you please review this submission and check if it is good to go?
Waiting to hear your thoughts on the changes done

@agriyakhetarpal
Copy link
Member

Hi @Aswinr24, thanks for your work! I haven't had time to look at these changes in detail, sorry for the delay on this. I'll try to do so this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants