[cmake-developers] [PATCH v6 0/2] Add XCTest Bundle Support

[Brad King]

On 02/25/2015 03:07 PM, Gregor Jasny wrote:
> Changes since v5:
> * Rebased against master (could drop two applied patches)
> * kept help modules list sorted
> * indirected xctest wiring

Great.  The tests pass on my machine with no special configuration.

The basic functionality is in good shape.  My remaining comments
are on the XCTestUtilities module that I hadn't fully reviewed
before:

* Please split addition of XCTestUtilities into its own commit
  between the current two with its own commit message.

* Please use the ".. command:: ..." explicit markup to document
  the module API.  That will make the functions indexed and
  linkable from other documentation.  See the ExternalData
  module documentation for an example.

* Please rename the APIs to start in "xctest_", e.g.
  add_xctest => xctest_add.  We like to keep module-provided
  APIs prefixed with something related to the module name.

>   if(NOT CMAKE_OSX_SYSROOT)
>     message(STATUS "Adding XCTest bundles requires CMAKE_OSX_SYSROOT to be set.")
>   endif()

Should this be a FATAL_ERROR?

>   get_target_property(TESTEE_TYPE ${testee} TYPE)
>   get_target_property(TESTEE_FRAMEWORK ${testee} FRAMEWORK)
>   get_target_property(TESTEE_MACOSX_BUNDLE ${testee} MACOSX_BUNDLE)

Please use get_property(... TARGET ${testee} ...) instead.  We
may one day deprecate the get_*_property commands because they
are so inconsistent in how they report undefined properties.

>   find_library(FOUNDATION_LIBRARY Foundation)
>   if(NOT FOUNDATION_LIBRARY)
>     message(STATUS "Could not find Foundation Framework.")
>   endif()
> 
>   find_library(XCTEST_LIBRARY XCTest)
>   if(NOT XCTEST_LIBRARY)
>     message(STATUS "Could not find XCTest Framework.")
>   endif()

Rather than polluting the cache with these, we could just trust
that they exist.  Is there any reason not to do:

  target_link_libraries(${target} PRIVATE
    "-framework Foundation" "-framework XCTest")

instead?  Let Xcode find its own frameworks.

Alternatively these should be something that the XCTestUtilities
module finds as part of include()ing it instead of calling
find_library again on every invocation of the API.

>   execute_process(
>     COMMAND xcrun --find xctest
>     OUTPUT_VARIABLE XCTEST_EXECUTABLE
>     OUTPUT_STRIP_TRAILING_WHITESPACE)

Use ERROR_VARIABLE here with a bogus local variable name just
to hide any stderr output from the user running CMake.  As
above one could pre-find this outside of any API call.

>   if(NOT XCTEST_EXECUTABLE)
>     message(STATUS "Unable to finc xctest binary.")
>   endif()

If you go with the pre-find approach then this can still be
in the function because we only need the value there.  However,
shouldn't this be an error too?

Thanks,
-Brad