-
Notifications
You must be signed in to change notification settings - Fork 324
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
base: main
Are you sure you want to change the base?
Convert Simulation tests to catch2 #3897
Conversation
There was a problem hiding this 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 SECTION
s and remove the cout
statements. When running the test suite, the sections will show as passed or failed.
Also, watch the 80 column limit.
There was a problem hiding this 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
SECTION
s and remove thecout
statements. When running the test suite, the sections will show as passed or failed.Also, watch the 80 column limit.
Done.
I need to add LoadOpenSimLibrary("osimActuators"); back to some files |
There was a problem hiding this 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.
@AllisonJohn let me know if you wanted to finish up this PR or if you want me to take it over. |
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)
This change is