Tomas/connection raii initial#413
Draft
ProfessorTom wants to merge 182 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
- 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()`
…ling - Split run() into handleOutgoingSSL(), handleOutgoingPlainTCP(), etc. - Added dedicated handshake success/failure handlers and loadSSLCerts() - Better separation of concerns for future SSL improvements
- Added PacketSenderQSslSocketInterface + RealQSslSocket wrapper - Updated BaseTcpThread and OutgoingTcpThread to primarily use the new interface - Removed most qobject_cast / dynamic_cast hacks from test doubles - MockSslSocket now implements the interface via multiple inheritance - Simplified socket access methods in BaseTcpThread - Updated tests and TestUtils to work with the new architecture All tests passing after migration.
…Interface - Replaced all remaining getSocket() calls with getSocketInterface() - Updated BaseTcpThread and OutgoingTcpThread to use the interface consistently - Expanded PacketSenderQSslSocketInterface with SSL and common socket methods - Updated RealQSslSocket and MockSslSocket implementations - Cleaned up test doubles (removed old qobject_cast hacks) - All tests passing This completes the core socket abstraction migration.
…ions - Added `FileUtils` namespace with helpers for loading base64-encoded resources - Implemented `loadSnakeOilCerts()`, `loadSSLCerts(bool)`, and related helpers - Added `hasLocalCertificate()` / `hasPrivateKey()` to `PacketSenderQSslSocketInterface` - Updated `handleOutgoingSSL()` to respect `LOAD_SNAKEOIL_CERTS` setting - Added comprehensive unit tests in `OutgoingTcpThreadConnectionTests`: - SSL handshake success/failure paths - Production vs snake oil certificate loading - Error handling for missing files - `run()` call sequence and integration - Updated test infrastructure (CMake, mocks, TestUtils) OutgoingTcpThread now has full SSL parity with the old TcpThread for single-shot connections.
…shot + persistent behavior - Set `persistent` flag from Packet in constructor (explicit copy) - Made `persistentConnectionLoop()` virtual for easier testing/subclassing - Added comprehensive run() tests: - Constructor correctly sets persistent flag (true/false/default) - Non-persistent path skips persistentConnectionLoop() - Persistent path calls persistentConnectionLoop() + closeConnection() - SSL + persistent sanity test - Minor cleanup and test double updates OutgoingTcpThread is now feature-complete for both single-shot and persistent SSL/TCP connections.
- Add incomingtcpthread.cpp to build - Extend BaseTcpThread with shouldUseSSL flag and getter - Add getSocketDescriptor()/setSocketDescriptor() to PacketSenderQSslSocketInterface + RealQSslSocket implementation (needed for taking ownership of accepted sockets) - Wire up unit tests and update MockSslSocket - Update test runner This lays the foundation for handling incoming TCP (and SSL) connections.
- Added buildInitialReceivedPacket() which creates the first Packet for an incoming connection (TCP or SSL) - Populates timestamp, name, tcpOrUdp, IPs, ports, and reads initial data from socket if available - Added comprehensive unit tests (data-driven for secure/insecure cases) - Updated test double and helper infrastructure This gives incoming connections a proper initial packet similar to outgoing ones.
- Added sendSmartReplyIfConfigured() that automatically replies to
incoming connections when SEND_RESPONSE and RESPONSE_HEX settings
are configured.
- Supports Packet macro expansion (e.g. {COUNTER}, {UNIXTIME}, etc.)
before sending the reply.
- Added comprehensive unit tests covering:
• Disabled by default / SEND_RESPONSE=false
• Empty RESPONSE_HEX
• Successful reply path
• Macro expansion (e.g. {COUNTER} → "1")
- Fixed IncomingTcpThreadTestDouble to use public inheritance
- Added proper call tracking and overrides for sendOutgoingPacket()
This enables smart/auto-reply functionality for incoming TCP/SSL connections.
- Move SSL certificate loading logic (loadSnakeOil* and loadSSLCerts) from OutgoingTcpThread up to BaseTcpThread to eliminate duplication. - Add emitSSLDiagnosticPackets() to IncomingTcpThread that emits four diagnostic packets on successful SSL handshake: • Cipher encryption method • Cipher authentication method • Peer certificate issuer • Local certificate issuer - Add corresponding virtual method and interface support (localCertificate()). - Add comprehensive unit tests covering: • null socket • non-encrypted socket • success path with all four packets - Update MockSslSocket and IncomingTcpThreadTestDouble accordingly.
- Extract call tracking logic into a reusable `CallTracker` base class (with `recordCall()`, `getCallCount()`, `wasMethodCalled()`, etc.) - Make `IncomingTcpThreadTestDouble` and `OutgoingTcpThreadTestDouble` inherit from `CallTracker` - Replace duplicated per-method counter variables and manual callSequence management with centralized tracking - Update all affected tests to use `getCallCount(CallTracker::XXX())` - Add `utils/calltracker.cpp` to unit test build This eliminates boilerplate, improves maintainability, and makes it easier to add new tracked methods in the future.
- Relocate `testutils.cpp` and `testutils.h` to `src/tests/unit/utils/` - Update CMakeLists.txt and all including test files to new include path - Adjust relative include for MockSslSocket in testutils.h This improves the organization of the unit test directory by grouping shared test helpers (alongside the newly added CallTracker) into a `utils/` folder.
- Extracted SSL handshake logic into `performSSLHandshakeIfNeeded()` - Load snake oil / real certs based on setting - Call `startServerEncryption()` + `waitForEncrypted(5000)` - Emit error on timeout - Call `emitSSLDiagnosticPackets()` only on success - Updated `PacketSenderQSslSocketInterface` + `RealQSslSocket` - Enhanced `MockSslSocket` with `mockSSLHandshakeShouldSucceed` control - Added comprehensive unit tests (null socket, snake oil setting, ignore SSL errors, startServerEncryption, waitForEncrypted success/failure, diagnostic packets behavior) - Minor cleanup: commented noisy debug messages in globals.h - Integrated with CallTracker for better test assertions This centralizes incoming SSL setup logic and makes it fully testable.
- Moved common `closeConnection()` logic (disconnectFromHost + close + emit status) from OutgoingTcpThread up to BaseTcpThread - Removed duplicate implementations and overrides from IncomingTcpThread and OutgoingTcpThread - Made `closeConnection()` a regular virtual method (no longer pure virtual) - Added full unit test coverage in BaseTcpThreadTests - Updated BaseTcpThreadTestDouble and removed duplicated tests from OutgoingTcpThreadTests - Integrated with CallTracker for call verification This eliminates code duplication and ensures consistent connection cleanup behavior for both incoming and outgoing threads.
- Added `handleIncomingConnection()`: emits status, performs SSL handshake (if enabled), builds initial packet, emits `packetReceived`, and sends smart reply if configured - Implemented `run()` to orchestrate the full incoming flow + cleanup via `closeConnection()` - Added early exit in `performSSLHandshakeIfNeeded()` when `shouldUseSSL` is false - Updated `IncomingTcpThreadTestDouble` and CallTracker with new tracked methods - Added comprehensive unit tests covering: - SSL disabled path - Null socket handling - Connection status emission - Full success call sequence - Packet emission - `run()` behavior This completes the main execution path for incoming TCP/SSL connections.
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.
Some benefits and thoughts as work continues on this branch:
TcpThreadclass will be replaced withBaseTcpThread,OutgoingTcpThreadandIncomingTcpThread.The idea here is to combine common code in the base class but separate the different directions so that they have room to breath and you can follow the main flows more easily. All three of those classes will automagically clean up both the socket and the threads via RAII and Qt's object lifecycle and parenting system.
There will be a
Connectionclass (possibly also anIncomingTCPConnectionand anOutgoingTcpConnectionclasses but I'm not sure yet) to handled the TCP connections.I'm still debating this abstract layer, but at the time of this writing (6.8.2016) there is a
Connectionclass in the code. The idea is thatConnectionManagerwill manageConnectionclasses which in turn will holdIncomingTcpThreadandOutgoingTcpThreadclasses.Unit tests have been added for code I have modified or added and will continue to be added for code I modify or add.
qMake is likely going away.
I'm not sure how Dan builds for each platform.
I was under the impression that most if not all platforms were using CMake now. I know snapcraft and macOS does. Regardless, the unit test only work with CMake, in part, because I am developing on a Mac. There may be some way to get the unit tests to build and run with qMake, but as I understand it, Dan wanted to move away from qMake anyway, so now's the chance.
This means I should be able to delete the .pro files in this PR. At the time of this writing I am not doing so because I don't know what the consequences are for taking that action. e.g. there may be one build that is still using qMake.
This likely means modifying the build process to build and run the unit tests and perhaps have such failures block PR merges. While the latter can be done once here on GitHub, the former will have to be done on each platform that PacketSender is built for. Given we are dealing with some low-level functionality (e.g. IPv4 vs IPv6, os default network stacks, etc.), it's possible that some unit tests may fail on platforms other than macOS. I haven't tested on any other platform. I fully intend for this to be a problem for Dan to solve, not because I'm being lazy or want to give Dan more work, but because at the time of this writing, this PR is not complete and I don't have access to each build system to test them.
Donations accepted here