Add ament_auto_add_gtest

[JWhitleyWork]

This is an alternative/replacement for #257. Instead of adding a new package, it just extends ament_cmake_auto with an ament_auto_add_gtest macro. I've tested this against 10 random packages in Autoware.Auto and it appears to work as expected. However, there are some unanswered questions as to how it should actually work:

Question 1

ament_lint_auto_find_test_dependencies() currently finds all test_depends in package.xml. However, this macro is part of ament_lint_auto and also triggers the lint hooks. Should I also create an ament_auto_find_test_dependencies() in ament_cmake_auto to replicate the find_package functionality without triggering the lint hooks?

Question 2

When writing tests, most developers will use packages which are listed as <build_depend> or <depend> in package.xml. These are automatically find_package()'d by ament_auto_find_build_dependencies(). However, according to REP-149:

A <test_depend> may reference a package also declared as some other type of dependency.

In addition, <depend> only adds a key as <build_depend>, <build_export_depend>, and <exec_depend> - explicitly not as a <test_depend>. To me, these two things together indicate that the CMake-produced list of test dependencies is not a superset of the listed <build_depend>s, <buildtool_depend>s, and <test_depend>s but is explicitly just the listed <test_depend>s. If this interpretation is correct, I think most developers are missing a lot of <test_depend>s and using this macro as currently written will expose that. So, the question is: Should we add all build and buildtool depends as dependencies to tests that use this macro or not?

[hidmic]

Should I also create an ament_auto_find_test_dependencies() in ament_cmake_auto to replicate the find_package functionality without triggering the lint hooks?

That sounds reasonable. ament_lint_auto is likely finding more than it needs to. It should probably rely on such a new ament_cmake_auto macro.

I think most developers are missing a lot of <test_depend>s and using this macro as currently written will expose that.

Well, quoting REP-149:

The <exec_depend> is needed for packages providing shared libraries, executable commands, Python modules, launch scripts or any other files required for running your package. It is also used by metapackages for grouping packages.

So, I'd expect tests to depend on test dependencies, execution dependencies, and the libraries provided by the package itself (and their build dependencies, but only transitively).

Should we add all build and buildtool depends as dependencies to tests that use this macro or not?

I'd think that ${PROJECT_NAME}_LIBRARIES plus test dependencies plus exec dependencies (including those listed through <depend>s) should be enough.

---

This is an alternative/replacement for #257.

An aspect not covered by this patch is automatic test discovery. A one GTest per file heuristic would probably deal with most typical cases. CC @ament/team for thoughts.

[JWhitleyWork]

ament_lint_auto is likely finding more than it needs to.
Actually, after loading the contents of the package.xml, this is all that ament_lint_auto_find_test_dependencies() does:

  foreach(_dep ${${PROJECT_NAME}_TEST_DEPENDS})
    find_package(${_dep} QUIET)
    if(${_dep}_FOUND)
      list(APPEND ${PROJECT_NAME}_FOUND_TEST_DEPENDS ${_dep})
    endif()
  endforeach()

Basically, exactly what I would expect ament_auto_find_test_dependencies() to do. This is why I asked about a replacement. Also, currently, ament_lint_auto does not rely on ament_cmake_auto and I would assume we don't want this cross-dependency between core packages.

So, I'd expect tests to depend on test dependencies, execution dependencies, and the libraries provided by the package itself (and their build dependencies, but only transitively).

Except they don't explicitly do that and I don't know that we want to do that. For example, if you depend on something heafty in building your library (like boost) but you don't use it at all in your tests (which may just access the API of your library), I wouldn't want all of my tests automatically depending on it just because it's a build_depend for the main library.

We definitely don't want to automatically have tests depend on exec_depends. This isn't even currently done for ament_add_executable or ament_add_library and I would argue that they would be more likely to depend on an exec_depend than a test (especially a unit tests).

An aspect not covered by this patch is automatic test discovery.
I'm not sure I get what you mean by "automatic test discovery." If you mean something like discovering tests based on folder structure and/or filenames, I'm very against that. Could you clarify?

[hidmic]

Basically, exactly what I would expect ament_auto_find_test_dependencies() to do.

Right, which includes all lint dependencies as a subset. That's what I meant by it finding more than it needs.

Also, currently, ament_lint_auto does not rely on ament_cmake_auto and I would assume we don't want this cross-dependency between core packages.

We can defer it for now but convergence would be nice, eventually.

I wouldn't want all of my tests automatically depending on it just because it's a build_depend for the main library.

We definitely don't want to automatically have tests depend on exec_depends.

Let's back up and build up to a reasonable heuristic for dependencies (which is all this is, a heuristic to cover as many use cases as possible).

At the bare minimum, tests have to depend on all package libraries and all test dependencies (that can be found by CMake). Exported build dependencies have to be included as well (explicitly or implicitly through CMake targets).

Should build tool dependencies be included? These are dependencies for the build process itself, so probably not.

Should build dependencies be included? It'd cover a lot more ground that way. It'll clutter the build graph too, and rely on the toolchain to ignore the unnecessary libraries (which it will, unless explicitly instructed not to do so). That's already the case for test dependencies and build export dependencies though, by virtue of package-level granularity for dependencies (i.e. we can't pick them wisely).

Should execution dependencies be included? An execution dependency that affects a test build is already a build dependency or should be a test dependency. So no. I stand corrected there.

Since a build dependency is a package needed at build-time, and considering ament_cmake_auto sole purpose is to provide a convenient interface, I'd be inclined towards including build dependencies and let the toolchain shave off the surplus. If I need precision in a complex package, I'll probably won't use ament_cmake_auto.

I'm not sure I get what you mean by "automatic test discovery." If you mean something like discovering tests based on folder structure and/or filenames, I'm very against that.

That's exactly what I meant. The test/test_*.c(c|cpp) path pattern repeats often enough to be leveraged as a reasonable default (emphasis on default).

[hidmic]

@JWhitleyWork friendly ping.

[JWhitleyWork]

Thanks for the ping, @hidmic. I have addressed your feedback:

• ament_auto_add_gtest now adds all found build dependencies as target dependencies in the same manner as ament_auto_add_executable in addition to any found test dependencies
• ament_auto_find_test_dependencies has been added as an analogue/alternative to ament_lint_auto_find_test_dependencies()

I tested this again on a few Autoware.Auto packages and the following syntax works as expected:

find_package(ament_auto_cmake REQUIRED)
ament_auto_find_build_dependencies()
...
if(BUILD_TESTING)
  ament_auto_find_test_dependencies()

  ament_auto_add_gtest(test_name
    "test/test_something.cpp"
  )
endif()

With the above syntax, leaving ament_lint_auto and ament_lint_common as dependencies runs the linter tests since the running of the linters is a package hook on ament_lint_auto. Removing ament_lint_auto skips building the tests, as expected.

While I get where you're coming from with regard to the filename-based auto test generation, I think this should be a separate macro such as ament_auto_add_gtests(). Additionally, I think this is something that will generate more discussion and seeing as none of the ament team responded, I'd rather keep that in a separate PR if that's OK with you.

[hidmic]

@JWhitleyWork this patch makes sense to me but for one question. Have you linted it?

While I get where you're coming from with regard to the filename-based auto test generation, I think this should be a separate macro such as ament_auto_add_gtests(). Additionally, I think this is something that will generate more discussion and seeing as none of the ament team responded, I'd rather keep that in a separate PR if that's OK with you.

Agree on it being a different macro and on deferring it to a follow-up PR. #257 by @cho3 did exercise the idea, so perhaps they may be willing to build on top of your PR?

[hidmic]

BTW sorry for the delay to reply.

[JWhitleyWork]

Agree on it being a different macro and on deferring it to a follow-up PR. #257 by @cho3 did exercise the idea, so perhaps they may be willing to build on top of your PR?

@cho3 's open-source contributions appear to have gone silent for the last year or so. I doubt he will be doing this anytime soon. Would you be interested in adding this functionaly?

Additionally, would you mind approving this if you're OK with the current implementation?

[hidmic]

Would you be interested in adding this functionaly?

It would be nice, but I won't block this PR on it.

---

LGTM pending green CI (above ament_cmake_auto, JIC):

• Linux
• Linux-aarch64
• macOS
• Windows

[hidmic]

Test failures appear unrelated. Moving forward here.