CHORE: refacto maxwell test #5601
Conversation
|
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5601 +/- ##
==========================================
- Coverage 85.25% 81.44% -3.82%
==========================================
Files 152 152
Lines 61012 60939 -73
==========================================
- Hits 52015 49630 -2385
- Misses 8997 11309 +2312 |
For more information, see https://pre-commit.ci
… chore/refacto_maxwell_test
gmalinve
requested review from
jvela018,
maxcapodi78,
Alberto-DM,
Samuelopez-ansys and
MaxJPRey
There was a problem hiding this comment.
Thanks for this huge refactoring @gmalinve
I left some minor comments, note that the ones of Eddy current -> Eddy Current can probably be discarded (but check the removal of an unnecessary " ")
On top of those comments here are three things I want to bring up:
- In future refactoring / test addition (or even here if you want) could you try to make the tests follow the pattern : Setup, Calls, Asserts (and TearDown if required) ? This would help to understand what is tested. Note that this will also lead to split some test into more tests (but the code is already here so that isn't really an issue)
- Would it make sense add extra fixture to also embed
m3d_app.solution_type = SOLUTIONS.WHATEVER? I see that multiple tests start with such definition and I was wondering if it would make sense. - Could you try running the tests locally and use random testing ? For example, add
import pytest
import random
def pytest_collection_modifyitems(session, config, items):
random.shuffle(items)
in the test file or you can have a look at https://pypi.org/project/pytest-randomly/
|
@gmalinve Thanks again for this huge refactoring and bug fixes. I don't think I'll have other changes to propose so once my requests are handled, it should be ready to be merged :) |
Co-authored-by: Sébastien Morais <[email protected]>
Co-authored-by: Sébastien Morais <[email protected]>
Description
Please provide a brief description of the changes made in this pull request.
Issue linked
Please mention the issue number or describe the problem this pull request addresses.
Checklist