Skip to content

Conversation

@RReichert
Copy link
Contributor

@RReichert RReichert commented May 6, 2022

Background

In looking to introduce unit/integration unit testing into Orion Engine for the Bosch integrity work, I've stumbled up on a few issues in Orion and friends. Firstly there aren't any find_packge scripts for orion-engine, orion_proto, sip, and albatros in this cmake repository, that isn't a problem since each repository has a custom script to include them in the cmake/ directory rather than the cmake/common. The problem is that each of these scripts have not included the necessary option(${PROJECT_NAME}_ENABLE_${FEATURE} "" false) statements which is needed to control the inclusion of submodule test executable.

This causes issues when running do-all-tests in orion-engine as unit tests from both sip (uses swift_add_tests) and starling (ex: the inclusion of test-starling-output-analyzer isn't controlled by starling_BUILD_TESTS here) are executed. So to fix this, this PR does two things:

  1. introduce the find_package scripts for the missing Swift repositories and marks all doc/tests/example as false
  2. fixes up the swift_add_test and swift_add_test_runner functions so that even if the targets aren't protected by a ${PROJECT}_BUILD_TESTS variable (and variants), it won't be included into the do-all-tests target.

Testing

Ran the following script in Orion and than tried to compile the submodules (orion-engine, sip, orion_proto) after I removed the local find_package scripts.

#!/usr/bin/bash

REPOSITORIES=(
    cmake/common/
    third_party/orion_proto/cmake/common/
    third_party/orion_proto/third_party/gnss-converters/c/cmake/common/
    third_party/orion_proto/third_party/gnss-converters/c/third_party/libsbp/c/cmake/common/
    third_party/orion_proto/third_party/gnss-converters/c/third_party/libswiftnav/cmake/common/
    third_party/orion_proto/third_party/sip/cmake/common/
    third_party/orion_proto/third_party/sip/third_party/libsbp/c/cmake/common/
    third_party/orion_proto/third_party/sip/third_party/libswiftnav/cmake/common/
    third_party/orion-engine/cmake/common/
    third_party/orion-engine/third_party/orion_proto/cmake/common
    third_party/orion-engine/third_party/orion_proto/third_party/gnss-converters/c/cmake/common/
    third_party/orion-engine/third_party/orion_proto/third_party/gnss-converters/c/third_party/libsbp/c/cmake/common/
    third_party/orion-engine/third_party/orion_proto/third_party/gnss-converters/c/third_party/libswiftnav/cmake/common/
    third_party/orion-engine/third_party/orion_proto/third_party/sip/cmake/common/
    third_party/orion-engine/third_party/orion_proto/third_party/sip/third_party/libsbp/c/cmake/common/
    third_party/orion-engine/third_party/orion_proto/third_party/sip/third_party/libswiftnav/cmake/common/
    third_party/orion-engine/third_party/starling/cmake/common/
    third_party/orion-engine/third_party/starling/third_party/libpal/cmake/common/
    third_party/orion-engine/third_party/starling/third_party/libsbp/c/cmake/common/
    third_party/orion-engine/third_party/starling/third_party/libswiftnav/cmake/common/
    third_party/orion-engine/third_party/starling/third_party/swiftlets/cmake/common/
    third_party/orion-engine/third_party/starling/third_party/swiftlets/third_party/gnss-converters/c/cmake/common/
    third_party/orion-engine/third_party/starling/third_party/swiftlets/third_party/gnss-converters/c/third_party/libsbp/c/cmake/common/
    third_party/orion-engine/third_party/starling/third_party/swiftlets/third_party/gnss-converters/c/third_party/libswiftnav/cmake/common/
    third_party/orion-engine/third_party/starling/third_party/swiftlets/third_party/libpal/cmake/common/
    third_party/orion-engine/third_party/starling/third_party/swiftlets/third_party/libsbp/c/cmake/common/
    third_party/orion-engine/third_party/starling/third_party/swiftlets/third_party/libswiftnav/cmake/common/
)

for repository in ${REPOSITORIES[@]}
do
  git -C ${repository} fetch
done


for repository in ${REPOSITORIES[@]}
do
  git -C ${repository} checkout origin/rodrigor/integrity-unit-tests-framework
done

Also to verify that this won't have any impact on Starling, I created this PR.

@sonarqubecloud
Copy link

sonarqubecloud bot commented May 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information


GenericFindDependency(
TARGET albatross
SYSTEM_INCLUDES
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these have been included in the customer scripts, I'm not in favor of doing this since this is our code and we should be linting, but I'm not going to attempt to open that can of worms right now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little unclear what you mean here. We shouldn't be setting the options in our find scripts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isaactorz We should only be using SYSTEM_INCLUDE for code that doesn't comply with our coding standards. For example, the optional library (see here) is something that we've imported into our code base from an external source and it might not comply with our strict coding guidelines. Swiftlets on the other hand is something that we internally developed (see here) and therefore must comply.

Internally this SYSTEM_INCLUDES flag makes sure that all header files public from these third party targets are included with -isystem when compiling with GCC and Clang rather than the standard -I. So for example, if we wanted to link optional, it would compile it with something like this:

gcc -o main main.cc ... -L third_party/optional -loptional -isystem /third_party/optional/include ...

rather than the usual

gcc -p main main.cc ... -L third_party/optional -loptional -I /third_party/optional/include ...

Any warnings from header files under -isystem would be ignored rather than cause an error during compilation. Likewise the clang-tidy would ignore any issues under -isystem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, thanks for clarifying.

message(FATAL_ERROR "Both INTEGRATION_TEST and UNIT_TEST option were specified, you can only specify one")
endif()

if (NOT ${PROJECT_NAME}_BUILD_TESTS)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was moved after the add_executable but before the creation of the do-* custom targets so that user can still build these targets manually but it won't be included into the do-all-tests target

@RReichert RReichert requested review from jungleraptor and woodfell May 6, 2022 15:18
Copy link
Contributor

@jungleraptor jungleraptor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm but one question.

What's the purpose for having both an ENABLE_TESTS and a BUILD_TESTS option?

@RReichert
Copy link
Contributor Author

@isaactorz

lgtm but one question.

What's the purpose for having both an ENABLE_TESTS and a BUILD_TESTS option?

From what I recall ${PROJECT_NAME}_ENABLE_TESTS is set by either the find_package or the user through cmake command line (ie -Dswiftlets_ENABLE_TESTS=true). The ${PROJECT_NAME}_BUILD_TESTS is setup by the SwiftCmakeOptions.cmake based on the value of ${PROJECT_NAME}_ENABLE_TESTS, which is normally a one to one assignment but we can add further logic to it (don't know if we do anything like that right now). We could for instance ask the system to build all tests across all organization and override the ${PROJECT_NAME}_ENABLE_TESTS value sent in find_package, this might be useful if we wanted to compile all tests across our codebase and get a real sense of how well our code is covered across the board. The ${PROJECT_NAME}_BUILD_TESTS is used by the build system to determine whether to include a test folder for example, we shouldn't be using ${PROJECT_NAME}_ENABLE_TESTS in our CMakeLists.txt file.

@RReichert RReichert merged commit 4c16386 into master May 7, 2022
@RReichert RReichert deleted the rodrigor/integrity-unit-tests-framework branch May 7, 2022 05:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants