Skip to content

Added cmake option for disabling tests #51

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
merged 4 commits into from
Apr 20, 2018

Conversation

EvilBeaver
Copy link
Contributor

It's useful to have an option for disabling tests when building on CI server.

Use Case:

  • Imagine you have a project which uses cppkafka as a submodule
  • You build project with CI server and for full-source build you do a git checkout with a recursive submodule update
  • Now after checkout you have googletest submodule also cheсked out and cppkafla will be doing a tests build
  • But as a submodule, cppkafka is always checked out on one specified commit and this means it's code is unchanged and stable. So it doesn't require any continuous testing.
  • Having cppkafka tests built on your daily builds all you have is a time waste

This PR introduces CMake option CPPKAFKA_BUILD_TESTS which is enabled by default and old behavior is untouched. But it's easy to disable building tests with this option.

@mfontanini
Copy link
Owner

Thanks for this PR! The tests are not included in the all Makefile target so I don't think normally this would be an issue since the build will just skip them.

Anyway, this is a reasonable change and I can see its usefulness. The only thing I'd change is the option name. I normally prefer names in the form of <project>_DISABLE_<feature> for these types of parameters. Maybe CPPKAKFA_DISABLE_TESTS? Otherwise this looks like CPPKAFKA_BUILD_SHARED but has a very different meaning: the latter controls how something is built and the former controls what is being built.

On a similar note, there should probably be a similar flag to disable examples as well, but that can be tacked later.

@EvilBeaver
Copy link
Contributor Author

Yes, i chose parameter name looking at CPPKAFKA_BUILD_SHARED option.
I build it using cmake and Visual Studio so Makefile and it's targets are not involved. Okay, I'll change option name

@EvilBeaver
Copy link
Contributor Author

done

@mfontanini
Copy link
Owner

Fair enough, I didn't consider that. Looks like I typo-ed the option name and you copy pasted it. It says CPPKAKFA. Could you correct it? Sorry about that!

@EvilBeaver
Copy link
Contributor Author

Copypastes are evil :) Fixed

@mfontanini
Copy link
Owner

lol yeah, also I should have probably worn my glasses while I replied in the morning. Thanks, I'll merge this later!

@mfontanini mfontanini merged commit c95d790 into mfontanini:master Apr 20, 2018
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