Unify and simplify c++ tests #488
Conversation
mattelser
requested review from
lilleyse,
weegeekps,
r-veenstra and
corybarr
as code owners
There was a problem hiding this comment.
Very nice, great to see the doctest results again
[doctest] doctest version is "2.4.9"
[doctest] run with "--help" for options
===============================================================================
[doctest] test cases: 11 | 11 passed | 0 failed | 0 skipped
[doctest] assertions: 1278 | 1278 passed | 0 failed |
[doctest] Status: SUCCESS!
tests/include/testUtils.h
Outdated
There was a problem hiding this comment.
It looks like this file got accidentally checked in. Could you remove it and update .gitignore with the updated path?
| @@ -325,7 +324,7 @@ if(CESIUM_OMNI_ENABLE_COVERAGE) | |||
| -DPROJECT_BUILD_DIRECTORY=${PROJECT_BINARY_DIR} | |||
| -DPROJECT_SOURCE_DIRECTORIES=${COVERAGE_SOURCE_DIRECTORIES_FORMATTED} | |||
| -DOUTPUT_DIRECTORY=${PROJECT_BINARY_DIR}/coverage -P ${PROJECT_SOURCE_DIR}/cmake/GenerateCoverage.cmake | |||
| DEPENDS $<TARGET_FILE:tests> | |||
| DEPENDS $<TARGET_FILE:cesium.omniverse.cpp.tests.plugin> | |||
There was a problem hiding this comment.
I would be surprised if this alone fixes coverage, but maybe worth running coverage just to check?
There was a problem hiding this comment.
confirmed it doesn't magically fix coverage (it finds no tests), but I figured it was worth updating since there is no longer a tests binary
There was a problem hiding this comment.
Could you remove this section from main.yml
- name: Tests
if: matrix.config.name != 'CentOS 7 - GCC'
run: ctest --test-dir build --output-on-failure
Understandably, right now it's printing:
Run ctest --test-dir build --output-on-failure
Internal ctest changing into directory: C:/a/cesium-omniverse/cesium-omniverse/build
Test project C:/a/cesium-omniverse/cesium-omniverse/build
No tests were found!!!
There was a problem hiding this comment.
Could you also remove the "Test" task in task.json and the associated testing code in vscode_build.py, and the documentation about that in developer-setup/README.md?
There was a problem hiding this comment.
Could you rename tests/public to tests/src?
The reason for separating the main extension code into bindings, core, plugins, and public was because each represented its own library. But since the test code is a single library it makes sense to go with a more generic folder name like src.
tests/public/testUtils.h.in
Outdated
There was a problem hiding this comment.
It could make sense to move testUtils.h.in to the include folder since that's where testUtils.h will get created
There was a problem hiding this comment.
I forgot to mention this in the other PR, but in the launch.json files could you move the Tests Extension configuration either after or before the python configurations? That way it isn't sandwiched between them awkwardly.
| include(Macros) | ||
|
|
||
| glob_files(SOURCES "${CMAKE_CURRENT_LIST_DIR}/*.cpp") | ||
|
|
||
| get_property(ADDITIONAL_LIBRARIES GLOBAL PROPERTY NVIDIA_ADDITIONAL_LIBRARIES_PROPERTY) | ||
|
|
There was a problem hiding this comment.
You should be able to remove these lines
|
Right... automerge kicked in since I approved... could you open up a follow up PR for #488 (review)? |
Fixes #486 Fixes #487
We currently have c++ tests separate from the tests extension. These tests were built first and had a straightforward doctest setup since they didn't have any dependencies on Omniverse. In building the tests extension, I tried this straightforward approach and ran into problems*, so I opted to use doctest's
CHECKs in a standalone configuration. This didn't buy us nearly as much functionality, so I tried the straightforward approach again when merging the old tests into the new extension. This time it came together, which simplifies the testing setup, puts it more in line with a standard doctest approach, and provides more functionality! 🎉*I was unable to create a test suite/case because I was trying to do so within the interface object used in the python bindings, which was an illegal attempt to statically define a function within a method (or something like that). I tried defining the function outside the object, but ran into similar trouble. For one reason or another I stopped short of the proper solution, which this PR contains.