Add tests for ament_python_install_package

[rkent]

This creates tests for ament_python_package_install. It is created in anticipation of #587. That PR does an extensive rewrite of python install using cmake. We want to ensure that that rewrite does not cause regressions in python package installation. To that end, these tests are developed first to test the existing code. It will be trivial once #587 is complete to extend these tests to include the common case where a message IDL is defined in a package containing python code, which I believe is the most common use case requiring #587. Just to be clear, this current PR does not need #587.

I did not want the build and testing of these test packages to pollute the install directory, so the code generation of these packages is done with the parent being build/ament_cmake_python_test. Under that directory will be the build, log, and install directories for the test packages.

The test packages are generated from templates. The generated packages are under build/ament_cmake_python_test/packages.

It is not reasonable to test all of the artifacts created by a colcon build of the test packages. What was done was create a test package that exercised each of the options to ament_python_install_package along with a smoke test of the features that should have been installed by those options.

There is no need to install this package in the official builds. I don't know how to prevent that, but there would be no real harm done if this is released.

@cottsay I hope this meets your request for some tests.

[rkent]

I'm trying to figure out where the install/lib files ended up in the build farm builds.

[rkent]

I've fixed the issues that prevented this from running successfully on the build farm.

[cottsay]

Thanks for writing these tests. Packages in ament_cmake are grossly under-tested, and your effort here is appreciated.

The first thing that comes to mind looking at this code is that we can't take a dependency on colcon here. No package in ROS requires colcon to build, and we demonstrate this by the lack of colcon in our deb and RPM builds.

It should be possible to chain-call to CMake from the tests of another CMake package. It might also be appropriate to just invoke the macros/functions you're looking to test as part of a package's build process, and then inspect and make assertions about the results of those operations as part of the package's tests. That's how I did this for ament_cmake_vendor_package: https://github.com/ament/ament_cmake/blob/rolling/ament_cmake_vendor_package/test/CMakeLists.txt

[rkent]

The first thing that comes to mind looking at this code is that we can't take a dependency on colcon here.

That is unfortunate, as these tests are really integration tests, and the integration is with colcon. IMHO tests should whenever possible resemble what people might actually use, and colcon is how people are expected to build.

But I will see if I can figure out the non-colcon way to build a package.

[rkent]

So I've tried the trick from ament_cmake_vendor_package to add tests to ament_cmake_python. (The trick, as I understand it, is to add the testing subdirectory to the main CMakeLists.txt with code like this:

if(BUILD_TESTING)
  add_subdirectory(test)
endif()

and in the test subdirectory, directly reference the ***-extras.cmake with code like this:

set(ament_cmake_python_DIR "${CMAKE_CURRENT_LIST_DIR}/../cmake")
include("${CMAKE_CURRENT_LIST_DIR}/../ament_cmake_python-extras.cmake")

The problem that I am having, when I include the code from #587 which adds the python package registration as a hook to ament_cmake_python with code like this:

    ament_register_extension(
      "ament_package"
      "ament_cmake_python"
      "ament_python_install_registered_packages.cmake")

is that weirdly the hook is running before all of the ament_python_install_package code runs for the tests, which seems to run later when ament_cmake_test is built. I think I can work my way around that with stuff like:

unset(_AMENT_PACKAGE_NAME)
project(python_package_version)

in the test packages, but my fear is that this makes these tests fragile with things unrelated to how the actual code works. I fear some poor coder in the future will add something completely unrelated to all of this, and have these tests fail and have to dive into the interactions between cmake, ament, and colcon to try to unravel the mysteries, as I have been doing.

I believe that the underlying problem is that ament_cmake_test itself depends on ament_cmake_python, so I cannot actually have ament_cmake_python use <test_depend>ament_cmake_test</test_depend> like ament_cmake_vendor_package does.

I'm going to try going back to a having a separate package ament_cmake_python_test like I did previously, only without colcon, to see if this will work in a less fragile way. We can discuss later which is the better approach.

[rkent]

@cottsay So I have gone through several iterations of this, with different options, with different downsides. I seem to be stubbornly wanting 1) python-generated test packages, that 2) exercise all relevant options, including '--symlink' which is a 'build' option. I don't believe I can do this cleanly with the 'subdirectory' approach, but I could do it if I call 'cmake' directly. But then there are complications of package dependencies. I asked myself, how does the build farm handle this?

I looks to me like the build farm handles this in testing by using 'colcon test'! Yes it is true that "No package in ROS requires colcon to build, and we demonstrate this by the lack of colcon in our deb and RPM builds." But AFAICT those builds are not building tests! The build that do run tests, the 'dev' and 'pr' builds, are using 'colcon test'.

So I ask, shouldn't it be OK to have a dependency on 'colcon' in the if (BUILD_TESTING) portion of a package?

[cottsay]

The build farm does indeed use colcon test in some cases. The value colcon provides us mainly stems from two things:

1. Resource allocation and ordering among multiple interdependent packages
2. Consistent developer interface and abstraction from the underlying build system (CMake, setuptools, meson, cargo, etc)

Specifically invoking the build of a single CMake package for validating it's functionality like this requires neither of these things. Using colcon in this context pulls in a significant amount of code that essentially amounts to command line argument translation.

Just to entertain the idea, even if we were to invoke colcon in tests like this, there are a number of configuration issues we'd need to address, such as:

1. What colcon extensions are required for the invocation? The dependencies must be declared.
2. Should we block any other extensions the user might have installed when we run the test? For example, additional invocations would be recorded by extensions like colcon-rerun, which could negatively impact the developer experience.
3. What about the user's global colcon configuration file? The workspace configuration file? Should they be ignored?

Regarding the buildfarm never running tests without colcon, we disabled tests in deb and RPM builds in Rolling because we no longer believed that they were providing sufficient value to the community to justify their cost. This was a fairly recent change, and humble is still an active distribution that runs tests in the packaging builds today. It's a boolean configuration in the buildfarm configuration that we can change at any time. There have been discussions about making that a per-package configuration as well.

To be clear: running colcon in package tests isn't something I support doing in ROS core packages, full stop. Colcon is a developer tool. Though not a perfect analogy, I'd draw similarities to setting up a Jenkins instance in your package tests just to build a single project, or launching a Visual Studio GUI to do the same.

If the subdirectory approach isn't working for you, CMake scripts can also be used. It's also possible to use CTest fixtures to coordinate multiple invocations, or even to invoke the CMake executable from pytest.

I think everybody wants more tests, but they need to be maintainable produce actionable results.

[rkent]

@cottsay as requested, these tests now use cmake directly rather than relying on colcon. I can't really test it outside of colcon, as all of the accessible buildfarm runs use colcon themselves.

This has options and comment-outs of code that is intended to test #587 so that it is fairy easy to modify to use with that PR.

Sorry for all of the multiprocessing stuff, without it these tests take over 60 seconds to run which gets annoying.

[kscottz]

@cottsay can you assign this issue to yourself? I don't have sufficient access to assign it to you.

[rkent]

@cottsay @kscottz @christophebedard is there any hope of getting some forward motion on this soon?

[rkent]

It's been 4 weeks now since this has been posted with no progress. My plan is after ROSCON is over, to try pushing again for some progress. This PR is a prerequisite of #587 as requested by the review there, so hopefully after this is landed we can move quickly on #587.