Add a cmake option for CrossSection #888
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #888 +/- ##
==========================================
- Coverage 91.84% 87.17% -4.67%
==========================================
Files 37 66 +29
Lines 4976 9430 +4454
Branches 0 1033 +1033
==========================================
+ Hits 4570 8221 +3651
- Misses 406 1209 +803 ☔ View full report in Codecov by Sentry. |
|
Tests pass now. |
CMakeLists.txt
Outdated
| option(MANIFOLD_EXCEPTIONS "Build manifold with exception enabled" ON) | ||
| option(BUILD_SHARED_LIBS "Build shared library" ON) | ||
| include(CMakeDependentOption) | ||
| cmake_dependent_option(MANIFOLD_TEST "Enable testing suite" ON "MANIFOLD_CROSS_SECTION" OFF) |
There was a problem hiding this comment.
This is new cmake syntax to me. What does it mean? It looks like cross section defaults to off, but I assume I'm wrong?
There was a problem hiding this comment.
This means MANIFOLD_TEST is only enabled when MANIFOLD_CROSS_SECTION is ON. I think this is a bit too strong.
There was a problem hiding this comment.
Okay, I can see why that was chosen though. I wonder how many #ifdefs we'll have to add to get it to run? May be a good idea to rearrange the tests a bit to put anything requiring CrossSection near each other.
|
I think we can run most of our tests when |
|
Updated to allow compatible tests to run. |
test/CMakeLists.txt
Outdated
| set(SOURCE_FILES test_main.cpp polygon_test.cpp sdf_test.cpp boolean_test.cpp) | ||
|
|
||
| if(MANIFOLD_CROSS_SECTION) | ||
| list(APPEND SOURCE_FILES boolean_complex_test.cpp cross_section_test.cpp hull_test.cpp manifold_test.cpp properties_test.cpp samples_test.cpp smooth_test.cpp) |
There was a problem hiding this comment.
How many of the tests in these files actually require cross section? I can see skipping cross_section_test.cpp, but it feels a bit like throwing the baby out with the bathwater to remove all of these. Can we just #ifdef around a few tests in each file?
|
Added the ifdef's, and a CI job to test without CrossSection (had to split the Python tests into a separate step) - 232 tests out of 250 are still run. (CI job name updated too to try and make the different runs clear) |
|
Oops, @cjmayo it looks like we missed something. We were running 261 tests before, but from this PR on we're only running 250, even with |
|
Ah, we need to pass the definition to c++. https://cmake.org/cmake/help/latest/command/target_compile_definitions.html |
On by default.