Page MenuHome

Tests: Disable tests for non-compiled libraries
ClosedPublic

Authored by Himanshi Kalra (calra) on Sep 9 2021, 9:50 PM.

Details

Summary

This diff disables tests for Boolean, subdivision surface and volume
when GMP, Opensubdiv and Openvdb are not compiled respectively.
It also changes the existing file structure and adds sub-folders for
boolean and subdivison tests. The volume folder only has one test and
is as unchanged structure-wise.

Here is the zip file with newer structure.

Diff Detail

Repository
rB Blender
Branch
disable_boolean_no_gmp (branched from master)
Build Status
Buildable 16909
Build 16909: arc lint + arc unit

Event Timeline

Himanshi Kalra (calra) requested review of this revision.Sep 9 2021, 9:50 PM
Himanshi Kalra (calra) created this revision.

All code snippets I "winged" cmake is a little picky sometimes it wants ${var} other times var on its own is enough, you may have to play with it a little.

tests/python/CMakeLists.txt
758

missing identation

765

Use On and Off (no quotes needed) rather than "1" and "0"

768

rather than a loop do something like

if (filename IN_LIST gmp_skip_tests)

775

rather see a

if(NOT DISABLE_BOOLEAN_TEST)
   add_blender_test(.....)
endif()
than mess about with continue
Ray Molenkamp (LazyDodo) requested changes to this revision.Sep 9 2021, 10:15 PM
This revision now requires changes to proceed.Sep 9 2021, 10:15 PM

This doesn't scale well. A new dependency like OpenVDB/NanoVDB for volume would need an exception and its own list of tests & variables.
Instead of globbing all files under a folder, suggest doing how its done in source code. Say WITH_USD controls if usd code files are built or not.
Same for testing, tests that need GMP could be in a separate folder, with its CMakeLists.txt globbing the blend files there. Or listing manually if finer control is needed, like having another dependency alongside GMP.

I like @Ankit Meel (ankitm) 's idea

A more elegant change by sticking the two problematic files into tests\modeling\geometry_nodes\mesh\boolean\ rather than tests\modeling\geometry_nodes\mesh and having something like

diff --git a/tests/python/CMakeLists.txt b/tests/python/CMakeLists.txt
index a1b94abc317..fa7c343a053 100644
--- a/tests/python/CMakeLists.txt
+++ b/tests/python/CMakeLists.txt
@@ -751,9 +751,12 @@ set(geo_node_tests
   utilities
   vector
   volume
-
 )

+if(WITH_GMP)
+  list(APPEND geo_node_tests mesh/boolean)
+endif()
+
 foreach(geo_node_test ${geo_node_tests})
   if(EXISTS "${TEST_SRC_DIR}/modeling/geometry_nodes/${geo_node_test}/")
     file(GLOB files "${TEST_SRC_DIR}/modeling/geometry_nodes/${geo_node_test}/*.blend")

would probably do the trick nicely

Had a chat with Ray and agreed upon not putting CMake files in SVN as my last comment hinted. People don't have the SVN folder open in their editor in case something in git changes.

Himanshi Kalra (calra) updated this revision to Diff 42195.EditedSep 21 2021, 8:42 PM
  • Merge branch 'master' into disable_boolean_no_gmp
  • Tests: Disables tests for non-compiled libraries.

Addressed comments by Ray and Ankit.

Himanshi Kalra (calra) edited the summary of this revision. (Show Details)Sep 21 2021, 8:47 PM
Himanshi Kalra (calra) retitled this revision from Tests: Disable boolean tests when compiled without GMP to Tests: Disable tests for non-compiled libraries.
Himanshi Kalra (calra) edited the summary of this revision. (Show Details)
  • Minor clenaup: Formatting and removed extra comment.
Ray Molenkamp (LazyDodo) requested changes to this revision.Sep 21 2021, 9:26 PM

Small tweaks to the text and I'd like the order swapped, code is easier to write than to read so anything we can do in that department is appreciated

if(WITH_OPENSUBDIV)
  list(APPEND geo_node_tests mesh/subdivision_tests)
else()
 MESSAGE(STATUS "Disabling mesh/subdivision_tests because WITH_OPENSUBDIV is off.")
endif()

is easier to read/understand than

if(NOT WITH_OPENSUBDIV)
 MESSAGE(STATUS "Disabling mesh/subdivision_tests because WITH_OPENSUBDIV is off.")
else()
  list(APPEND geo_node_tests mesh/subdivision_tests)
endif()
tests/python/CMakeLists.txt
762
768
774
This revision now requires changes to proceed.Sep 21 2021, 9:26 PM
Himanshi Kalra (calra) marked 3 inline comments as done.
  • Addressed comments by Ray.

Improve code readability, updated error message for clarity.

This revision is now accepted and ready to land.Sep 27 2021, 8:36 PM