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

Unit test 150 #198

Merged
merged 25 commits into from
Mar 14, 2024
Merged

Unit test 150 #198

merged 25 commits into from
Mar 14, 2024

Conversation

yuecheng-yu
Copy link
Collaborator

Current situation

Refer to #150

Release Notes

A unit testing program using GoogleTest to test get_pk2cc is implemented and built. It allows user defined deformation gradient as input to check the correctness of get_pk2cc (material model) by comparing the output stress and tangent tensors.

Documentation

None

Testing

To be tested

Code of Conduct & Contributing Guidelines

Copy link

codecov bot commented Feb 23, 2024

Codecov Report

Attention: Patch coverage is 97.01493% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 62.90%. Comparing base (b99e12e) to head (2bf104c).
Report is 6 commits behind head on main.

Files Patch % Lines
tests/unitTests/test.h 94.87% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #198      +/-   ##
==========================================
+ Coverage   57.50%   62.90%   +5.40%     
==========================================
  Files         103      105       +2     
  Lines       26964    27214     +250     
==========================================
+ Hits        15505    17119    +1614     
+ Misses      11459    10095    -1364     

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

@yuecheng-yu yuecheng-yu marked this pull request as ready for review February 26, 2024 21:46
@yuecheng-yu yuecheng-yu requested a review from mrp089 February 26, 2024 21:46
@mrp089
Copy link
Member

mrp089 commented Feb 26, 2024

@ktbolt, I'll look over this PR and after @yuecheng-yu and I have iterated over it, I'll assign you as a reviewer

Copy link
Member

@mrp089 mrp089 left a comment

Choose a reason for hiding this comment

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

Thank you for making the unit tests work, @yuecheng-yu! I added a couple of comments on how to clean up the code and make it more modular. Please have a look and let me know if you have any questions.

Code/Source/svFSI/CMakeLists.txt Outdated Show resolved Hide resolved
Code/Source/svFSI/CMakeLists.txt Outdated Show resolved Hide resolved
tests/unitTests/test.cpp Outdated Show resolved Hide resolved
tests/unitTests/test.cpp Outdated Show resolved Hide resolved
tests/unitTests/unitTest.xml Outdated Show resolved Hide resolved
tests/unitTests/test.cpp Outdated Show resolved Hide resolved
tests/unitTests/test.cpp Outdated Show resolved Hide resolved
tests/unitTests/test.cpp Outdated Show resolved Hide resolved
tests/unitTests/hello_test.cpp Outdated Show resolved Hide resolved
tests/unitTests/test.cpp Outdated Show resolved Hide resolved
@mrp089
Copy link
Member

mrp089 commented Mar 1, 2024

Very nice idea to use a fixture! Do you think we could use a Mock object for things like com_mod and dmn?

@yuecheng-yu
Copy link
Collaborator Author

Yes, I think it's possible. When I compared mock object with test fixture, I felt that while mock object is more ideal for unit testing, it may be harder to use for our current structures of material model. Because each type of model has completely different parameters/classes and also the parameters used in get_pk2cc such as C10/C01 are different from the input file (Elastic Modules, etc.), I think it could be a better timing to use Mock method for comprehensive unit testing after we have a new material model interface. Meanwhile, I personally really like the fixture thing which can rerun the setup code, which I think is the main reason I picked fixture to try first, please forget my previous blah blah. I'll try to write a GMock frame for nHK first to see how it works.

@ktbolt
Copy link
Collaborator

ktbolt commented Mar 1, 2024

@mrp089 @yuecheng-yu I feel that there is too much discussion here that is not included in the Issue. I think we should have code reviews before people do a PR.

@mrp089
Copy link
Member

mrp089 commented Mar 4, 2024

True, but unfortunately, creating a (draft) PR is necessary to get the code review function in GitHub (which nicely lets multiple people interact on the code). And it's better to have a discussion in the PR than not having a discussion. The discussion is still easily accessible since the merged commit is linked to the PR (and indirectly to the issue).

@yuecheng-yu
Copy link
Collaborator Author

@mrp089 I created an Isotropic material class for unit test, wrapped the mock class inside the test class. So the users only need to define material parameters, input deformation gradient, and reference solutions at the testing interface (test.cpp).

Copy link
Member

@mrp089 mrp089 left a comment

Choose a reason for hiding this comment

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

Nicely done, @yuecheng-yu! This is looking good, just some minor cleanups.

Code/Source/svFSI/mat_models.cpp Outdated Show resolved Hide resolved
tests/unitTests/mesh/unit_cube_volume.vtu Outdated Show resolved Hide resolved
tests/unitTests/test.cpp Outdated Show resolved Hide resolved
tests/unitTests/test.h Outdated Show resolved Hide resolved
tests/unitTests/test.h Outdated Show resolved Hide resolved
tests/unitTests/test.h Show resolved Hide resolved
tests/unitTests/test.cpp Outdated Show resolved Hide resolved
tests/unitTests/test.cpp Outdated Show resolved Hide resolved
tests/unitTests/unitTest.xml Outdated Show resolved Hide resolved
tests/unitTests/test.cpp Outdated Show resolved Hide resolved
tests/unitTests/test.cpp Outdated Show resolved Hide resolved
@mrp089 mrp089 merged commit b28958c into SimVascular:main Mar 14, 2024
7 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.

3 participants