You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2020/10/05 11:30:28 UTC
[GitHub] [qpid-proton] jiridanek opened a new pull request #271: PROTON-2170: allow the user to disable tests
jiridanek opened a new pull request #271:
URL: https://github.com/apache/qpid-proton/pull/271
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek commented on pull request #271: PROTON-2170: Respect the BUILD_TESTING option to allow the user to disable tests
Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #271:
URL: https://github.com/apache/qpid-proton/pull/271#issuecomment-709900061
With this patch, `-DBUILD_TESTING=OFF` is to have implicit effects in CTests, that is, `enable_testing()` is not called, and all `add_test` calls in CMake become noops. (This is because we now don't call `enable_testing()` ourselves and rely on CTest to call it when `BUILD_TESTING=ON`, which is the default.)
Futhermore, when `BUILD_TESTING=OFF`, compilation of test targets is skipped. This is the main goal of this patch.
There is a possible alternative implementation; conditionally set the `EXCLUDE_FROM_ALL` property on the test targets, https://cmake.org/cmake/help/v3.14/prop_tgt/EXCLUDE_FROM_ALL.html, instead of conditionally skipping part of the CMake file with `if`. This is something I did not try.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek commented on pull request #271: PROTON-2170: allow the user to disable tests
Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #271:
URL: https://github.com/apache/qpid-proton/pull/271#issuecomment-703830429
The `BUILD_TESTING` option is a standard CMake/CTest feature. A well-behaved project should work with it, not against it. https://cmake.org/cmake/help/v3.7/module/CTest.html
By default, it is `BUILD_TESTING=ON`. The ability to turn it off is there for experienced users who have their reasons, know what they are doing, and appreciate this level of control over the build.
Maven has a similar set of features. When running `mvn package`, it is possible to skip running tests, skip building tests altogether (maven doc discourages doing that, but it is possible), skip the RAT check, and skip building javadoc.
I am all for keeping `BUILD_TESTING=ON` by default. But setting it to OFF should actually work, that is, stop building the tests. Currently, the project alwaysbuilds and runs the tests, irrespective of this setting.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek merged pull request #271: PROTON-2170: Respect the BUILD_TESTING option to allow the user to disable tests
Posted by GitBox <gi...@apache.org>.
jiridanek merged pull request #271:
URL: https://github.com/apache/qpid-proton/pull/271
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek commented on pull request #271: PROTON-2170: allow the user to disable tests
Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #271:
URL: https://github.com/apache/qpid-proton/pull/271#issuecomment-703583299
@astitcher @ffontaine One open question. Do we want to call `enable_testing()` in top level CMakeLists.txt? That would have the effect of registering tests that don't require building any artifacts. Meaning that `-DBUILD_TESTING=OFF` does not turn off all testing, only that which prolongs the compilation step.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek commented on pull request #271: PROTON-2170: allow the user to disable tests
Posted by GitBox <gi...@apache.org>.
jiridanek commented on pull request #271:
URL: https://github.com/apache/qpid-proton/pull/271#issuecomment-703576119
I've modified the patch submitted to Jira, because it is actually not necessary to touch Proton bindings which do not build anything extra to enable testing, that is, Python, Ruby, and Go.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
[GitHub] [qpid-proton] jiridanek commented on a change in pull request #271: PROTON-2170: Respect the BUILD_TESTING option to allow the user to disable tests
Posted by GitBox <gi...@apache.org>.
jiridanek commented on a change in pull request #271:
URL: https://github.com/apache/qpid-proton/pull/271#discussion_r502779168
##########
File path: cpp/CMakeLists.txt
##########
@@ -231,90 +231,92 @@ install (FILES
${CMAKE_CURRENT_BINARY_DIR}/ProtonCppConfigVersion.cmake
DESTINATION ${LIB_INSTALL_DIR}/cmake/ProtonCpp)
-set(testdata "${CMAKE_CURRENT_BINARY_DIR}/testdata")
-set(test_env "")
-
-# SASL configuration for tests
-if(CyrusSASL_Saslpasswd_EXECUTABLE)
- configure_file("${CMAKE_CURRENT_SOURCE_DIR}/testdata/sasl-conf/proton-server.conf.in"
- "${testdata}/sasl-conf/proton-server.conf")
- execute_process(
- COMMAND echo password
- COMMAND ${CyrusSASL_Saslpasswd_EXECUTABLE} -c -p -f "${testdata}/sasl-conf/proton.sasldb" -u proton user
- RESULT_VARIABLE ret)
- if(ret)
- message(WARNING "${CyrusSASL_Saslpasswd_EXECUTABLE}: error ${ret} - some SASL tests will be skipped")
- else()
- set(test_env ${test_env} "PN_SASL_CONFIG_PATH=${testdata}/sasl-conf")
+if (BUILD_TESTING)
+ set(testdata "${CMAKE_CURRENT_BINARY_DIR}/testdata")
Review comment:
Adding one level of indentation to these multiple screens of code looks undesirable. It would be best to factor out all this into its own `tests.cmake` and conditionally include it here. I'll have to try if/how this works in CMake.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org