[test] Make all tests green when using a minimal build #20711
Conversation
|
We'll see the tests, however the formatting errors reflect the original formatting and might not be really worth fixing (at the cost of changing the formatting of stable tests) |
|
A bit related: maybe we should also add a cmake error if minimal=ON but X11/asimage=ON, see https://root-forum.cern.ch/t/cant-built-minimal-root-6-38-00-with-x11-and-asimage/64532 |
Test Results 21 files 21 suites 3d 17h 0m 0s ⏱️ Results for commit 94c0e70. ♻️ This comment has been updated with latest results. |
dpiparo
force-pushed
the
minimal_tests
branch
from
1eae4b8 to
4cb08d6
Compare
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
Thanks for the initiative! I would just suggest to not introduce a public R__IS_MINIMAL macro at this point.
core/rint/test/TTabComTests.cxx
Outdated
| " TH1DModel" | ||
| #endif | ||
| " TH1Editor TH1F TH1I TH1L TH1S"; | ||
| #ifndef R__IS_MINIMAL |
There was a problem hiding this comment.
| #ifndef R__IS_MINIMAL | |
| #if not defined(ROOT_IS_MINIMAL) |
I would prefer to not add a ROOT runtime preprocessor macro R__IS_MINIMAL that is also visible to the user if there is no strong reason for it. Because then we need to keep it in principle forever for backwards compatibility, locking ourselves in without a strong reason. Instead, one can add a
if (minimal)
target_compile_definitions(TTabComTests PRIVATE ROOT_IS_MINIMAL)
endif()to the CMakeLists.txt. The runtime R__ flags only need to be defined if we need to do these conditionals also in the header files included by the users, but here it's just for a test.
There was a problem hiding this comment.
This is a good suggestion. I stopped the builds and will change that part of the PR.
There was a problem hiding this comment.
I would just suggest to not introduce a public R__IS_MINIMAL macro at this point.
I disagree. The fact that the test are failing under the minimal build is only a secondary symptoms. The real issue is that the tests were failing because a feature they depend on was turned off and they were not reacting properly to that turning off. The problem would still have been there if the users explicitly disable 'just' those feature.
There was a problem hiding this comment.
I don't understand how the R__IS_MINIMAL macro would sustainably address the issue you describe.
If it's useful for the user to check if ROOT has a certain feature, we should introduce R__HAS_FEATURE macros like we always did.
The R__IS_MINIMAL would be mute, because one can toggle any an feature on top of a minimal build. Why would you use it then?
dpiparo
force-pushed
the
minimal_tests
branch
from
4cb08d6 to
94c0e70
Compare
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
Thank you, very good! I see you just disable the TTabComTests test now for minimal=ON. That's indeed even simpler than making the test conditional on minimal=ON with preprocessor macros.
|
Thanks for the advice! |
| # For the list of contributors see $ROOTSYS/README/CREDITS. | ||
|
|
||
| ROOT_ADD_GTEST(TTabComTests TTabComTests.cxx LIBRARIES Rint) | ||
| if(NOT minimal) |
There was a problem hiding this comment.
Is that really the best test? What 'feature(s)' is missing that make the test not working?
| @@ -89,4 +89,9 @@ ROOTTEST_ADD_TEST(hello_ROOT_C | |||
| OUTREF hello_ROOT_C_${libsuf}.ref | |||
| FIXTURES_REQUIRED root-meta-hello_ROOT-fixture) | |||
|
|
|||
| if(NOT minimal) | |||
There was a problem hiding this comment.
Is that really the best test? What 'feature(s)' is missing that make the test not working?
| @@ -440,6 +423,12 @@ else() | |||
| list(APPEND root7_veto io/ntuple/ntpl011_global_temperatures.C) | |||
| endif() | |||
|
|
|||
| #---These do not run with minimal | |||
| if(minimal) | |||
There was a problem hiding this comment.
Is that really the best test? What 'feature' is missing that make the test not working?
4 participants
Adapt the tests in order to make them run properly with minimal builds or disable them if they do not make sense for that configuration.
Fixes #20315