Added Common++ unit test project based on Gtest.

[Dimi1010]

Summary

This PR introduces dedicated unit tests for the Common++ module based on the GoogleTest framework as well as several minor improvements. The dependency is resolved via find_package and conditionally via FetchContent if find_package fails.

What's Changed?

New dependency - GoogleTest

Added GoogleTest as dependency to the project when PCAPPP_BUILD_TESTS is set to ON. The dependency resolution happens in one of two ways: find_package or FetchContent. The project prioritizes the result of find_package, to allow environments to supply their own.

Restricted FetchContent dependency resolution.

For environments that disallow or discourage the use of FetchContent (e.g. brew, vcpkg, etc) an additional project option PCAPPP_ALLOW_FETCH_CONTENT (default: ON) is added. Setting the option to OFF would disable the automatic fetching and require all dependencies that are optionally supplied by fetch content be resolvable by find_package in the build environment.

NB: The googletest dependency is still declared inside the FetchContent infrastructure, but the option disables calling FetchContent_MakeAvailable effectively disabling the fetch operation.

New 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.16.0) during the cmake build process.
• Enabled googletest macro definitions for pre-commit's cppcheck step.
• Added listener that can be used to extend a unit test to check for memory leaks.

Fixes and additions

• Added #include <array> to MacAddress.h to fix transitive include.
• Added a new namespace pcpp::literals containing literals for quickly constructing IPv4Address (_ipv4) and IPv6Address (_ipv6).

[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.

[seladb]

As the TODO comment mentions, package managers don't approve packages that use FetchContent_Declare, we already had this experience before...

[Dimi1010]

Yeah. Tbh, we could possibly have a copy of the lib in 3rdParty to build the tests with.

I am unsure if we could use the brew version since we need to use up to 1.16 as 1.17 (current newest) requires cpp17.

[seladb]

yeah, putting it in 3rdParty is the only option to keep PcapPlusPlus in package managers.

Though I'm still not sure that we want to merge this PR. Having 2 test frameworks doesn't look like a good idea, and converting all of our tests to Gtest will be an extremely hard task

[Dimi1010]

I did start another branch [1] off of this one, for the Packet++ tests, to see how it would go. You can take a look, if you want. The new project is Tests/Packet++Test-google.

It actually mostly involves trivial changes, like macro replacements.

• PTF_TEST_CASE -> TEST(Fixture, TestName)
• PTF_ASSERT_EQUAL -> ASSERT_EQ / EXPECT_EQ
• PTF_ASSERT_NULL -> ASSERT_EQ(statement, nullptr)
• PTF_ASSERT_NOT_NULL -> ASSERT_NE(statement, nullptr)

The only non-trivial assertion so far has been PTF_ASSERT_BUF_COMPARE which was replaced by EXPECT_TRUE(test::BuffersMatch(actualBuf, actualBufSize, expectedBuf, expectedBufSize)) which ended up being a more comprehensive check IMO. [2].

The loading utilities needed a revamp, as I didn't want to use macros everywhere and the current ones had a limitation of always using the current working directory (has issues with out of source builds).

For the majority of the cases the code:

timeval time;
gettimeofday(&time, nullptr);

READ_FILE_AND_CREATE_PACKET(1, "PacketExamples/TcpPacketNoOptions.dat");
// rest of test case

ended up being a one liner:

// std::unique_ptr<RawPacket>
auto rawPacket1 = test::createPacketFromHexResource("PacketExamples/TcpPacketWithOptions3.dat", /* optional packet factory */, /* optional data loader */);

[seladb]

I'm not sure how we'll be able to review these huge PRs...

[seladb]

Ah ok, that makes sense. I just wanted to confirm that we're not missing anything.

So I guess we decided to go with Gtest?

[Dimi1010]

@seladb I guess so.

IMO, the first thing to do would be to disable building the tests in vcpkg and homebrew install scenarios, via a revision to the portfile / bottle, so we know it can work fine as is, before adding fetch content to the test infrastructure.

Unless anyone has any objections? @egecetin @clementperon @tigercosmos

[seladb]

Yes I think that's a good idea 👍

[Dimi1010]

Opened a PR to disable the tests in vcpkg here microsoft/vcpkg#46941.

Also found a bug when examining the build. It placed the test executables inside the vcpkg_installed/vcpkg/blds/pcapplusplus/src/***.clean which is a bug, IMO.

Our test projects don't really support out of source builds currently, so I plan on fixing this in the Gtest rework.

[Dimi1010]

Homebrew has also been updated Homebrew/homebrew-core#233719

Conan needs no update as we already don't build tests there on install.

[Dimi1010]

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

[Dimi1010]

Should probably change the namespace to pcpp_tests...