EVerest / everest-framework

Apache License 2.0
21 stars 19 forks source link

Name test target after repo to make it unique. #140

Closed gberardi-pillar closed 9 months ago

gberardi-pillar commented 11 months ago

everest-core cannot build with tests (BUILD_TESTING=ON) due to submodules having generic names that collide when CMake runs.

This change makes the test target for everest-framework unique by naming it after the repo, a pattern that can be followed for all repos.

I also used the Modern CMake pattern of enabling tests only if the project is the main project (not a child of a parent project) or if it is explicitly specified with cmake -Deverest-framework_BUILD_TESTING=ON to allow flexibility.

NOTE: This change does not enable automated builds and tests, as there are other PRs to handle that work.

gberardi-pillar commented 11 months ago

@shankari Here's a minor change to review.

gberardi-pillar commented 11 months ago

Hi @gberardi-pillar, thanks for your contribution.

IMO, it's a good idea to prefix the tests-names.

To be clear, do you mean change the test target name from ${PROJECT_NAME}_tests to tests_${PROJECT_NAME}? If that is the norm, I can change it.

However, I think the root-cause is that BUILD_TESTING is used in all dependencies, too. So when you activate it here (in everest-framework), then it's activated also in many/all(?) dependencies. And then the clash happens.

Is it intended that BUILD_TESTING is used generic in all repos? @hikinggrass @andistorm @a-w50 (Note for myself: If so, revert it in this PR)

BUILD_TESTING is the variable used by CTest, and so I presume that we want to keep that standardized: https://cmake.org/cmake/help/latest/module/CTest.html

Now, there might be some discussion about how to handle testing overall. For instance, if I build everest-framework and run its tests, should doing so also run the tests of its dependencies, which makes sense since I am also building those dependencies, or should I trust that those dependencies ran their own tests before they reached the point of being used by a parent repo and have some way of indicating that the unit tests I want to run are exclusively the current project in that repo?

But for now, presuming that we are building all of the code, as C++ generally wants all code modules to be compiled with the same flags, I think it makes sense to run all of the tests with it, and setting BUILD_TESTING=ON in a parent module should run the tests in all submodules as well.

a-w50 commented 11 months ago

Hi,

this PR is quite similar to #137 , maybe we can use that one instead of this one.

gberardi-pillar commented 10 months ago

Hi,

this PR is quite similar to #137 , maybe we can use that one instead of this one.

That PR is a draft (so I don't know how long it will be before it is in a state to actually be reviewed), and it is supposed to be fixing an issue with a config variable being null (and so seems to be doing too much). This PR is more specific to changing the test target name.

Dominik-K commented 10 months ago

@gberardi-pillar

IMO, it's a good idea to prefix the tests-names.

To be clear, do you mean change the test target name from ${PROJECT_NAME}_tests to tests_${PROJECT_NAME}? If that is the norm, I can change it.

No, just like you did, i.e. PROJECT_NAME_tests. I just wanted to support that approach :+1: Sorry for the confusion.

With prefixes you can see at first step which file belongs to which project. I don't see a use-case where a sorting by tests_... is needed. But perhaps someone else more used to the CMake stuff. E.g. @a-w50 @hikinggrass

You mentioned (in the mailing-list) https://cliutils.gitlab.io/modern-cmake/chapters/testing.html. That looks like a good document for best-practices. In there, it also uses the prefix-approach.

So I would say it's good to go :+1:

Dominik-K commented 10 months ago

Let's discuss in general at

shankari commented 10 months ago

I see that there is a longer-term issue, but it is generally in line with this approach and is primarily tracking its application to multiple repos. So let's get this merged and get the ball rolling on additional changes!

I do note that the "Bazel Build" workflow is currently broken although the failure does not seem to be related to this change. @golovasteek you last committed the bazel build CI; is there a plan to fix the action?

golovasteek commented 10 months ago

I do note that the "Bazel Build" workflow is currently broken although the failure does not seem to be related to this change. @golovasteek you last committed the bazel build CI; is there a plan to fix the action?

I've fixed the bazel-build yesterday, it should work after rebase onto main.