Tomas/connection raii initial#413
Draft
ProfessorTom wants to merge 130 commits intodannagle:developmentfrom
Draft
Tomas/connection raii initial#413ProfessorTom wants to merge 130 commits intodannagle:developmentfrom
ProfessorTom wants to merge 130 commits intodannagle:developmentfrom
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
…dshake()` for consistency with upcoming naming scheme.
…or handling - Added getSslErrors() and getSslHandshakeErrors() as virtual methods - These allow TestTcpThreadClass to override and return mocked error lists - Uses const_cast internally to work around Qt's non-const sslErrors() API
- Override the new virtual helpers so MockSslSocket can supply error lists - Enables proper testing of the SSL error emission path in handleIncomingSSLHandshake()
- Moved all incoming/server-side SSL handling (loadSSLCerts, startServerEncryption, waitForEncrypted, error handling, and certificate/cipher info emission) from run() into a dedicated method handleIncomingSSLHandshake(QSslSocket &sock) - Made the method virtual to support mocking in unit tests
…ndshake - testHandleIncomingSSLHandshake_success() - testHandleIncomingSSLHandshake_withErrors() - testHandleOutgoingSSLHandshake_success() - testHandleOutgoingSSLHandshake_withErrors() Tests verify packet emission for both success and error cases using MockSslSocket.
…rization tests - Created protected virtual runOutgoingClient() containing the main outgoing client path - Added callRunOutgoingClient() helper in TestTcpThreadClass for direct testing - Added two characterization tests: - testRunOutgoingClient_plainTCP_connectFailure() - testRunOutgoingClient_SSL_path_is_attempted() This will significantly reduce the size and complexity of run() while protecting the outgoing behavior with tests.
- Extracted the initial packet building logic for incoming connections into its own method: buildInitialReceivedPacket(QSslSocket &sock) - Added comprehensive characterization test: - testBuildInitialReceivedPacket() for plain TCP path - testBuildInitialReceivedPacket_SSLPath() for SSL path - Used MockSslSocket to reliably test the isEncrypted() branch - Improved test robustness with proper socket acceptance and cleanup This will help make the incoming path in run() much easier to read and sets up for further extractions (runIncomingConnection(), handleIncomingPersistentConnection(), etc.).
- Centralized all incoming/server logic into runIncomingConnection() - run() is now very short and readable with clear outgoing vs incoming branches - No behavior change
- Extracted persistent connection setup logic into its own method - Added two characterization tests: - testPrepareForPersistentLoop_preparesSendPacketCorrectly() - testPrepareForPersistentLoop_withRealSocket_updatesPorts() - Minor cleanup in packet preparation logic - Added callPrepareForPersistentLoop() and getSendPacket() to TestTcpThreadClass - Exposed setSocketDescriptor() for testing - Updated tests to use unique_ptr for better cleanup"
- Moved prepareForPersistentLoop() and persistentConnectionLoop() into new file src/persistentLoopConnection.cpp - Moved corresponding unit tests to persistentconnectionlooptests.cpp - Updated test runner and both CMakeLists.txt files - Fixed include paths for unit tests This reduces the size of tcpthread.cpp and sets up for targeted refactoring of the persistent loop logic.
- 5 tests now cover major paths: * normal data/response flow * idle status emission * immediate exit on closeRequest * exit on connection broken * final cleanup behavior - Provides safety net before we begin extracting smaller functions - Minor test helper improvements
- Extracted cleanup logic into cleanupAfterPersistentConnectionLoop() - Added testCleanupAfterPersistentConnectionLoop_whenClientConnectionIsNull_emitsDisconnected() - Verified that "Disconnected" status is emitted even when clientConnection is nullptr - Added callCleanupAfterPersistentConnectionLoop() helper in TestTcpThreadClass - Minor test infrastructure improvements (call counts pattern started) This establishes the pattern we'll use for testing extracted functions.
- Extracted final socket cleanup logic into its own method - Added virtual deleteSocketLater() wrapper for testability - Added three unit tests for cleanup behavior: * Null clientConnection case * Normal (non-managed) happy path * Managed-by-Connection case (verifies no deleteLater) - Updated TestTcpThreadClass with getMockSocket() helper and proper base member handling - Removed name-hiding clientConnection member This establishes the extraction + testing pattern we'll use for the rest of persistentConnectionLoop().
- Added virtual getPeerAddress() accessor to enable test mocking - Extracted getPeerAddressAsString() to eliminate duplicated address formatting code - Removed unused ipMode calculation from persistentConnectionLoop() - Updated both fromIP assignments to use the new helper method - Added unit tests for IPv4 and IPv6 address formatting paths - Updated TestTcpThreadClass with proper overrides and mock support This reduces duplication and makes the main loop significantly cleaner and more maintainable.
- Extracted packet sending logic into sendCurrentPacket() - Added two unit tests: * testSendCurrentPacket_emitsConnectionStatusWhenDataExists() * testSendCurrentPacket_emitsSentPacketWhenDataExists() - Added callSendCurrentPacket() helper and call count tracking in TestTcpThreadClass This continues the incremental extraction of persistentConnectionLoop(), making the main loop more readable.
- Added testSendCurrentPacket_doesNothingWhenNoDataToSend() - Verifies that no status or packetSent signals are emitted when sendPacket has no data - Completes basic behavior coverage for the extracted sendCurrentPacket() function
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.