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

Convert Simulation tests to catch2 #3897

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

AllisonJohn
Copy link
Member

@AllisonJohn AllisonJohn commented Aug 30, 2024

Fixes issue #3555

Brief summary of changes

Converted remaining test files in the simulation folder to use catch test cases

Testing I've completed

tests still pass

Looking for feedback on...

CHANGELOG.md (choose one)

  • no need to update because internal

This change is Reviewable

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

A couple minor suggestions, but looks good overall!

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AllisonJohn)


OpenSim/Simulation/Test/testInverseKinematicsSolver.cpp line 243 at r1 (raw file):

            noiseLevel*double(noise(gen)), SimTK::ZAxis );

        cout << "!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!" << endl;

Remove?


OpenSim/Simulation/Test/testMomentArms.cpp line 348 at r1 (raw file):

        SimTK::Vec2(-2*SimTK::Pi/3, SimTK::Pi/18), 0.0,
        "VASINT of BothLegs with no mass: FAILED");
    cout << "VASINT of BothLegs with no mass: PASSED\n" << endl;

It would be better to remove all these subtests into SECTIONs and remove the cout statements. When running the test suite, the sections will show as passed or failed.

Also, watch the 80 column limit.

Copy link
Member Author

@AllisonJohn AllisonJohn left a comment

Choose a reason for hiding this comment

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

Reviewable status: 10 of 12 files reviewed, 2 unresolved discussions (waiting on @nickbianco)


OpenSim/Simulation/Test/testInverseKinematicsSolver.cpp line 243 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

Remove?

Done.


OpenSim/Simulation/Test/testMomentArms.cpp line 348 at r1 (raw file):

Previously, nickbianco (Nick Bianco) wrote…

It would be better to remove all these subtests into SECTIONs and remove the cout statements. When running the test suite, the sections will show as passed or failed.

Also, watch the 80 column limit.

Done.

@AllisonJohn
Copy link
Member Author

I need to add LoadOpenSimLibrary("osimActuators"); back to some files

Copy link
Member

@nickbianco nickbianco left a comment

Choose a reason for hiding this comment

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

A few more suggestions to clean things up.

Reviewed 2 of 2 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @AllisonJohn)


OpenSim/Simulation/Test/testModelInterface.cpp line 38 at r3 (raw file):

TEST_CASE("testModelFinalizePropertiesAndConnections")
{
    LoadOpenSimLibrary("osimActuators");

Replace with include?


OpenSim/Simulation/Test/testMomentArms.cpp line 347 at r3 (raw file):

        testMomentArmDefinitionForModel("BothLegs22.osim", "r_knee_angle", "VASINT",
            SimTK::Vec2(-2*SimTK::Pi/3, SimTK::Pi/18), 0.0,
            "VASINT of BothLegs with no mass: FAILED");

Similar to removing the cout statements, we should remove the need for an error message from testMomentArmDefinitionForModel since the Catch framework will tell us if the subtest passed or failed.


OpenSim/Simulation/Test/testStatesTrajectory.cpp line 266 at r3 (raw file):

TEST_CASE("testPopulateTrajectoryAndStatesTrajectoryReporter") {
    LoadOpenSimLibrary("osimActuators");

If every test needs this library, we should just include it above rather than loading it for every test.

@nickbianco
Copy link
Member

@AllisonJohn let me know if you wanted to finish up this PR or if you want me to take it over.

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