Skip to content

fix: Make tests optional#691

Closed
joeyparrish wants to merge 1 commit intomadler:developfrom
joeyparrish:optional_tests
Closed

fix: Make tests optional#691
joeyparrish wants to merge 1 commit intomadler:developfrom
joeyparrish:optional_tests

Conversation

@joeyparrish
Copy link
Copy Markdown

@joeyparrish joeyparrish commented Aug 16, 2022 •
edited
Loading

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().

@Neustradamus Neustradamus mentioned this pull request Aug 18, 2022
Closed
@EmosewaMC
Copy link
Copy Markdown

EmosewaMC commented Sep 4, 2022

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
Copy Markdown
Author

joeyparrish commented Nov 3, 2022

@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
Copy Markdown
Contributor

nmoinvaz commented Nov 3, 2022

Can you just do add_subdirectory(zlib EXCLUDE_FROM_ALL)?

@joeyparrish
Copy link
Copy Markdown
Author

joeyparrish commented Nov 3, 2022

We are doing that: https://github.com/shaka-project/shaka-packager/blob/0ee4af438d4ff943cf747f083cd149223d2bd4c5/packager/third_party/CMakeLists.txt#L39

@nmoinvaz
Copy link
Copy Markdown
Contributor

nmoinvaz commented Nov 3, 2022 •
edited
Loading

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
Copy Markdown
Author

joeyparrish commented Nov 3, 2022

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
Copy Markdown
Author

joeyparrish commented Nov 3, 2022

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

nmoinvaz
nmoinvaz reviewed Nov 3, 2022
View reviewed changes
@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
@joeyparrish
fix: Make tests optional
d8fb711 
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
Copy Markdown
Author

joeyparrish commented Nov 9, 2022

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
Copy Markdown

EmosewaMC commented Feb 13, 2023

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

@Neustradamus
Copy link
Copy Markdown

Neustradamus commented Jul 30, 2023

@madler: What do you think?

@EmosewaMC
Copy link
Copy Markdown

EmosewaMC commented Dec 30, 2023

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

@madler
Copy link
Copy Markdown
Owner

madler commented Jan 18, 2024

A fix like this has been applied.

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

EmosewaMC commented Jan 18, 2024

Thank you!

@joeyparrish joeyparrish deleted the optional_tests branch March 11, 2024 21:35
@Neustradamus Neustradamus mentioned this pull request Jan 1, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@nmoinvaz nmoinvaz nmoinvaz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@joeyparrish @EmosewaMC @nmoinvaz @Neustradamus @madler