Repository navigation
Replace OpenSimAddTests macro with equivalent OpenSimAddTest for individual tests - #4466
nickbianco wants to merge 2 commits into
Conversation
787792d to
813a9e3
Compare
|
@aymanhab would you be able to review this one? |
aymanhab
left a comment
There was a problem hiding this comment.
Just had a few questions, clarifications but looks great. Nice work @nickbianco
@aymanhab reviewed 26 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on nickbianco).
Applications/CMC/tests/CMakeLists.txt line 8 at r1 (raw file):
resources/*.xml resources/*.sto resources/*.mot
Do we really need to keep around these vtp files?
cmake/OpenSimMacros.cmake line 466 at r1 (raw file):
# Add the test. add_test(NAME ${OSIMTEST_NAME} COMMAND ${OSIMTEST_NAME} ${test_args})
At some point we used symlink to avoid copying resource files, is that still the behavior? If not, are there any noticeable effects on time or space to run the tests?
cmake/OpenSimMacros.cmake line 476 at r1 (raw file):
ENVIRONMENT ${OSIMTEST_ENVIRONMENT}) # Copy test resources.
Do we know if any of these commented out flags were actually used? Should we keep these options easy to toggle?
OpenSim/Common/tests/CMakeLists.txt line 22 at r1 (raw file):
if(WITH_EZC3D) OpenSimAddTest(NAME testC3DFileAdapter LINKLIBS ${OSIM_COMMON_TEST_LIBS}
Glad to see this reversed 👍
OpenSim/Simulation/SimbodyEngine/tests/CMakeLists.txt line 7 at r1 (raw file):
LINKLIBS osimSimulation osimCommon osimAnalyses RESOURCES resources/testJointConstraints.osim )
May not be related but I'd think tests under Simulation library would not need to link osimAnalyses
nickbianco
left a comment
There was a problem hiding this comment.
Thanks @aymanhab! I've addressed all comments.
@nickbianco partially reviewed 26 files and made 6 comments.
Reviewable status: 25 of 26 files reviewed, 5 unresolved discussions (waiting on aymanhab).
Applications/CMC/tests/CMakeLists.txt line 8 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Do we really need to keep around these vtp files?
Probably not. I think it would make sense to defer picking out unnecessary files to later PRs. This change should make it easier to do that incrementally.
cmake/OpenSimMacros.cmake line 466 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
At some point we used symlink to avoid copying resource files, is that still the behavior? If not, are there any noticeable effects on time or space to run the tests?
Ah, looks like I lost that behavior in this edit. Adding it back.
cmake/OpenSimMacros.cmake line 476 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Do we know if any of these commented out flags were actually used? Should we keep these options easy to toggle?
I don't know if they were used, but I'm inclined to remove them if they were commented out.
OpenSim/Common/tests/CMakeLists.txt line 22 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
Glad to see this reversed 👍
Done.
OpenSim/Simulation/SimbodyEngine/tests/CMakeLists.txt line 7 at r1 (raw file):
Previously, aymanhab (Ayman Habib) wrote…
May not be related but I'd think tests under Simulation library would not need to link osimAnalyses
This was the case before these changes. I think this one could also be fixed in a follow up PR.
Fixes issue #4441
Brief summary of changes
The testing macro
OpenSimAddTestshas been changed toOpenSimAddTest, a mostly equivalent macro for adding a single test target at a time. This change fixes #4441, since now test resources are explicitly assigned to each test target. This also makes it easier to identify which resources are associated with each test just be looking at each test directory's CMakeLists file (rather than parsing through everything individual test).OpenSimAddTestautomatically links toosimTestingandCatch2::Catch2WithMain; other libraries can be linked to via theLINKLIBSargument, as before. Other arguments includeRESOURCES, to specify test resources,EXTRA_SOURCESto specify any related.hor.cppfiles,ENVIRONMENTto provide test-specific environment variables, andDISABLEDto indicate to CTest to skip the test when running the test suite.Other changes include:
MocoAddTestinOpenSim/Moco/tests/CMakeLists.txthas been replaced withOpenSimAddTest.testContext.cpphas been converted to the Catch2 framework, which is necessary since the new macro automatically links toosimTestingandCatch2::Catch2WithMain.Testing I've completed
Ran tests locally; CI.
Looking for feedback on...
CHANGELOG.md (choose one)
This change is