Tomas/connection raii initial#413
Draft
ProfessorTom wants to merge 114 commits intodannagle:developmentfrom
Draft
Tomas/connection raii initial#413ProfessorTom wants to merge 114 commits intodannagle:developmentfrom
ProfessorTom wants to merge 114 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
Mainly for test injection / mocking. - New constructor: TCPThread(QSslSocket*, host, port, initialPacket, parent) - Move socket parenting + signal wiring into constructor - Set sendPacket.toIP/port from constructor args
give ourselves some conceptual toString()-like magic
...before we start replacing existing code in `run()` with code we pulled out into our smaller methods.
…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.).
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.