search

aurora-opensource / au

A C++14-compatible physical units library with no dependencies and a single-file delivery option. Emphasis on safety, accessibility, performance, and developer experience.
Apache License 2.0
329 stars 21 forks source link

Gate CMake tests behind `AU_ENABLE_TESTING` option #303

Closed chiphogg closed 1 month ago

chiphogg commented 1 month ago

This brings in the external contributions from #297, merges in the latest main, and prepends an AU_ prefix to the option.

A future PR will add another option to let users opt out of the GTest dependency entirely, losing access to the Au::testing target (which wouldn't be any loss to people who don't use GTest).

To test:

cmake \
  -S . \
  -B cmake/build \
  -DAU_ENABLE_TESTING=FALSE \
  -DCMAKE_VERIFY_INTERFACE_HEADER_SETS=TRUE \
&& cmake \
  --build cmake/build \
  --target all

Helps #257.

chiphogg commented 1 month ago

It looks like targets in au/code/au/CMakeLists.txt like chrono_policy_validation still depend on GTest::gtest. Is that expected?

I see an error when I run

docker run --rm \
ubuntu@sha256:2e863c44b718727c860746568e1d54afd13b2fa71b160f5cd9058fc436217b30 \
/bin/bash -c \
'apt update && apt install -y clang cmake git && git clone -b chiphogg/update_vars#257 https://github.com/aurora-opensource/au.git && cd au && cmake -S . -B cmake/build -DAU_ENABLE_TESTING=FALSE -DCMAKE_VERIFY_INTERFACE_HEADER_SETS=TRUE && cmake --build cmake/build -j $(proc) --target all'

Nice catch!

The current plan of record includes making two options: one for running the tests, and one for including the gtest dependency at all. In the end, if the latter is false, then we will force the former to be false as well.

This present PR just adds the first option. The main reason it exists is so that we can land @ericriff's contributions to main, and give him credit. But his PR predates the fuller understanding reflected in our plan of record: it conflates the two options, and assumes that users skipping the tests would also want to remove the GTest dependency. I fixed it up in the latest commit.

Of course, this reveals a hole in our CI testing: we didn't add tests for this new option. I generally prefer to do this in the same PR, but editing .github will add another reviewer to this PR, and I think it makes more sense to do this holistically once both options have been added. I filed #304 so we won't lose track of this.

chiphogg commented 1 month ago

(See the updated PR summary for a testing command.)