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

Integrate Googletest as Test Framework #13

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

zhjwpku
Copy link
Contributor

@zhjwpku zhjwpku commented Dec 15, 2024

In the discussion #12 regarding the choice between Googletest and Catch2 for our testing framework, most of the developers have experience with Googletest, and while they are open to considering Catch2, they see no immediate advantage, as no one on the team has experience with it. Given this context, the decision to adopt Googletest was made based on the familiarity and comfort level of the majority of the team.

We can switch to Catch2 with one shot if it proves to be a better fit for our needs in the future.

@pitrou
Copy link
Member

pitrou commented Dec 15, 2024

Thanks @zhjwpku !

What I would be interested to know is the compile and link times, respectively, for both GTest and Catch2.
My experience in Arrow is that GTest's main header gtest.h imposes a large compilation overhead when you have many independent test modules, and I'm curious if Catch2 improves the situation.

@zhjwpku
Copy link
Contributor Author

zhjwpku commented Dec 16, 2024

Thanks @zhjwpku !

What I would be interested to know is the compile and link times, respectively, for both GTest and Catch2. My experience in Arrow is that GTest's main header gtest.h imposes a large compilation overhead when you have many independent test modules, and I'm curious if Catch2 improves the situation.

I did 3 tests:

  1. using -E to see the preprocessed file size
    • 4301941 -- gtest
    • 4086315 -- catch2
  2. using time to see the compile time
    • 0.40s user 0.06s system 78% cpu 0.587 total -- gtest
    • 0.32s user 0.06s system 76% cpu 0.494 total -- catch2
  3. using time to see the link time
    • 0.04s user 0.02s system 79% cpu 0.080 total -- gtest
    • 0.09s user 0.03s system 98% cpu 0.120 total -- catch2

As the results shows, catch2 is better at preprocessed file size and compile time, but gtest is better when it comes to link time. I don't think the above tests is adequate to prove which is better than the other, since the UT in this PR is fairly simple.

I think we should focus closely on the specific benefits that the test framework is expected to deliver for this project.

@pitrou
Copy link
Member

pitrou commented Dec 16, 2024

Thanks for the number @zhjwpku .

As the results shows, catch2 is better at preprocessed file size and compile time, but gtest is better when it comes to link time. I don't think the above tests is adequate to prove which is better than the other, since the UT in this PR is fairly simple.

Well, it does tell us that they are close in terms of inclusion overhead, which was my concern.

I think we should focus closely on the specific benefits that the test framework is expected to deliver for this project.

Agreed. Is there a detailed (hopefully unbiased) comparison somewhere?

@zhjwpku
Copy link
Contributor Author

zhjwpku commented Dec 16, 2024

Thanks for the number @zhjwpku .

As the results shows, catch2 is better at preprocessed file size and compile time, but gtest is better when it comes to link time. I don't think the above tests is adequate to prove which is better than the other, since the UT in this PR is fairly simple.

Well, it does tell us that they are close in terms of inclusion overhead, which was my concern.

I think we should focus closely on the specific benefits that the test framework is expected to deliver for this project.

Agreed. Is there a detailed (hopefully unbiased) comparison somewhere?

I found this link: https://yurigeronimus.medium.com/guide-for-choosing-a-test-framework-for-your-c-project-2a7741b53317
Not sure if it is biased though.

@pitrou
Copy link
Member

pitrou commented Dec 16, 2024

I found this link: https://yurigeronimus.medium.com/guide-for-choosing-a-test-framework-for-your-c-project-2a7741b53317

Seems like a lot of fluff with no substance, unfortunately.

@zhjwpku zhjwpku mentioned this pull request Dec 23, 2024
@zhjwpku zhjwpku force-pushed the add_unittest_framework branch 2 times, most recently from d3cee92 to 55eb59b Compare December 23, 2024 13:04
@zhjwpku zhjwpku requested a review from wgtmac December 24, 2024 13:19
README.md Outdated Show resolved Hide resolved
test/CMakeLists.txt Outdated Show resolved Hide resolved
@zhjwpku zhjwpku requested a review from wgtmac December 25, 2024 02:14
Copy link
Member

@wgtmac wgtmac left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

We can always improve it when we have better ideas.

@zhjwpku
Copy link
Contributor Author

zhjwpku commented Dec 27, 2024

@raulcd @Fokko can you please review this PR?

@wgtmac
Copy link
Member

wgtmac commented Dec 27, 2024

BTW, should we run ctest in build_iceberg.sh (or a separate script) from pre-commit check in the CI?

@zhjwpku
Copy link
Contributor Author

zhjwpku commented Dec 27, 2024

BTW, should we run ctest in build_iceberg.sh (or a separate script) from pre-commit check in the CI?

I've added the ctest command in build_iceberg.sh, this will run test on all platforms we are targeting.

@zhjwpku zhjwpku force-pushed the add_unittest_framework branch from 3fe843c to a307f41 Compare December 27, 2024 07:53
Copy link
Member

@raulcd raulcd left a comment

Choose a reason for hiding this comment

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

Thanks for the work @zhjwpku ! 👍

ci/scripts/build_iceberg.sh Show resolved Hide resolved
Per the discussion in apache#12, we agreed on using GTest as iceberg-cpp's
unit test framework.

Signed-off-by: Junwang Zhao <[email protected]>
@zhjwpku zhjwpku force-pushed the add_unittest_framework branch from a307f41 to 17de3cb Compare January 11, 2025 02:44
@wgtmac
Copy link
Member

wgtmac commented Jan 12, 2025

@Fokko @Xuanwo Should we merge this?

@wgtmac
Copy link
Member

wgtmac commented Jan 12, 2025

@zhjwpku It is better to change the PR title to reflect that we have chosen googletest?

@zhjwpku zhjwpku changed the title Integrate Test Framework Integrate Googletest as Test Framework Jan 13, 2025
@zhjwpku
Copy link
Contributor Author

zhjwpku commented Jan 13, 2025

@zhjwpku It is better to change the PR title to reflect that we have chosen googletest?

Updated the PR title as well as the PR's first message, thanks for the suggestion ;)

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @zhjwpku for working on this, and thank you all for the review. Let's move.

@Xuanwo Xuanwo merged commit c8d8af9 into apache:main Jan 13, 2025
@zhjwpku zhjwpku deleted the add_unittest_framework branch January 13, 2025 02:23
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