feat: add test support library and fixtures - #913
niteshpurohit wants to merge 3 commits into
Conversation
2bbbf8d to
1aeafaa
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical portability and cleanup defects, plus test-wrapper validation gaps, remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a reusable POSIX test-support library, fixtures, stable runner exit codes, and CMake/CTest integration.
Changes:
- Added temporary-directory and socket fixtures.
- Migrated existing tests to the shared runner.
- Added
scripts/testand wrapper validation. - Integrated support targets through CMake.
File summaries
| File | Summary | Findings |
|---|---|---|
tests/support/runner_contract.cpp |
Tests runner exit codes. | None. |
tests/support/laghu_test_support.hpp |
Declares test-support APIs. | None. |
tests/support/laghu_test_support.cpp |
Implements fixtures and runner. | Critical: Define the required feature-test macro before system headers (line 14, 1 vote). Critical: Clear the temporary path when mkdtemp fails to avoid deleting pre-existing paths (lines 205 and 274, 2 votes). |
tests/support/fixtures.cpp |
Tests fixture behavior. | None. |
tests/os/iovec_translation.cpp |
Migrates the iovec test. | None. |
tests/core/result_errors.cpp |
Migrates the result error test. | None. |
tests/adapters/crypto_provider.cpp |
Migrates the crypto test. | None. |
scripts/test |
Provides CTest execution and filtering. | Moderate: Reject empty or unconfigured test trees using CTest’s no-tests error option (line 62, 1 vote). |
CMakeLists.txt |
Builds and links test targets. | None. |
cmake/LaghuToolchain.cmake |
Registers wrapper and validation tests. | None. |
cmake/ExpectTestWrapper.cmake |
Validates wrapper behavior. | Moderate: Replace the hard-coded missing path with a guaranteed non-directory or test-owned path (line 23, 1 vote). |
Review details
Suppressed comments (3)
cmake/ExpectTestWrapper.cmake:23
- This assertion is not deterministic because it relies on the hard-coded
/tmp/laghu-wrapper-missingpath not existing. If that directory is present on a developer or CI host,scripts/testproceeds to invoke CTest and the expected 66 result can fail; use a guaranteed non-directory or test-owned missing path instead.
laghu_expect_test_wrapper(66 --build /tmp/laghu-wrapper-missing)
scripts/test:62
- A directory-only check is not enough to prove that this is a configured test tree: CTest normally exits successfully with
No tests were found!!!for an empty/unconfigured directory. As a result,scripts/test --build <existing-empty-dir>can report success without running the suite. Pass CTest's no-tests error option so a full or filtered run cannot silently pass without executing a test.
set -- ctest --test-dir "$build_directory" --output-on-failure
tests/support/laghu_test_support.cpp:276
- If the requested pathname already exists,
bindfails withEADDRINUSE, but the localUnixSocketstill haspath_size_set, so its destructor unlinks that pre-existing entry. This can delete a caller-created socket or regular file when a fixture name collides. Only retain the path for destructor cleanup afterbindsucceeds (while still retaining it through a laterlistenfailure).
if (::bind(socket.fd_, reinterpret_cast<const sockaddr*>(&address), address_size) != 0 ||
::listen(socket.fd_, 8) != 0) {
return std::unexpected{system_failure()};
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Introduced a new static library `laghu_test_support` to provide common testing utilities. - Created test fixtures for temporary directories and socket connections to facilitate testing. - Implemented a test runner with stable exit codes to validate test outcomes. - Updated existing tests to utilize the new test support library for better organization and maintainability. - Added a script for running tests with options for build directory and filtering. - Enhanced CMake configuration to include new test executables and link against the test support library. closes: #73
- Replace std::snprintf with std::to_chars for better performance and safety. - Simplify the logic for creating nested directories by using a suffix array. - Ensure proper error handling and path size management during directory creation.
08c1d33 to
a24a4db
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Critical build and fixture-ownership issues, plus portability and nondeterministic-test findings, remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (7)
Previously missed (1) — in code that hasn't changed since the last review.
cmake/ExpectTestWrapper.cmake:23
- This expectation is nondeterministic because
/tmp/laghu-wrapper-missingmay already exist from a prior run or the host environment. In that case the wrapper proceeds toctestand no longer returns the expected 66; use a path that cannot be a directory or create an isolated missing path before this assertion.
tests/support/fixtures.cpp:128
- This BSD
sockaddr_inused for the UDP destination likewise leavessin_lenunset. Populate the platform-specific length field beforesendtoso the local datagram fixture works on Darwin and FreeBSD.
sockaddr_in address{};
address.sin_family = AF_INET;
address.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
address.sin_port = htons(receiver->port());
tests/support/fixtures.cpp:105
- This BSD
sockaddr_inused for the TCP connect hassin_len == 0because the structure is value-initialized. Initialize the platform-specific length field beforeconnect, otherwise the fixture is not portable to the advertised Darwin/FreeBSD targets.
sockaddr_in address{};
address.sin_family = AF_INET;
address.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
address.sin_port = htons(listener->port());
tests/support/fixtures.cpp:82
- The Unix client address also leaves the BSD
sun_lenfield zero. Once the listener is made portable, this connect path can still fail on Darwin/FreeBSD; initialize the platform-specific length field beforeconnect.
sockaddr_un address{};
address.sun_family = AF_UNIX;
if (listener->path().size() >= sizeof(address.sun_path)) {
(void)::close(client);
return false;
}
std::memcpy(address.sun_path, listener->path().data(), listener->path().size() + 1U);
const auto address_size = static_cast<socklen_t>(offsetof(sockaddr_un, sun_path) +
listener->path().size() + 1U);
tests/support/laghu_test_support.cpp:140
- This zero-initialized
sockaddr_inleaves the BSDsin_lenfield unset. On Darwin and FreeBSD the field is part of the native address ABI and can cause the loopback bind to fail, so initialize it under the BSD platform guards before callingbind.
address.sin_family = AF_INET;
address.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
address.sin_port = 0;
tests/support/laghu_test_support.cpp:273
- On Darwin and FreeBSD,
sockaddr_unhas a native length field, but this address leaves it zero. The Unix-socket fixture can therefore fail with an invalid address even though the pathname and passed length are correct; setsun_lento the computed address length on those platforms.
sockaddr_un address{};
address.sun_family = AF_UNIX;
std::memcpy(address.sun_path, socket.path_, socket.path_size_ + 1U);
const auto address_size = static_cast<socklen_t>(offsetof(sockaddr_un, sun_path) + socket.path_size_ + 1U);
tests/support/laghu_test_support.cpp:14
- On the project's strict C++23 builds, this source does not define an X/Open/POSIX feature-test macro before including
<ftw.h>and<cstdlib>. On glibc,nftw, theFTW_*constants, andmkdtempare hidden unless the corresponding feature level is enabled, so a normal Linux build fails with undeclared API/constant errors. Define a platform-appropriate feature level for this target before any system header, or replace these gated APIs.
#include <ftw.h>
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical portability issues and moderate fixture cleanup and validation issues remain.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
tests/support/fixtures.cpp:176
- Because
||short-circuits, a failed join offirst_threadskips joiningsecond_thread, which may still be running while this function returns. Attempt both joins before evaluating the combined result so the fixture never leaves a worker thread unjoined.
if (::pthread_join(first_thread, nullptr) != 0 || ::pthread_join(second_thread, nullptr) != 0) {
return false;
tests/support/laghu_test_support.cpp:203
- When any append fails,
directory.path_size_can still be non-zero even thoughmkdtemphas not created this path. The local destructor then callsremove_tree(path_); for example, an overlongTMPDIRcan leavepath_equal to the existing TMPDIR and recursively delete it. Clear the candidate's ownership state before returning (or track ownership separately).
!append_text(directory.path_, sizeof(directory.path_), directory.path_size_, "-XXXXXX")) {
return std::unexpected{FixtureFailure{FixtureError::path_too_long, 0}};
tests/support/laghu_test_support.cpp:243
path_size_only records that a candidate path was assembled; it is not an ownership flag. On a failed bind (includingEADDRINUSE), or even a path-length/socket-creation failure, this destructor unlinks a path it did not create, potentially deleting another fixture's socket or file. Add an ownership flag and set it only afterbindsucceeds.
if (path_size_ != 0U) {
(void)::unlink(path_);
- Files reviewed: 11/11 changed files
- Comments generated: 3
- Review effort level: Lite
- Added TEST_BUILD and TEST_ROOT variables to ExpectTestWrapper for better configuration. - Implemented failure handling in laghu_expect_test_wrapper_failure function. - Created temporary directories for tests to ensure isolation and prevent conflicts. - Updated test script to suppress error output for non-existent tests. - Introduced new checks for temporary directory and Unix socket ownership in fixtures.
laghu_test_supportto provide common testing utilities.closes: #73