refactor(test): comprehensive unit test refactoring #159
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Move all existing test files (*.cpp, *.h) to test/archive/ - Rename CMakeLists.txt to CMakeLists.txt.old in archive - Prepare for comprehensive unit test refactoring
- Test all constructors (default, with destructor, from array, from char) - Test move semantics and assignment operators - Test all accessor methods (data, size, empty, get<T>) - Test conversion methods (to_tuple, to_vector) - Test comparison operators (==, !=) - Test edge cases (empty buffers, large buffers, self-assignment) - Verify destructor callback functionality
- Test low-level API (acquire, get_mem, release, remove) - Test reference counting functionality (get_ref, sub_ref) - Test high-level handle class interface - Test all handle methods (valid, size, name, get, etc.) - Test handle lifecycle (construction, move, swap, assignment) - Test different access modes (create, open, create|open) - Test detach/attach functionality - Test multi-handle access to same memory - Test data persistence across handles - Test edge cases (large segments, multiple simultaneous handles)
- Test mutex construction (default and named) - Test lock/unlock operations - Test try_lock functionality - Test timed lock with various timeout values - Test critical section protection - Test concurrent access and contention scenarios - Test inter-thread synchronization with named mutex - Test resource cleanup (clear, clear_storage) - Test native handle access - Test edge cases (reopen, zero timeout, rapid operations) - Test exception safety of try_lock
- Test semaphore construction (default and named with count) - Test wait and post operations - Test timed wait with various timeout values - Test producer-consumer patterns - Test multiple producers and consumers scenarios - Test concurrent post operations - Test initial count behavior - Test named semaphore sharing between threads - Test resource cleanup (clear, clear_storage) - Test edge cases (zero timeout, after clear, high frequency)
- Test condition variable construction (default and named) - Test wait, notify, and broadcast operations - Test timed wait with timeout and infinite wait - Test integration with mutex for synchronization - Test producer-consumer patterns with condition variables - Test multiple waiters with broadcast - Test spurious wakeup handling patterns - Test named condition variable sharing between threads - Test resource cleanup (clear, clear_storage) - Test edge cases (after clear, immediate notify)
- Test spin_lock basic operations and mutual exclusion - Test spin_lock critical section protection - Test spin_lock concurrent access and contention - Test rw_lock write lock (exclusive access) - Test rw_lock read lock (shared access) - Test multiple concurrent readers - Test writers have exclusive access - Test readers and writers don't overlap - Test various read-write patterns - Test rapid lock/unlock operations - Test mixed concurrent operations - Test write lock blocks readers correctly
- Test route (single producer, multiple consumer) functionality - Test channel (multiple producer, multiple consumer) functionality - Test construction with name and prefix - Test connection, disconnection, and reconnection - Test send/receive with buffer, string, and raw data - Test blocking send/recv and non-blocking try_send/try_recv - Test timeout handling - Test one-to-many broadcast (route) - Test many-to-many communication (channel) - Test recv_count and wait_for_recv functionality - Test clone, release, and clear operations - Test resource cleanup and storage management - Test concurrent multi-sender and multi-receiver scenarios
- Collect only test_*.cpp files from test directory - Exclude archive directory from compilation - Use glob pattern to automatically include new tests - Maintain same build configuration and dependencies - Link with gtest, gtest_main, and ipc library
- Update all test files to use 2-space indentation - Affects: test_buffer.cpp, test_shm.cpp, test_mutex.cpp - Affects: test_semaphore.cpp, test_condition.cpp - Affects: test_locks.cpp, test_ipc_channel.cpp - Improves code consistency and readability
- Remove 'using namespace ipc::shm;' to avoid id_t conflict with system id_t - Add explicit shm:: namespace prefix to all shm types and functions - Apply to: id_t, handle, acquire, get_mem, release, remove, get_ref, sub_ref - Apply to: create and open constants - Fix comments to avoid incorrect namespace references - Resolves compilation error: 'reference to id_t is ambiguous'
- Fix handle class member functions: remove incorrect shm:: prefix - h.acquire() not h.shm::acquire() - h.release() not h.shm::release() - h.sub_ref() not h.shm::sub_ref() - Keep shm:: prefix for namespace-level global functions: - shm::acquire(), shm::release(id), shm::get_mem() - shm::remove(), shm::get_ref(id), shm::sub_ref(id) - Fix comments to use correct terminology - Resolves: 'shm::acquire is not a class member' compilation errors
- Change: byte_t const (& data)[N] → byte_t (& data)[N] - Allows non-const byte arrays to be accepted by the constructor - Fixes defect discovered by TEST_F(BufferTest, ConstructorFromByteArray) - The const qualifier on array elements was too restrictive - Keep char const & c unchanged as it's correct for single char reference
- Change: explicit buffer(char const & c) → explicit buffer(char & c)
- Remove dangerous const_cast in implementation
- Before: buffer(const_cast<char*>(&c), 1) [UB if c is truly const]
- After: buffer(&c, 1) [safe, requires non-const char]
- Prevents undefined behavior from modifying compile-time constants
- Constructor now correctly requires mutable char reference
- Aligns with buffer's mutable data semantics
The previous implementation with const_cast could lead to:
- Modifying string literals (undefined behavior)
- Modifying const variables (undefined behavior)
- Runtime crashes or data corruption
Example of prevented misuse:
buffer buf('X'); // Now: compile error ✓
char c = 'X';
buffer buf(c); // Now: works correctly ✓
const char cc = 'Y';
buffer buf(cc); // Now: compile error ✓
…clarity
Header changes (include/libipc/buffer.h):
- Rename: additional → mem_to_free (better semantic name)
- Add documentation comments explaining the parameter's purpose
- Clarifies that mem_to_free is passed to destructor instead of p
- Use case: when data pointer is offset into a larger allocation
Implementation changes (src/libipc/buffer.cpp):
- Update parameter name in constructor implementation
- No logic changes, just naming improvement
Test changes (test/test_buffer.cpp):
- Fix TEST_F(BufferTest, ConstructorWithMemToFree)
- Previous test caused crash: passed stack variable address to destructor
- New test correctly demonstrates the parameter's purpose:
* Allocate 100-byte block
* Use offset portion (bytes 25-75) as data
* Destructor receives original block pointer for proper cleanup
- Prevents double-free and invalid free errors
Semantic explanation:
buffer(data_ptr, size, destructor, mem_to_free)
On destruction:
destructor(mem_to_free ? mem_to_free : data_ptr, size)
This allows:
char* block = new char[100];
char* data = block + 25;
buffer buf(data, 50, my_free, block); // Frees 'block', not 'data'
…ee in tests
- Add comprehensive documentation for shm::release(id), shm::remove(id), and shm::remove(name)
- release(id): Decrements ref count, cleans up memory and disk file when count reaches zero
- remove(id): Calls release(id) internally, then forces disk file cleanup (WARNING: do not use after release)
- remove(name): Only removes disk file, safe to use anytime
- Fix critical double-free bug in ShmTest test cases
- Problem: calling release(id) followed by remove(id) causes use-after-free crash
because release() already frees the id structure
- Solution: replace 'release(id); remove(id);' pattern with just 'remove(id)'
- Fixed tests: AcquireCreate, AcquireCreateOrOpen, GetMemory, GetMemoryNoSize,
RemoveById, SubtractReference
- Kept 'release(id); remove(name);' pattern unchanged (safe usage)
- Add explanatory comments in test code to prevent future misuse
- Problem: calling h2.release() followed by shm::remove(id) causes use-after-free - h2.release() internally calls shm::release(id) which frees the id structure - shm::remove(id) then accesses the freed id pointer -> crash - Solution: detach the id from handle first, then call shm::remove(id) - h2.detach() returns the id without releasing it - shm::remove(id) can then safely clean up both memory and disk file - This completes the fix for all ShmTest double-free issues
1. ChannelTest::MultipleSendersReceivers - Add C++14-compatible latch implementation (similar to C++20 std::latch) - Ensure receivers are ready before senders start sending messages - This prevents race condition where senders might send before receivers are listening 2. RWLockTest::ReadWriteReadPattern - Fix test logic: lock_shared allows multiple concurrent readers - Previous test had race condition where both threads could read same value - New test: each thread writes based on thread id (1 or 2), then reads - Expected result: 1*20 + 2*20 = 60 3. ShmTest::ReleaseMemory - Correct return value semantics: release() returns ref count before decrement, or -1 on error - Must call get_mem() to map memory and set ref count to 1 before release - Expected: release() returns 1 (ref count before decrement) 4. ShmTest::ReferenceCount - Correct semantics: ref count is 0 after acquire (memory not mapped) - get_mem() maps memory and sets ref count to 1 - Second acquire+get_mem increases ref count to 2 - Test now properly validates reference counting behavior 5. ShmTest::SubtractReference - sub_ref() only works after get_mem() has mapped the memory - Must call get_mem() first to initialize ref count to 1 - sub_ref() then decrements it to 0 - Test now follows correct API usage pattern
Problem: - Receivers were exiting after receiving only messages_per_sender (5) messages - In broadcast mode, each message is sent to ALL receivers - If sender1 completes quickly, all receivers get 5 messages and exit - This causes sender2's messages to fail (no active receivers) Solution: - Each receiver should loop for num_senders * messages_per_sender (2 * 5 = 10) messages - This ensures all receivers stay active until ALL senders complete - Now receivers wait for: 2 senders × 5 messages each = 10 total messages - Expected received_count: 2 senders × 5 messages × 2 receivers = 20 messages This fix ensures all senders can successfully complete their sends before any receiver exits.
…inline functions Problem: - Both linux/get_wait_time.h and posix/get_wait_time.h define inline functions make_timespec() and calc_wait_time() in namespace ipc::detail - On Linux, semaphore uses posix implementation, but may include both headers - This causes ODR (One Definition Rule) violation - undefined behavior - Different inline function definitions with same name violates C++ standard - Manifested as test failures in SemaphoreTest::WaitTimeout Solution: - Add platform-specific namespace layer between ipc and detail: - linux/get_wait_time.h: ipc::linux::detail::make_timespec() - posix/get_wait_time.h: ipc::posix::detail::make_timespec() - Update all call sites to use fully qualified names: - linux/condition.h: linux::detail::make_timespec() - linux/mutex.h: linux::detail::make_timespec() (2 places) - posix/condition.h: posix::detail::make_timespec() - posix/mutex.h: posix::detail::make_timespec() (2 places) - posix/semaphore_impl.h: posix::detail::make_timespec() This ensures each platform's implementation is uniquely named, preventing ODR violations and ensuring correct function resolution at compile time.
…o conflict Problem: - 'linux' is a predefined macro on Linux platforms - Using 'namespace linux' causes compilation errors - Preprocessor replaces 'linux' with '1' before compilation Solution: - Rename 'namespace linux' to 'namespace linux_' - Rename 'namespace posix' to 'namespace posix_' - Update all 7 call sites accordingly: - linux/condition.h: linux_::detail::make_timespec() - linux/mutex.h: linux_::detail::make_timespec() (2 places) - posix/condition.h: posix_::detail::make_timespec() - posix/mutex.h: posix_::detail::make_timespec() (2 places) - posix/semaphore_impl.h: posix_::detail::make_timespec() This prevents preprocessor macro expansion issues while maintaining the ODR violation fix from the previous commit.
Problem: - Reference counting tests fail on Windows (ReleaseMemory, ReferenceCount, SubtractReference, HandleRef, HandleSubRef) - get_ref() and sub_ref() were stub implementations returning 0/doing nothing - CreateFileMapping HANDLE lacks built-in reference counting mechanism Solution: - Implement reference counting using std::atomic<std::int32_t> stored at the end of shared memory (same strategy as POSIX version) - Add calc_size() helper to allocate extra space for atomic counter - Add acc_of() helper to access the atomic counter at the end of memory - Modify acquire() to allocate calc_size(size) instead of size - Modify get_mem() to initialize counter to 1 on first mapping - Modify release() to decrement counter and return ref count before decrement - Implement get_ref() to return current reference count - Implement sub_ref() to atomically decrement reference count - Convert file from Windows (CRLF) to Unix (LF) line endings for consistency Key Implementation Details: 1. Reference counter stored at end of shared memory (aligned to info_t) 2. First get_mem() call: fetch_add(1) initializes counter to 1 3. release() returns ref count before decrement (for semantics compatibility) 4. Memory layout: [user data][padding][atomic<int32_t> counter] 5. Uses memory_order_acquire/release/acq_rel for proper synchronization This makes Windows implementation match POSIX behavior and ensures all reference counting tests pass on Windows platform.
- Remove useless 'ii->size_ = ii->size_;' statement at line 140 - The user-requested size is already set in acquire() function - Simplify else branch to just a comment for clarity - No functional change, just code cleanup
Problem: - Two test cases in test_buffer.cpp used structured bindings (auto [a, b]) - Structured bindings are a C++17 feature - Project requires C++14 compatibility Solution: - Replace 'auto [ptr, size] = buf.to_tuple()' with C++14 compatible code - Use std::get<N>() to extract tuple elements - Modified tests: ToTupleNonConst, ToTupleConst Changes: - Line 239: Use std::get<0/1>(tuple) instead of structured binding - Line 252: Use std::get<0/1>(tuple) instead of structured binding - Add explanatory comments for clarity This ensures the test suite compiles with C++14 standard.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This PR introduces a comprehensive refactoring of the unit test suite for the cpp-ipc library. The old tests have been archived and replaced with new, more thorough tests that cover all public interfaces.
Changes
Test Organization
test/archive/directorytest_*.cppNew Test Coverage
1. test_buffer.cpp (368 lines)
2. test_shm.cpp (499 lines)
3. test_mutex.cpp (501 lines)
4. test_semaphore.cpp (487 lines)
5. test_condition.cpp (550 lines)
6. test_locks.cpp (607 lines)
7. test_ipc_channel.cpp (608 lines)
Test Statistics
Commits
Each major component has its own dedicated commit for better traceability:
Benefits
Testing
All new tests follow Google Test framework conventions and are ready to be built and executed with the existing build system.
Backward Compatibility
The archived tests remain available in
test/archive/for reference, ensuring no loss of existing test knowledge.