@purpleKarrot , thanks for contributions inside the Beman Project!

@bretbrownjr , can you please review this?

I'm missing something in all of the CMake arcana.

Bottom, line I wouldn't call myself opposed to this change, but it's hard to approve and merge without more documentation and education for Beman contributors. I really like the thought and expertise that went into this PR, though.

So just to reiterate what I think is required of us. I think we should intentionally and cleanly support a test workflow. I don't think conditionally defining test hooks is the best way to do that. It's a lot cleaner to conditionally build tests but to unconditionally provide the hooks. Why? Because there's no standard and widely adopted mechanism or workflow to query a CMake project about whether there are tests or not. So it's best to let the user just run tests and let the xUnit results and CTest output convey the outcome of that action.

As to the justification in a885d77, most users use the default Unix Makefiles generator or one of the Ninja generators, and all of those define a test target. The commit is justifying itself in terms of those popular workflows that are familiar to the typical Beman contributors. And those are the workflows mechanically supported because they are wired up in CI, so I think that's good justification, even if there are other workflows in question. I think editing the commit to be more pedantically encompassing isn't worth it at this point, but maybe we should update the Beman Standard in more detail to document explicitly which CMake and CTest workflows we use and support?

As to enable_testing() versus include(CTest), I'm aware of include(CTest) and the recommendation to use it. I don't object to replacing enable_testing() with that if we all like that better, but I do have a minor quibble about how it clutters up the project's build targets with certain generators (i.e., output from ninja -t targets all includes a lot of often irrelevant targets to support CTest features). I'm not a huge fan of BUILD_TESTING to conflate the building of tests and the act of wiring them up to the test target, but I guess I don't mind using it in the literal sense to disable building of tests. But to reiterate, I'm not convinced we should use it in the more encompassing sense to control existence of test hooks.

As to the proposed commit, I have concerns about using CTestUseLaunchers, mainly because it's an arcane feature that needs to be better explained and documented at a minimum. I think it's fair to say that this isn't typical CMake style or even a pattern espoused in the latest upstream CMake docs or in Professional CMake (though maybe I missed something in there). Normally my bar for unusual or surprising code is to justify it in a comment, but the CTestUseLaunchers docs aren't helpful because they assume the reader knows a lot of specifics about various CTest details and workflows. It's also unclear if any of th described mechanisms and features are important to CMake build-and-test workflows (i.e., what is currently used in CI and docs) and how many are important to CDash or CMake-as-a-scripting-language workflows (which we are not supporting in this project so far). I'm not aware of a published tutorial or explainer of CTestUseLaunchers we could link to otherwise.

For similar reasons (i.e., explaining surprising or important code), I have similar concerns about intentionally moving enable_testing() inside the BEMAN_EXEMPLAR_BUILD_TESTS conditional. If it's incorrect to put that line somewhere else, that fact is not self-evident, so a justification comment of some sort is warranted. Again, it could be with a link for more info -- perhaps to an entry in a newly improved Beman Standard or some other developer documentation.

https://github.com/search?q=%22include%28CTestUseLaunchers%29%22&type=code shows 382 hits, many of which are forks of the same video project, or cmake itself or cmake docs.

If it's useful, are we using it?

@purpleKarrot Just checking in on this. I'm interested in this PR still. It sounds like you might have some ideas worth considering. We're looking for more explanation to proceed with this change.

If it's easier to have a more interactive discussion, we have a Discourse that supports both forum posts and interactive chatting. Feel free to bring this up there.

@purpleKarrot's rationale makes sense to me. The standard way to run tests in CMake projects is using the ctest executable. My understanding is that this patch will allow the ctest command to operate properly without confusingly marking this project as having testing enabled.

Hey @purpleKarrot, this PR was reviewed positively a couple of weeks ago. You should be able to merge it yourself but we can do it on your behalf if you like.

Hey @purpleKarrot, this PR was reviewed positively a couple of weeks ago. You should be able to merge it yourself but we can do it on your behalf if you like.

I see options to rebase and close, but no option to merge. Please do it or give me permission.

I see options to rebase and close, but no option to merge. Please do it or give me permission.

Permission changes scare me more than merging. While we're sorting out the right bits, I'm hitting the "Merge pull request" button.