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

Final pytest migration #4443

Merged
merged 11 commits into from
Sep 28, 2024

Conversation

prady0t
Copy link
Contributor

@prady0t prady0t commented Sep 16, 2024

WIP

Signed-off-by: Pradyot Ranjan <[email protected]>
@prady0t
Copy link
Contributor Author

prady0t commented Sep 16, 2024

Just two files left
test_idaklu_jax.py : remove parameterized dependency.
test_solution.py : problem with converting assertLogs to pytest

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.46%. Comparing base (62a7ee8) to head (5345162).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4443   +/-   ##
========================================
  Coverage    99.46%   99.46%           
========================================
  Files          293      293           
  Lines        22332    22332           
========================================
  Hits         22212    22212           
  Misses         120      120           

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

@agriyakhetarpal
Copy link
Member

Unfortunately, there's no standard way to replace self.assertLogs() as far as I could find. I think the best way is to hack into PyBaMM's logger and retrieve the result you want, which is okay since we have just one test that uses this. For example,

import io
import logging
log_capture = io.StringIO()
handler = logging.StreamHandler(log_capture)
handler.setLevel(logging.ERROR)
logger = logging.getLogger('pybamm.logger')
logger.addHandler(handler)

and then, if you run the code that should produce the logger warning: pybamm.Solution(ts, bad_ys, model, {}), you can check the output using log_capture.getvalue() and check if "exceeds the maximum" string is in there. Later, you can destroy the handler using logger.removeHandler(handler) and clean it up, so that it doesn't affect any other tests. This worked for me as a quick experiment, could you try and see how it holds up in the test suite?

@agriyakhetarpal
Copy link
Member

@prady0t, how is this going? The tests are all already passing here, and the approach I suggested should be okay in general – it works on my machine. Please let me know if you need additional help with this.

@prady0t prady0t marked this pull request as ready for review September 26, 2024 21:16
@prady0t
Copy link
Contributor Author

prady0t commented Sep 26, 2024

Hi. I've updated the files. Lets wait for CI to run.

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.

Thanks, @prady0t – I guess the next step to do after this PR should be to disallow the use of unittest through the Ruff rule as we discussed earlier

Edit: I notice some failing tests with the parameterisation, could you try fixing them locally?

pyproject.toml Show resolved Hide resolved
Signed-off-by: Pradyot Ranjan <[email protected]>
@prady0t
Copy link
Contributor Author

prady0t commented Sep 28, 2024

@agriyakhetarpal I've updated this PR.

Signed-off-by: Pradyot Ranjan <[email protected]>
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.

Thanks for completing the migration, @prady0t!

@agriyakhetarpal agriyakhetarpal merged commit b6fa67a into pybamm-team:develop Sep 28, 2024
26 checks passed
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.

2 participants