Skip to content

Support CTest #1468

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 3 commits into from
Jan 21, 2023
Merged

Support CTest #1468

merged 3 commits into from
Jan 21, 2023

Conversation

jimmy-park
Copy link
Contributor

I added test/CMakeLists.txt because the current testing workflow is separate from the CMake build system.
Now we can do this for testing in a cross-platform way.

cmake -S . -B build -DHTTPLIB_TEST=ON
cmake --build build && cd build
ctest

I also tested httplib with OpenSSL as shown in test/Makefile.
This requires me to generate some cert files in the build directory.
Here is results on my machine (Windows 10)

result-windows

It may test with Brotli and zlib as well if libraries are installed correctly.

Add HTTPLIB_TEST option
Downlaod GoogleTest source using FetchContent module
Generate cert files to test httplib with OpenSSL
@jimmy-park jimmy-park marked this pull request as ready for review January 14, 2023 05:12
@jimmy-park
Copy link
Contributor Author

Testing with OpenSSL, Brotli and zlib was successful in my VM (Ubuntu 22.04).
But it needed to install following libraries.

sudo apt install -y libssl-dev libbrotli-dev zlib1g-dev

@jimmy-park
Copy link
Contributor Author

For some reason, generating cert2.pem only fails on macOS unless we add the -new option.
1a41e71 will fix this issue.

PS. tests in CI sometimes fail due to network issues.

@yhirose
Copy link
Owner

yhirose commented Jan 21, 2023

@jimmy-park, thanks for the pull request. I found some embedded strings, 1) physical paths such as www, www2..., 2) version number like https://github.com/google/googletest/archive/release-1.12.1.tar.gz. These my cause problems eventually and need to be taken care of. Please see more information in #955, #1033, #1044.

I actually don't maintain any CMake related files. @sum01 kindly takes care of those files, so that I don't need to worry about it. Could you willingly take a responsibility to maintain test/CMakeLists.txt whenever problems happen with it in the future? If so, I'll accept this pull request. Please see my policy about CMake and Meson in the following comment.
#955 (comment)

Thanks for your understanding!

@jimmy-park
Copy link
Contributor Author

@yhirose Thanks for replying.

  1. physical paths such as www, www2...

Since test.cc requires some files where it located in and httplib-test will be executed in CMAKE_CURRENT_BINARY_DIR, I needed to copy these files from the test directory to the CMAKE_CURRENT_BINARY_DIR. Cert files are created the same way.

  1. version number like https://github.com/google/googletest/archive/release-1.12.1.tar.gz

We can choose the latest main branch using https://github.com/google/googletest/archive/main.tar.gz. It seems like Google also recommends this way. Is it okay with this library?

I saw comments and understand what you are concerned about. I'm willing to maintain test/CMakeLists.txt and update it when test/Makefile changes.

Thanks for the useful library 😄

@yhirose
Copy link
Owner

yhirose commented Jan 21, 2023

@jimmy-park thanks for the answers, and accept maintaining it! As you mentioned, could you try to use Google Test from the latest main branch?

@jimmy-park
Copy link
Contributor Author

@yhirose Sure!

@yhirose yhirose merged commit 20cba2e into yhirose:master Jan 21, 2023
@yhirose
Copy link
Owner

yhirose commented Jan 21, 2023

@jimmy-park, thanks for the fine contribution!

@jimmy-park jimmy-park deleted the support-ctest branch January 21, 2023 08:51
Copy link
Contributor

@Tachi107 Tachi107 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for my late review, but I wasn't notified about this patch until the 0.12.0 release.

Mostly minor issues/suggestions

@yhirose
Copy link
Owner

yhirose commented Feb 10, 2023

@jimmy-park could you replay to @Tachi107's comments and suggestions?

@jimmy-park
Copy link
Contributor Author

@Tachi107 Thanks for suggestions. I commented my thoughts and some possible modifications.

@yhirose I think applying @Tachi107's suggestion is required. However, since this pull request had been merged, I will open a new pull request after resolving these conversations.

@yhirose
Copy link
Owner

yhirose commented Feb 10, 2023

@jimmy-park yes, please go ahead to make another pull request for the adjustments. Thanks!

ExclusiveOrange pushed a commit to ExclusiveOrange/cpp-httplib-exor that referenced this pull request May 2, 2023
* Add test/CMakeLists.txt to enable testing with CTest

Add HTTPLIB_TEST option
Downlaod GoogleTest source using FetchContent module
Generate cert files to test httplib with OpenSSL

* Generate cert2.pem with a new certificate request

* Use the latest GoogleTest library
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.

3 participants