Added Common++ unit test project based on Gtest.

[Dimi1010]

trafficstars

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

[codecov[bot]]

Codecov Report

:x: Patch coverage is 97.01327% with 27 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 83.68%. Comparing base (0132d27) to head (0752481).

Files with missing lines	Patch %	Lines
Tests/Common++Test/Tests/IPAddressTests.cpp	97.30%	13 Missing :warning:
Tests/Common++Test/Utils/MemoryLeakListener.cpp	0.00%	9 Missing :warning:
Tests/Common++Test/Tests/MacAddressTests.cpp	96.66%	3 Missing :warning:
Tests/Common++Test/Tests/LoggerTests.cpp	97.40%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1507      +/-   ##
==========================================
+ Coverage   83.41%   83.68%   +0.27%     
==========================================
  Files         311      322      +11     
  Lines       55019    55511     +492     
  Branches    11816    11822       +6     
==========================================
+ Hits        45892    46453     +561     
+ Misses       7852     7826      -26     
+ Partials     1275     1232      -43     

Flag	Coverage Δ
alpine320	76.21% <98.52%> (+0.32%)	:arrow_up:
fedora42	75.77% <98.51%> (-0.07%)	:arrow_down:
macos-14	81.89% <96.89%> (+0.39%)	:arrow_up:
macos-15	81.88% <96.89%> (+0.37%)	:arrow_up:
mingw32	70.05% <ø> (-0.48%)	:arrow_down:
mingw64	70.03% <ø> (-0.38%)	:arrow_down:
npcap	?
rhel94	75.81% <98.51%> (-0.06%)	:arrow_down:
ubuntu2004	59.98% <98.57%> (-0.15%)	:arrow_down:
ubuntu2004-zstd	60.08% <98.57%> (-0.15%)	:arrow_down:
ubuntu2204	75.75% <98.51%> (-0.05%)	:arrow_down:
ubuntu2204-icpx	57.92% <ø> (-2.63%)	:arrow_down:
ubuntu2404	75.84% <98.53%> (-0.05%)	:arrow_down:
ubuntu2404-arm64	75.88% <98.53%> (+0.32%)	:arrow_up:
unittest	83.68% <97.01%> (+0.27%)	:arrow_up:
windows-2022	85.42% <ø> (+0.16%)	:arrow_up:
windows-2025	85.45% <ø> (+0.11%)	:arrow_up:
winpcap	85.45% <ø> (-0.09%)	:arrow_down:
xdp	54.49% <98.51%> (+0.96%)	:arrow_up:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Dimi1010]

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]

@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]

@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]

@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]

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]

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.

[tigercosmos]

@Dimi1010 Do you want to continue this?

[Dimi1010]

@Dimi1010 Do you want to continue this?

Eh... eventually... Don't really have time for it now tho, so gonna close it for the moment.

[Dimi1010]

@seladb @egecetin @tigercosmos @clementperon

So... minor issue with the google test integration. The framework allocates to heap during unit tests. Due to that when the per-test memory leak tracking is added like we have in the current tests, it gives false positives. Another thing is that the MemPlumber::stopAndFreeMemory after the test essentially corrupts the heap, leading to access violations when the framework searches for what it has allocated during the test.

I don't think we can run the leak tracking on per test level as is. It might be workable on a global level though.

I am open to suggestions on how to proceed.

[tigercosmos]

I am okay to move the memory-leaking test to global as long as it helps to find out issues.

[Dimi1010]

Update on the memory leak infrastructure: It still doesn't work directly on global level as gtest uses thread local variables that end up lazy initializing at runtime, but get released on program exit. :/

[tigercosmos]

Not sure if I fully understood. Aren't all tests passed now?

[Dimi1010]

Not sure if I fully understood. Aren't all tests passed now?

Tests pass because the memleak is disabled in the gtest project currently. [1]

[Dimi1010]

@tigercosmos In my tests, it didn't work having the memory tracking encompass all the tests as a single start/stop pair, either.

That is because gtest appears to allocate on heap from thread local variables during the tests. That memory isn't released until the main thread actually exits when the program shuts down, and gets detected as a false positive. :/ (and leads to crash when the thread actually tries to deallocate)

This is the culprit. From googletest v1.16.0 gtest-port.h

// Implements thread-local storage on Windows systems.
//
//   // Thread 1
//   ThreadLocal<int> tl(100);  // 100 is the default value for each thread.
//
//   // Thread 2
//   tl.set(150);  // Changes the value for thread 2 only.
//   EXPECT_EQ(150, tl.get());
//
//   // Thread 1
//   EXPECT_EQ(100, tl.get());  // In thread 1, tl has the original value.
//   tl.set(200);
//   EXPECT_EQ(200, tl.get());
//
// The template type argument T must have a public copy constructor.
// In addition, the default ThreadLocal constructor requires T to have
// a public default constructor.
//
// The users of a TheadLocal instance have to make sure that all but one
// threads (including the main one) using that instance have exited before
// destroying it. Otherwise, the per-thread objects managed for them by the
// ThreadLocal instance are not guaranteed to be destroyed on all platforms.
//
// Google Test only uses global ThreadLocal objects.  That means they
// will die after main() has returned.  Therefore, no per-thread
// object managed by Google Test will be leaked as long as all threads
// using Google Test have exited when main() returns.

[Dimi1010]

@seladb Updated the PR description, so you might want to reread that. Main update is New dependency - GoogleTest under What's Changed?