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

Added Common++ unit test project based on Gtest. #1507

Open
wants to merge 40 commits into
base: dev
Choose a base branch
from

Conversation

Dimi1010
Copy link
Collaborator

@Dimi1010 Dimi1010 commented Jul 21, 2024

Summary

This PR introduces dedicated unit tests for the Common++ module based on the GoogleTest framework as well as several minor improvements.

What's Changed?

Unit tests

  • A new project Test/Common++ has been added with unit tests.
    • Tested Classes
      • IPAddress, IPv4Address, IPv6Address - tests provided in IPAddressTests.cpp
      • IPNetwork, IPv4Network, IPv6Network - tests provided in IPAddressTests.cpp
      • MacAddress - tests provided in MacAddressTests.cpp
      • PointereVector<T> - tests provided in PointerVectorTests.cpp
      • LRUList<T> - tests provided in LRUListTests.cpp
    • Tested Functions
      • byteArrayToHexString - tests provided in GeneralUtilsTests.cpp
      • hexStringToByteArray - tests provided in GeneralUtilsTests.cpp
      • cross_platform_memmem - tests provided in GeneralUtilsTests.cpp
      • align - tests provided in GeneralUtilsTests.cpp
  • Added machinery to fetch googletest (v1.12.0) during the cmake build process.
  • Enabled googletest macro definitions for pre-commit's cppcheck step.
  • Added custom fixture MemoryLeakDetectorTest that can be used to extend a unit test to check for memory leaks.
    • The fixture uses the existing MemPlumber implementation to detect memory leaks.

Fixes and additions

  • Added a new constructor to MacAddress that takes an std::array<uint8_t, 6>.
  • Added a new namespace pcpp::literals containing literals for quickly constructing IPv4Address (_ipv4) and IPv6Address (_ipv6).
  • Moved std::ostream operator << overloads for inside the pcpp namespace to fix ADL resolution issues for the following classes:
    • IPAddress
    • IPv4Address
    • IPv6Address
    • IPNetwork
    • IPv4Network
    • IPv6Network
    • MacAddress

…L as it does not consider functions in the global namespace.

- Pulled operator<< for IpAddress into the `pcpp` namespace.
- Pulled operator<< for IpV4Address into the `pcpp` namespace.
- Pulled operator<< for IpV6Address into the `pcpp` namespace.
- Pulled operator<< for IpNetwork into the `pcpp` namespace.
- Pulled operator<< for IpV4Network into the `pcpp` namespace.
- Pulled operator<< for IpV6Network into the `pcpp` namespace.
- Pulled operator<< for MacAddress into the `pcpp` namespace.
Copy link

codecov bot commented Sep 8, 2024

Codecov Report

Attention: Patch coverage is 5.46022% with 606 lines in your changes missing coverage. Please review.

Project coverage is 81.98%. Comparing base (eaf6133) to head (c20e5d3).

Files with missing lines Patch % Lines
Tests/Common++Test/Tests/IPAddressTests.cpp 0.00% 410 Missing ⚠️
Tests/Common++Test/Tests/PointerVectorTests.cpp 0.00% 66 Missing ⚠️
Tests/Common++Test/Tests/LRUListTests.cpp 0.00% 50 Missing ⚠️
Tests/Common++Test/Tests/MacAddressTests.cpp 0.00% 41 Missing ⚠️
Tests/Common++Test/Tests/GeneralUtilsTests.cpp 0.00% 19 Missing ⚠️
...s/Common++Test/Utils/MemoryLeakDetectorFixture.hpp 0.00% 10 Missing ⚠️
Common++/header/IpAddress.h 83.33% 5 Missing and 1 partial ⚠️
Common++/header/MacAddress.h 55.55% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1507      +/-   ##
==========================================
- Coverage   83.14%   81.98%   -1.16%     
==========================================
  Files         276      282       +6     
  Lines       47315    47978     +663     
  Branches     9528     9893     +365     
==========================================
- Hits        39338    39334       -4     
- Misses       7089     7944     +855     
+ Partials      888      700     -188     
Flag Coverage Δ
fedora40 73.56% <3.70%> (-1.68%) ⬇️
macos-13 79.42% <2.21%> (-1.26%) ⬇️
macos-14 79.42% <2.21%> (-1.26%) ⬇️
macos-15 79.39% <2.21%> (-1.26%) ⬇️
mingw32 70.63% <80.76%> (-0.22%) ⬇️
mingw64 70.60% <80.76%> (-0.22%) ⬇️
npcap 85.12% <100.00%> (-0.09%) ⬇️
rhel94 73.45% <3.70%> (-1.63%) ⬇️
ubuntu2004 57.57% <3.69%> (-1.10%) ⬇️
ubuntu2004-zstd 57.67% <3.69%> (-1.12%) ⬇️
ubuntu2204 73.37% <3.70%> (-1.65%) ⬇️
ubuntu2204-icpx 61.28% <37.83%> (-0.24%) ⬇️
ubuntu2404 73.60% <3.63%> (-1.66%) ⬇️
unittest 81.98% <5.46%> (-1.16%) ⬇️
windows-2019 85.23% <100.00%> (-0.02%) ⬇️
windows-2022 85.25% <100.00%> (-0.02%) ⬇️
winpcap 85.21% <100.00%> (ø)
xdp 49.20% <0.00%> (-1.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Dimi1010 Dimi1010 marked this pull request as ready for review September 8, 2024 11:45
@Dimi1010
Copy link
Collaborator Author

Dimi1010 commented Sep 8, 2024

So far, it hasn't been integrated into the workflow or the codecov as I'm not really sure how precisely that is achieved.

@seladb
Copy link
Owner

seladb commented Oct 14, 2024

@Dimi1010 @tigercosmos @egecetin @clementperon let's discuss this change: if we want to move away from the existing tests infra I think it should be a consternated effort to migrate all tests to Gtest. It'll take time, but may be feasible if all of us are working on it together. The question is: can we all commit to work on it before the next version?

@egecetin
Copy link
Collaborator

@seladb What is the expected date of the new release? If it will be about 25.04 it is easy to work on it by planning. The more important question is, we talked about the gtest in #1463 to improve performance. Is everyone thinking moving Gtest is better, or anyone have any doubts (for performance, maintainability or maybe interface related)? It is better to share it if have any, before starting to work.

In my opinion, it will reduce maintaining requirements because of gtest has lots of developers and they already fix their bugs and has a good interface for a C++ code and have integration with the external tools/extensions but for performance I don't expect a drastically improvement. So, I think moving to gtest is feasible.

@seladb
Copy link
Owner

seladb commented Oct 15, 2024

@seladb What is the expected date of the new release? If it will be about 25.04 it is easy to work on it by planning. The more important question is, we talked about the gtest in #1463 to improve performance. Is everyone thinking moving Gtest is better, or anyone have any doubts (for performance, maintainability or maybe interface related)? It is better to share it if have any, before starting to work.

In my opinion, it will reduce maintaining requirements because of gtest has lots of developers and they already fix their bugs and has a good interface for a C++ code and have integration with the external tools/extensions but for performance I don't expect a drastically improvement. So, I think moving to gtest is feasible.

I hope we can release faster, hopefully before 25.04. To be honest, I don't think performance is a good enough reason to switch to Gtest. The main reason would be having more standardize code and less custom-made stuff. But as I mentioned, it will require a lot of work (and most of it is boring - rewrite tests...), so the question is - can we commit to that and do we think it's worth the effort?

@Dimi1010
Copy link
Collaborator Author

Dimi1010 commented Oct 15, 2024

I think it will be beneficial overall. The rollout can be done in phases (project by project) similar to the way we did the clang-format roll out. I could probably work through it slowly as I get free time.

Also, a lot of the current tests could probably benefit by the expanded non-critical failure support, I know we have something similar in the PTF framework but its not exactly the same. In GTest a non-critical failure (EXPECT_*) the test is still marked as failed, but continues to run to catch other errors in the unit test if they exist. Fatal failures (ASSERT_*) are mostly used for conditions that invalidate the rest of the unit test. The fact that the current framework only catches 1 error per unit test has been somewhat annoying to me.

@seladb
Copy link
Owner

seladb commented Oct 16, 2024

I think it will be beneficial overall. The rollout can be done in phases (project by project) similar to the way we did the clang-format roll out. I could probably work through it slowly as I get free time.

The difference between this and clang-format is that this project will be much slower and more tedious because we need to go over all the tests and port them one by one. Maybe we can open a separate branch for it so we don't merge unfinished work to dev and block master?

Anyway, this will require more time and commitment from everyone, and I wonder if we can/want to do it.

However, if we open a separate branch and it doesn't block anything then the risk is quite minimal 🤔

Also, a lot of the current tests could probably benefit by the expanded non-critical failure support, I know we have something similar in the PTF framework but its not exactly the same. In GTest a non-critical failure (EXPECT_) the test is still marked as failed, but continues to run to catch other errors in the unit test if they exist. Fatal failures (ASSERT_) are mostly used for conditions that invalidate the rest of the unit test. The fact that the current framework only catches 1 error per unit test has been somewhat annoying to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants