8000 Add UTPP_INCLUDE_TESTS_IN_BUILD CMake option by pjohnmeyer · Pull Request #115 · unittest-cpp/unittest-cpp · GitHub
[go: up one dir, main page]

Skip to content

Add UTPP_INCLUDE_TESTS_IN_BUILD CMake option #115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

pjohnmeyer
Copy link
Member

This PR helps address #104 by giving the downstream project the ability to bypass tests at build time. The default behavior remains the same.

@pjohnmeyer pjohnmeyer added this to the 1.6.2 milestone May 7, 2016
@pjohnmeyer pjohnmeyer changed the title Add UTPP_SKIP_TESTS_AS_BUILD_STEP CMake option Add UTPP_RUN_TESTS_AS_BUILD_STEP CMake option May 7, 2016
@sbraz
Copy link
sbraz commented May 7, 2016

It would be nice if we could also have an option to skip the build part since end users don't need the tests at all. We currently comment out that part: https://gitweb.gentoo.org/repo/gentoo.git/tree/dev-libs/unittest++/unittest++-1.6.1.ebuild#n26
I'm no cmake expert but I know it has ctest which is usually meant to run the tests. Any reason you're not using it?

@pjohnmeyer
Copy link
Member Author

I would prefer to see the tests built even if they aren't being run, because if the tests fail to compile that may catch an issue with the package on the end system. It's not as good of a check as actually running them, but it's better than nothing. Think of it as building an "example client" of the library to ensure that everything is okay.

I previously removed the usage of CTest because it added complexity to the CMake files for no discernible gain. I've seen CTest be useful for projects that are maintaining large CI matrix builds and submitting results to a CDash dashboard, but no such infrastructure exists for this project, so this is simpler for me as a maintainer.

My recommendation is to use the new flag and leave the compiling of the tests in. Alternately, you could download the release (starting with the latest release, v1.6.1) from https://github.com/unittest-cpp/unittest-cpp/releases and use configure and make, but as I've just started doing this it may be wiser to stick with CMake for the near future.

@sbraz
Copy link
sbraz commented May 7, 2016

I would prefer to see the tests built even if they aren't being run, because if the tests fail to compile that may catch an issue with the package on the end system. It's not as good of a check as actually running them, but it's better than nothing. Think of it as building an "example client" of the library to ensure that everything is okay.

Well ebuilds are for end users. End users do not need to compile examples or tests. This is what Gentoo developers do. If someone wants to build the tests, they can but it should not be unconditional.
Of course, this is not such a big deal as we are able to edit CMakeLists.txt directly. But, since we're not going to change the behaviour of our ebuild (I've discussed that with an experienced Gentoo dev), it would make it easier for us to have a flag to disable test building.

I previously removed the usage of CTest because it added complexity to the CMake files for no discernible gain. I've seen CTest be useful for projects that are maintaining large CI matrix builds and submitting results to a CDash dashboard, but no such infrastructure exists for this project, so this is simpler for me as a maintainer.

Fair enough, I just asked out of curiosity :)

Alternately, you could download the release (starting with the latest release, v1.6.1) from https://github.com/unittest-cpp/unittest-cpp/releases and use configure and make, but as I've just started doing this it may be wiser to stick with CMake for the near future.

I did not see that you had a configure script in recent releases. What does it do differently?

@pjohnmeyer
Copy link
Member Author

The configure script is generated by autoreconf, and in turn will generate a Makefile with an install target. The install target builds the library and installs it. There is a separate check target for building and running tests.

@pjohnmeyer
Copy link
Member Author

If you are intending to switch to using the released ./configure script, I'll close this pull request.

If you'd like to stick with CMake, I'll make the necessary changes to this PR so you can stop patching the CMake file. Please advise.

@pjohnmeyer
Copy link
Member Author
pjohnmeyer commented Jul 16, 2016

@sbraz could use some additional input on this. I would like to eliminate the need for Gentoo to have to run sed on the CMake file, but I need specific guidance to do that.

@pjohnmeyer pjohnmeyer removed this from the 1.6.2 milestone Jul 16, 2016
@sbraz
Copy link
sbraz commented Jul 17, 2016

Well what we could do is:

  • add a switch to disable test building (-DBUILD_TESTS=OFF, maybe?)
  • add another switch that prevents the tests from being run by cmake (-DRUN_TESTS=OFF?)
    What do you think? I can try to make a PR but I'm no cmake expert.

UTPP_SKIP_TESTS_AS_BUILD_STEP with a default value of OFF was an
unfortunate choice, so this changes it to UTPP_RUN_TESTS_AS_BUILD_STEP
with a default value of ON.
As part of this, changed the option name to
`UTPP_INCLUDE_TESTS_IN_BUILD`. The test target is still added to the
build and can be built/run separately using the CMake --target option.
@pjohnmeyer pjohnmeyer force-pushed the add_cmake_option_to_disable_running_tests_as_build_step branch from 4d7ae2e to 6b69ed7 Compare July 19, 2016 02:15
@pjohnmeyer pjohnmeyer changed the title Add UTPP_RUN_TESTS_AS_BUILD_STEP CMake option Add UTPP_INCLUDE_TESTS_IN_BUILD CMake option Jul 19, 2016
@pjohnmeyer
Copy link
Member Author

@sbraz I don't want to overcomplicate the build file with any more flags than needed. The latest push includes a single option, UTPP_INCLUDE_TESTS_IN_BUILD. If it is set to OFF, the build and run of the TestUnitTest++ target are skipped as part of the default build. The target still exists and can still be built, but it is not built by default.

@sbraz
Copy link
sbraz commented Jul 20, 2016

Well we'll still need to run sed because we don't want to build tests and run them during the same phase.

@pjohnmeyer
Copy link
Member Author

Why bother with sed at that point? If the tests fail then the build phase fails. If the tests pass then the build phase passes, and the test phase passes. They take a very very short time to run, so running them twice -- while admittedly wasteful -- is of little harm.

If I were to change it any further, it would be to change the op 8000 tion to be something like UTPP_N_PHASE_BUILD, in which each step of the build is fully independent and separately buildable. I don't particularly care for it, though, and not sure that it adds any value to the project.

@pjohnmeyer
Copy link
Member Author

Merging this as-is, as I can see the utility in it as currently implemented.

@pjohnmeyer pjohnmeyer added this to the 2.0.0 milestone Aug 17, 2016
@pjohnmeyer pjohnmeyer merged commit 83dbcc0 into master Aug 17, 2016
@sbraz
Copy link
sbraz commented Aug 17, 2016

Why bother with sed at that point? If the tests fail then the build phase fails. If the tests pass then the build phase passes, and the test phase passes. They take a very very short time to run, so running them twice -- while admittedly wasteful -- is of little harm.

Hi, sorry I didn't reply before.
Most Gentoo users don't need tests, we need to run them separately like this: gentoo/gentoo@9d26b34#diff-2375929512a67fe5d82edbc04e6a9730R32
This is the way all ebuilds should work and I'm not really at liberty to change that.
We also have this issue where they fail on hardened systems so running them in the compile phase will prevent these users from installing the package altogether.

@pjohnmeyer
Copy link
Member Author

Understood. I will happily consider a pull request from another contributor to address this.

@pjohnmeyer pjohnmeyer deleted the add_cmake_option_to_disable_running_tests_as_build_step branch January 13, 2017 19:41
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.

2 participants
0