Skip to content

Conversation

@aabrown100-git
Copy link
Collaborator

@aabrown100-git aabrown100-git commented Jul 10, 2025

Current situation

Resolves #338

Release Notes

  • Split up code originally in unitTests/test.h and unitTests/test.cpp into separate files. These contained tests for different material models. I create separate test_material_xxx.h and test_material_xxx.cpp files for each material model.
  • Put all material model tests into a subfolder material_model_tests
  • Added README.md describing unit tests
  • Modified CMakeLists.txt to automatically find all *.cpp files under unitTests/ and compile and run them when unit testing is performed.

Documentation

  • README.md provides documentation for unit testing

Testing

  • Verified that all original tests (127 tests from 16 test suites) in test.cpp were copied into the new test_material_xxx.cpp files.

Code of Conduct & Contributing Guidelines

This comment was marked as outdated.

@aabrown100-git aabrown100-git requested a review from Copilot July 10, 2025 23:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reorganizes the existing monolithic unit test into per-material-model files with shared test utilities, and updates the build to auto-discover new tests.

  • Extracted common mocks and helpers into tests/unitTests/test_common.h
  • Refactored test_material_common.h to include shared helpers and made utility functions inline
  • Split unitTests/test.cpp into multiple test_material_<model>.h/.cpp pairs under material_model_tests/

Reviewed Changes

Copilot reviewed 22 out of 23 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/unitTests/test_common.h New common mocks and base TestBase class for all tests
tests/unitTests/material_model_tests/test_material_common.h Refactored common test helpers, moved implementations inline
tests/unitTests/material_model_tests/test_material_simo_taylor_91_volumetric_penalty.h New header for Simo-Taylor91 volumetric penalty model tests
tests/unitTests/material_model_tests/test_material_simo_taylor_91_volumetric_penalty.cpp New test fixture and cases for ST91 volumetric penalty
tests/unitTests/material_model_tests/test_material_quadratic_volumetric_penalty.h New header for quadratic volumetric penalty model tests
tests/unitTests/material_model_tests/test_material_quadratic_volumetric_penalty.cpp New test fixture and cases for quadratic volumetric penalty
tests/unitTests/material_model_tests/test_material_neohookean.h New header for Neo-Hookean model tests
tests/unitTests/material_model_tests/test_material_neohookean.cpp New Neo-Hookean test fixture and cases
tests/unitTests/material_model_tests/test_material_mooney_rivlin.h New header for Mooney-Rivlin model tests
tests/unitTests/material_model_tests/test_material_mooney_rivlin.cpp New Mooney-Rivlin test fixture and cases
tests/unitTests/material_model_tests/test_material_miehe_94_volumetric_penalty.h New header for Miehe94 volumetric penalty model tests
tests/unitTests/material_model_tests/test_material_miehe_94_volumetric_penalty.cpp New Miehe94 test fixture and cases
tests/unitTests/material_model_tests/test_material_holzapfel_ogden_MA.h New header for Holzapfel-Ogden MA model tests
tests/unitTests/material_model_tests/test_material_holzapfel_ogden_MA.cpp New Holzapfel-Ogden MA test fixture and cases
tests/unitTests/material_model_tests/test_material_holzapfel_ogden.h New header for Holzapfel-Ogden model tests
tests/unitTests/material_model_tests/test_material_holzapfel_ogden.cpp New Holzapfel-Ogden test fixture and cases
Comments suppressed due to low confidence (4)

tests/unitTests/test_common.h:54

  • [nitpick] The class name 'MockdmnType' mixes casing; for consistency, consider renaming to 'MockDmnType' following CamelCase conventions.
class MockdmnType : public dmnType {

tests/unitTests/material_model_tests/test_material_common.h:1793

  • The signature of convertToArray passes both parameters by value and returns void, yet includes a return statement. It should be changed to void convertToArray(const Array3x3 &stdArray, Array &F), remove the return, and write directly into F.
    void convertToArray(Array3x3 stdArray, Array<double> F) {

tests/unitTests/material_model_tests/test_material_common.h:1777

  • convertToStdArray uses assert() but the file does not include . Please add #include to ensure assert is declared.
    Array3x3 convertToStdArray(const Array<double> &F) {

tests/unitTests/material_model_tests/test_material_common.h:280

  • computeLinearRegression uses std::accumulate but the header is not included. Please add #include at the top to avoid compilation errors.
inline std::pair<double, double> computeLinearRegression(const std::vector<double>& x, const std::vector<double>& y) {

Copy link
Collaborator

@ktbolt ktbolt left a comment

Choose a reason for hiding this comment

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

@aabrown100-git Very good work !

@ktbolt
Copy link
Collaborator

ktbolt commented Jul 11, 2025

@aabrown100-git A few general comments about the unit tests.

  1. I would be good to see more comments describing what the tests are doing and why, and any assumptions, and what criteria is used to pass or fail the test.

  2. The tolerances used to pass a test should be constants set in a visible location, like at the top of the class.

  3. Function and variable names do not conform to our coding standards: they should use snake case.

  4. Function names are much too long (e.g. TestPK2StressConvergenceOrderTriaxialStretch)

  5. There are lots of useless comments

    // Loop over F in F_small_list
    for (auto F_std : F_small_list) {
        // Convert to Array
        convertToArray(F_std, F);
  1. Why is convertToArray need to be used ?

@ktbolt
Copy link
Collaborator

ktbolt commented Jul 11, 2025

@aabrown100-git Don't worry about making changes for

3. Function and variable names do not conform to our coding standards: they should use snake case.

4. Function names are much too long (e.g. TestPK2StressConvergenceOrderTriaxialStretch)

you've spent a good amount of time on this and have done a great job !

…al tests to use Vector class for improved clarity and performance. Refactor convertToArray() to use const reference. Per comment by @ktbolt
…of std:Arrays, avoiding conversion between the two. Also, adjusted the tolerances on some tests to get them to pass. Also, use const references when accessing elements of F_small_list, for example
@aabrown100-git
Copy link
Collaborator Author

Thanks @ktbolt. I've addressed most of your comments, will continue this weekend.

@aabrown100-git
Copy link
Collaborator Author

@ktbolt I think I've addressed all your comments. Let me know if there's anything else I should change!

Copy link
Collaborator

@ktbolt ktbolt left a comment

Choose a reason for hiding this comment

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

Looks good !

@ktbolt ktbolt merged commit 553849b into SimVascular:main Jul 15, 2025
4 checks passed
@aabrown100-git aabrown100-git deleted the reorg_unit_tests branch July 17, 2025 22:26
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.

Split up unit tests

3 participants