Skip to content
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

fix: Make tests optional #691

Closed
wants to merge 1 commit into from

Conversation

joeyparrish
Copy link

@joeyparrish joeyparrish commented Aug 16, 2022

When including zlib as a submodule, it is useful to be able to disable the tests.

EXCLUDE_FROM_ALL when including zlib will prevent tests from being automatically built, but cmake will still attempt to run them, resulting in something like this from make test in the host project:

    Start 5: example
Could not find executable example
Looked in the following places:
example
example
Release/example
Release/example
Debug/example
Debug/example
MinSizeRel/example
MinSizeRel/example
RelWithDebInfo/example
RelWithDebInfo/example
Deployment/example
Deployment/example
Development/example
Development/example
Unable to find executable: example
5/6 Test #5: example ..........................***Not Run   0.00 sec
    Start 6: example64
Could not find executable example64
Looked in the following places:
example64
example64
Release/example64
Release/example64
Debug/example64
Debug/example64
MinSizeRel/example64
MinSizeRel/example64
RelWithDebInfo/example64
RelWithDebInfo/example64
Deployment/example64
Deployment/example64
Development/example64
Development/example64
Unable to find executable: example64
6/6 Test #6: example64 ........................***Not Run   0.00 sec

67% tests passed, 2 tests failed out of 6

Total Test time (real) =  23.97 sec

The following tests FAILED:
          5 - example (Not Run)
          6 - example64 (Not Run)

This is discussed in https://gitlab.kitware.com/cmake/cmake/-/issues/20212, but after 2 years, there is no obvious consensus on a fix. So it should be up to the library to be responsible and use the BUILD_TESTING variable to guard their calls to add_test().

@EmosewaMC
Copy link

It'd be great to be able to disable the tests when using this project as a submodule. I'm spending more time than I would like attempting to get these tests found by ctest so I can get back to working on other stuff. Just being able to disable them since they do not pertain to our project would be great.

@joeyparrish
Copy link
Author

@madler, would you please look at this when you have time? We are currently forced to use a fork of zlib because of CMake issues like this.

@nmoinvaz
Copy link
Contributor

nmoinvaz commented Nov 3, 2022

Can you just do add_subdirectory(zlib EXCLUDE_FROM_ALL)?

@joeyparrish
Copy link
Author

@nmoinvaz
Copy link
Contributor

nmoinvaz commented Nov 3, 2022

Actually, I just ran across this in another repository. Here is the associated Kitware issue: https://gitlab.kitware.com/cmake/cmake/-/issues/20212.

Alternatively, you could use zlib-ng which has options to disable testing, but it doesn't fix the problem for this repo.

@joeyparrish
Copy link
Author

Yes, thank you for bringing this up. My PR description was poor, and should have had those details.

Here is what happens with EXCLUDE_FROM_ALL, but without this PR:

    Start 5: example
Could not find executable example
Looked in the following places:
example
example
Release/example
Release/example
Debug/example
Debug/example
MinSizeRel/example
MinSizeRel/example
RelWithDebInfo/example
RelWithDebInfo/example
Deployment/example
Deployment/example
Development/example
Development/example
Unable to find executable: example
5/6 Test #5: example ..........................***Not Run   0.00 sec
    Start 6: example64
Could not find executable example64
Looked in the following places:
example64
example64
Release/example64
Release/example64
Debug/example64
Debug/example64
MinSizeRel/example64
MinSizeRel/example64
RelWithDebInfo/example64
RelWithDebInfo/example64
Deployment/example64
Deployment/example64
Development/example64
Development/example64
Unable to find executable: example64
6/6 Test #6: example64 ........................***Not Run   0.00 sec

67% tests passed, 2 tests failed out of 6

Total Test time (real) =  23.97 sec

The following tests FAILED:
          5 - example (Not Run)
          6 - example64 (Not Run)

@joeyparrish
Copy link
Author

I updated the PR description with more detail on why this is necessary. Thanks, @nmoinvaz, for the link to the cmake issue tracker!

@joeyparrish joeyparrish changed the base branch from master to develop November 9, 2022 15:07
@joeyparrish joeyparrish changed the title fix: Make test executables optional fix: Make tests optional Nov 9, 2022
When including zlib as a submodule, it is useful to be able to disable
the tests.  EXCLUDE_FROM_ALL when including zlib will prevent tests
from being automatically built, but cmake will still attempt to run
them, resulting in a test failure.

This problem is discussed in
https://gitlab.kitware.com/cmake/cmake/-/issues/20212, but after 2
years, there is no obvious consensus on a fix. So it should be up to
the library to be responsible and use the BUILD_TESTING variable to
guard their calls to add_test().
@joeyparrish
Copy link
Author

The change is now backward compatible for the sake of those who depend on the example and minigzip binaries. Only the add_test() calls have been made conditional on BUILD_TESTING. This has also been rebased and retargeted to the develop branch.

Thanks!

@EmosewaMC
Copy link

Bumping as this thread has not seen activity for a few months.

@Neustradamus
Copy link

@madler: What do you think?

@EmosewaMC
Copy link

Bumping as this thread has not seen activity for a few months.

@madler
Copy link
Owner

madler commented Jan 18, 2024

A fix like this has been applied.

@madler madler closed this Jan 18, 2024
@EmosewaMC
Copy link

Thank you!

@joeyparrish joeyparrish deleted the optional_tests branch March 11, 2024 21:35
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.

5 participants