Tomas/connection raii initial#413
Draft
ProfessorTom wants to merge 164 commits into
Draft
Conversation
…tart unit testing
- New ctor: TCPThread(host, port, initialPacket, parent) - Creates QSslSocket early - Connects connected/errorOccurred/stateChanged signals - Stores host/port for run() - Sets m_managedByConnection = true
- [[nodiscard]] bool isValid() const - Checks clientConnection null, error codes, socket state - Adds detailed qDebug/qWarning logging for failure reasons
- Add null check for clientConnection before calling close() - Add waitForDisconnected(1000) for clean shutdown when socket exists - Log warning if called with null socket - Prevents potential segfault in dtor when socket not initialized
- Add if (clientConnection) guard before close() - Only call waitForDisconnected() if socket is not UnconnectedState or ClosingState - Log when wait is skipped or socket is null - Eliminates Qt warning about waitForDisconnected in UnconnectedState - Prevents potential null dereference in Connection dtor
…safe cleanup - Update ctor to pass host/port/initialPacket - Add public start() method with isValid() check - Auto-start in ctor - Add send(), isConnected(), isSecure() - Forward key TCPThread signals - Add m_threadStarted flag for safe dtor cleanup
- Add testThreadStartsAndStops() to verify no crash on start/dtor - Give thread time to start with QTest::qWait(500) - Check isConnected() (with fallback for Connecting state)
…plication - Link packet.cpp, tcpthread.cpp, sendpacketbutton.cpp - Add Qt6::Network for QSslSocket/QTcpSocket - Switch to per test class QApplication isolation with runGuiTest/runNonGuiTest
Extracted the decision logic for breaking the persistent connection loop into its own const method. This improves readability of the main loop and makes the behavior unit-testable. - Added TCPThread::shouldBreakPersistentLoop() - Updated test double (TestTcpThreadClass) to support the new method - Added comprehensive unit tests for both persistent and non-persistent cases
…ent connections - Introduce `resetPacketForPersistentLoop()` which clears the sendPacket (via clear()) but explicitly re-sets `persistent = true` so the loop continues correctly. - Add comprehensive unit test `testResetPacketForPersistentLoop()` verifying all fields are reset to defaults (except persistent), and that the timestamp is properly updated to a newer value. - Expose method in TCPThread and TestTcpThreadClass. This prevents stale packet data from persisting across iterations in persistent TCP/UDP loops while maintaining the connection state.
- Replace manual `!sendPacket.persistent` check + else-branch with dedicated `shouldBreakPersistentLoop()` helper - Introduce `resetPacketForPersistentLoop()` to prepare for the next iteration Makes the loop intent clearer and centralizes the persistent-loop decision/reset logic.
- Added BaseTcpThread as the foundation for the upcoming OutgoingTcpThread and IncomingTcpThread split. - Implemented key virtual getters: isValid(), getSocket(), isSocketEncrypted(), getPeerPort(), getLocalPort(), getIPConnectionProtocol(), getPeerAddressAsString(), getSocketPeerAddress(). - Added comprehensive unit tests covering null cases, real sockets, IPv4/IPv6 behavior, and edge cases. - Updated test infrastructure (BaseTcpThreadTestDouble, MockSslSocket, CMake). - Cleaned up Packet::removeIPv6Mapping() for better IPv6 handling. This is an intermediate step toward removing the monolithic TCPThread.
- Added OutgoingTcpThread with main constructor taking socket + Packet - Added validation for destination address and port in constructor - Implemented isValid() with outgoing-specific checks + base delegation - Added getDestinationAddress() and getDestinationPort() getters - Added comprehensive unit tests for constructor validation and isValid() - Updated BaseTcpThread with virtual isSocketValid() for better testability - Added OutgoingTcpThreadTestDouble This establishes the foundation for outgoing client connections.
- Added convenience constructor `OutgoingTcpThread(const Packet& packetToSend, QObject* parent = nullptr)` that internally creates the QSslSocket and delegates to the main constructor. - Updated all unit tests to use the new convenience constructor (much cleaner). - Updated OutgoingTcpThreadTestDouble to support the new constructor. - Removed manual QSslSocket creation from most tests. This improves usability for production code while keeping the socket-taking constructor available for tests.
…nit tests - Introduced `bool Packet::isValidForSending(QString* errorMessage = nullptr)` to centralize validation logic for packets before sending. - Validates the three critical fields: non-empty `toIP`, positive `port`, and non-empty data (`getByteArray()`). - Returns detailed human-readable error messages via optional out-parameter when validation fails. - Added `PacketTests` class with 8 focused unit tests covering all success and failure paths, including error message verification. - Wired new tests into the unit test runner. This enables defensive runtime checks before sending and makes future packet validation consistent and testable.
…tests - Fixed copy constructor to include `resolvedIP` (was missing, causing equality tests to fail) - Implemented `Packet::operator==()` comparing all meaningful fields (timestamp intentionally excluded) - Implemented `Packet::operator!=()` in terms of `operator==()` - Added thorough unit tests: - Identical packets are equal - Timestamp is ignored for equality - Data-driven tests covering all 16 fields in `operator==()` - Tests for `operator!=()` behavior This makes `Packet` a proper value type and improves reliability of tests that compare packets (especially in the new TCP thread code).
…shot tests - Converted `BaseTcpThread::socket` to `std::unique_ptr<QSslSocket>` for clearer ownership - Moved common `sendOutgoingPacket(Packet&)` logic into `BaseTcpThread` - Implemented full `run()` flow in `OutgoingTcpThread` (one-shot TCP send) - Added `closeConnection()`, `prepareOutgoingPacket()`, and debug helpers - Added extensive unit tests covering: - Successful connection + send + cleanup flow - Connection failure path - Error message emission on failure - Method call ordering and delay handling - Multiple `connectionStatus` signals in correct sequence This completes the basic one-shot outgoing TCP functionality with solid test coverage.
…nLoop scaffolding - Moved `createPacketForTest()` and `createMockSocketForTest()` to shared `TestUtils` to eliminate duplication.
…cpThread - Added core persistent loop infrastructure: - `shouldContinuePersistentLoop()` - `shouldStopPersistentConnectionLoop()` - `handlePersistentIdleCase()` (with throttling via `lastIdleStatusEmitTime`) - `persistentConnectionLoop()` main loop with idle path detection - Enhanced BaseTcpThread: - `stop()`, `shouldStop()`, and `interruptibleWaitForReadyRead()` for better control - Added comprehensive unit tests for: - Guard conditions (`shouldContinue*` / `shouldStop*`) - Idle case behavior and throttling - Loop entry/exit logic - Non-idle path skipping - Updated test double (`OutgoingTcpThreadTestDouble`) with counters and helpers for better testability - Minor improvements to socket state handling and test utilities (`createIdlePacketForTest()`) This establishes the foundation for persistent TCP connections.
…ions - Added `readSocketData()` helper in BaseTcpThread - Implemented `buildReceivedPacket()` and `processIncomingData()` in OutgoingTcpThread - Updated `persistentConnectionLoop()` to call `processIncomingData()` when data is available - Reactivated `packetReceived` signal - Added comprehensive unit tests for all new methods (null socket, no data, with data cases) - Updated MockSslSocket and OutgoingTcpThreadTestDouble accordingly
- Added `waitForAndProcessIncomingData()` to handle the "wait for incoming data before sending" behavior - Updated `persistentConnectionLoop()` to branch correctly: - `receiveBeforeSend == true` → waitForAndProcessIncomingData() - Normal idle path otherwise - Added comprehensive unit tests for the new flow and call ordering - Enhanced test double with tracking for `waitForAndProcessIncomingData()` This completes the core conditional behavior for persistent outgoing connections.
…hex() - Added `buildReplyPacket()` to OutgoingTcpThread for constructing smart/auto responses - Fixed hex casing inconsistency: updated ASCIITohex() to always return uppercase (matching byteArrayToHex()) - Added comprehensive parameterized tests for buildReplyPacket() (encrypted/plain, empty responseData, etc.) - Added testASCIITohex_alwaysReturnsUppercase() to prevent future regressions - Updated test double with call tracking for the new method This establishes a clean, testable foundation for smart response functionality.
…est coverage - Extracted reply decision logic into `shouldSendReply()` - Added `sendReplyIfNeeded()` as the central method for conditional reply sending - Command-line reply packet now correctly takes precedence over settings - Refactored buildReplyPacket() response data handling - Made sendOutgoingPacket() virtual in BaseTcpThread - Added proper consoleMode handling - Improved testability with getSettings() temporary file support for unit tests - Added comprehensive unit tests for shouldSendReply() (data-driven) and sendReplyIfNeeded() - Added test helpers: normalizeReplyPacket, setupThreadToSendReply, buildExpectedReplyPacket This improves separation of concerns and makes reply behavior much more testable and maintainable.
ProfessorTom
force-pushed
the
tomas/connection-raii-initial
branch
from
1c8340b to
99d2b79
Compare
- Automatically trigger reply logic after receiving a packet in OutgoingTcpThread - Added unit test to verify sendReplyIfNeeded() is called from processIncomingData() - Updated test double to track buildReceivedPacket() in call sequence This connects the incoming data processing with the newly implemented reply system.
…nections - Call `processIncomingData()` after sending in the non-persistent path of `OutgoingTcpThread::run()`. This wires up `sendReplyIfNeeded()` (basic response / command-line reply) for normal one-off sends. - Updated `SingleSendOutgoingTcpThreadTests` to reflect new call sequence.
…nseData() - Extracted smart response logic into `getSmartResponseData()` - Added QSettings overload to `Packet::fetchSmartConfig()` for testability - Updated `buildReplyPacket()` to use the new smart response function - Added full unit test coverage (`testGetSmartResponseData`) - Updated test double for call tracking Smart responses now have proper priority (after command-line replies) and work in both single-shot and persistent modes.
- Integrated `getSmartResponseData()` into `buildReplyPacket()` so smart responses now have correct priority over basic responses. - Added `testSingleShot_sendsSmartResponse()` in SingleSendOutgoingTcpThreadTests - Added `testPersistent_sendsSmartResponse()` in OutgoingTcpThreadPersistentConnectionLoopTests Smart response support is now fully active for both single-shot and persistent outgoing connections.
- Refactored duplicated call sequence expectations into `getSendReplyIfNeededExpectedCallSequence()`
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Before submitting a pull request:
This will likely be a Cthulhu PR that will need to be broken up into a few smaller PRs once the work is done. Unfortunately, the way through is a long running branch in the short term.