Open
Conversation
WalkthroughAdapterClient gains a handshakeTimeout parameter, handles JSON-RPC Changes
Sequence Diagram(s)sequenceDiagram
participant AdapterProc as AdapterProcess
participant AdapterClient
participant ModbusPoll
participant QtLog as Qt Logging
AdapterProc->>AdapterClient: JSON-RPC notification\n(method: "adapter.diagnostic", params: {level, message})
activate AdapterClient
AdapterClient->>AdapterClient: onNotificationReceived()\nif method == "adapter.diagnostic"
AdapterClient->>AdapterClient: validate params is object\nextract level, message
AdapterClient->>ModbusPoll: emit diagnosticReceived(level, message)
deactivate AdapterClient
activate ModbusPoll
ModbusPoll->>ModbusPoll: onAdapterDiagnostic()\nmap level -> Qt logging function
ModbusPoll->>QtLog: log message at mapped level
deactivate ModbusPoll
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/communication/tst_modbuspoll.cpp (1)
9-19: Consider resetting captured values at the start of each test.If a test runs and the target method fails to emit a log message (e.g., due to a regression), the test could incorrectly pass using stale values from a previous test. Resetting the globals before each invocation improves test isolation.
♻️ Proposed fix
void captureHandler(QtMsgType type, const QMessageLogContext&, const QString& msg) { g_capturedType = type; g_capturedMessage = msg; } + + void resetCaptured() + { + g_capturedType = QtMsgType{}; + g_capturedMessage.clear(); + } }Then call
resetCaptured()at the start of each test, e.g.:void TestModbusPoll::diagnosticDebugLevel() { resetCaptured(); QtMessageHandler previous = qInstallMessageHandler(captureHandler); // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/communication/tst_modbuspoll.cpp` around lines 9 - 19, Add a resetCaptured() helper that sets g_capturedType to a default and clears g_capturedMessage, and call resetCaptured() at the start of each test that uses captureHandler (e.g., TestModbusPoll::diagnosticDebugLevel and other TestModbusPoll tests) so stale global values cannot make a test pass; update tests to invoke resetCaptured() before qInstallMessageHandler(captureHandler) and before exercising the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ProtocolAdapter/adapterclient.cpp`:
- Around line 170-178: The diagnostic handler should validate that params
contains string fields "level" and "message" before emitting; update the code in
adapterclient.cpp where params and QJsonObject obj are used to check
obj.contains("level") && obj.value("level").isString() and similarly for
"message", and if either check fails call qCWarning(scopeComm) with a clear
message (including the raw params) and return without emitting
diagnosticReceived; otherwise call diagnosticReceived with the verified string
values (use toString() only after isString()).
---
Nitpick comments:
In `@tests/communication/tst_modbuspoll.cpp`:
- Around line 9-19: Add a resetCaptured() helper that sets g_capturedType to a
default and clears g_capturedMessage, and call resetCaptured() at the start of
each test that uses captureHandler (e.g., TestModbusPoll::diagnosticDebugLevel
and other TestModbusPoll tests) so stale global values cannot make a test pass;
update tests to invoke resetCaptured() before
qInstallMessageHandler(captureHandler) and before exercising the code under
test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: afce44c4-9607-4aae-9de5-47d21cba23d6
📒 Files selected for processing (10)
src/ProtocolAdapter/adapterclient.cppsrc/ProtocolAdapter/adapterclient.hsrc/communication/modbuspoll.cppsrc/communication/modbuspoll.htests/CMakeLists.txttests/ProtocolAdapter/tst_adapterclient.cpptests/ProtocolAdapter/tst_adapterclient.htests/communication/CMakeLists.txttests/communication/tst_modbuspoll.cpptests/communication/tst_modbuspoll.h
AdapterClient now connects to AdapterProcess::notificationReceived and emits diagnosticReceived(level, message) for adapter.diagnostic notifications. ModbusPoll connects to this signal and routes the message to the existing scopeComm logging category using the appropriate qCDebug/qCInfo/qCWarning call, so adapter-side log messages flow through ScopeLogging into DiagnosticModel alongside host-side entries. Tests updated: notificationIgnored now also verifies diagnosticReceived stays silent for non-diagnostic notifications. Two new tests cover warning and debug level forwarding. https://claude.ai/code/session_01UhRoTiKzKdkuurvzKQBVjr
Guard against malformed adapter.diagnostic params in onNotificationReceived: if params is not a JSON object the signal is not emitted and a warning is logged instead of silently forwarding empty strings. Add explicit "warning" branch to onAdapterDiagnostic and an else branch that logs the unknown level so unrecognised values are visible rather than being silently promoted. New test diagnosticMalformedParams verifies no signal is emitted for non-object params. New tst_modbuspoll covers all four branches of the level-mapping logic (debug, info, warning, unknown) using a captured Qt message handler. https://claude.ai/code/session_01UhRoTiKzKdkuurvzKQBVjr
jgeudens
force-pushed
the
claude/route-logging-diagnostics-4Awjy
branch
from
c91f778 to
602a676
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/ProtocolAdapter/adapterclient.cpp (1)
176-178:⚠️ Potential issue | 🟡 MinorValidate
levelandmessagetypes before emitting diagnostics.At the moment, missing/non-string fields silently become empty strings via
toString(), which can create misleading downstream logs.Proposed fix
- QJsonObject obj = params.toObject(); - emit diagnosticReceived(obj.value(QStringLiteral("level")).toString(), - obj.value(QStringLiteral("message")).toString()); + const QJsonObject obj = params.toObject(); + const QJsonValue levelValue = obj.value(QStringLiteral("level")); + const QJsonValue messageValue = obj.value(QStringLiteral("message")); + if (!levelValue.isString() || !messageValue.isString()) + { + qCWarning(scopeComm) << "AdapterClient: adapter.diagnostic requires string level and message"; + return; + } + + emit diagnosticReceived(levelValue.toString(), messageValue.toString());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ProtocolAdapter/adapterclient.cpp` around lines 176 - 178, The code currently reads `params.toObject()` and blindly calls `toString()` on `obj.value("level")` and `obj.value("message")`; validate that both keys exist and that `obj.value(QStringLiteral("level")).isString()` and `obj.value(QStringLiteral("message")).isString()` before converting to QString and calling `emit diagnosticReceived(... )`; if either check fails, avoid emitting a potentially misleading empty/invalid diagnostic (log or emit an explicit malformed-diagnostic warning instead). Ensure you update the logic around the `QJsonObject obj = params.toObject();` block and the `emit diagnosticReceived` call so the values are type-checked first and only converted when valid.
🧹 Nitpick comments (1)
tests/communication/tst_modbuspoll.cpp (1)
72-82: Add a dedicated test for the protocol-defined"error"level.The suite currently validates an unknown
"critical"level path, but it does not assert behaviour for"error", which is now part of the protocol contract.Based on learnings: Applies to tests/**/*.cpp : When fixing a bug, add a test that reproduces the issue before implementing the fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/communication/tst_modbuspoll.cpp` around lines 72 - 82, Add a new unit test mirroring TestModbusPoll::diagnosticUnknownLevel that invokes the same onAdapterDiagnostic method on _pModbusPoll but passes QStringLiteral("error") as the diagnostic level and an appropriate message, installs captureHandler via qInstallMessageHandler, restores the previous handler, and then asserts the expected behavior (e.g., QCOMPARE on g_capturedType and QVERIFY on g_capturedMessage) to validate the protocol-defined "error" level path; reference TestModbusPoll::diagnosticUnknownLevel and the onAdapterDiagnostic invocation to implement the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@adapters/describe.json`:
- Line 15: The defaults in adapters/describe.json for the fields persistent and
int32LittleEndian were changed to true while adapters/json-rpc-spec.md still
documents them as false, causing an API mismatch for adapter.describe; fix by
making them consistent: either revert the two occurrences of persistent and
int32LittleEndian in adapters/describe.json back to false to match the spec, or
update adapters/json-rpc-spec.md to state the new true defaults—ensure you
update both places where those keys appear in adapters/describe.json and update
the adapter.describe section of the spec accordingly so runtime behavior and
documentation match.
In `@src/communication/modbuspoll.cpp`:
- Around line 123-149: The onAdapterDiagnostic function currently misses
handling the protocol "error" level causing those messages to hit the fallback;
add an else-if branch that checks for level == QStringLiteral("error") and route
it to qCCritical(scopeAdapter) << message (use qCCritical to preserve proper
severity), leaving other branches and the fallback unchanged so unknown levels
still warn; update only the ModbusPoll::onAdapterDiagnostic function and
reference the scopeAdapter logging category when adding the mapping.
In `@src/ProtocolAdapter/adapterclient.h`:
- Around line 117-120: Update the docstring for the signal that is emitted on
adapter.diagnostic notifications (the comment block above the signal declaration
in adapterclient.h) to include the missing "error" severity level; change the
severity list from "debug", "info", or "warning" to include "error" (e.g.,
"debug", "info", "warning", or "error") so the documentation matches the current
protocol.
In `@tests/communication/tst_modbuspoll.cpp`:
- Around line 36-82: Before invoking onAdapterDiagnostic in each TestModbusPoll
test (diagnosticDebugLevel, diagnosticInfoLevel, diagnosticWarningLevel,
diagnosticUnknownLevel), reset the shared capture state g_capturedType and
g_capturedMessage to their default/empty values so prior test output cannot mask
failures; do this immediately before qInstallMessageHandler(captureHandler)
(references: g_capturedType, g_capturedMessage, captureHandler, _pModbusPoll,
onAdapterDiagnostic).
- Line 26: The test enables the wrong logging category so adapter debug messages
never reach the handler; update the QLoggingCategory::setFilterRules call used
in the test setup to include the adapter category (e.g., enable
scope.comm.adapter.debug=true or a wildcard like scope.comm.*.debug=true) so the
messages emitted by onAdapterDiagnostic (category scope.comm.adapter) are
delivered and the debug-level assertions become reliable.
---
Duplicate comments:
In `@src/ProtocolAdapter/adapterclient.cpp`:
- Around line 176-178: The code currently reads `params.toObject()` and blindly
calls `toString()` on `obj.value("level")` and `obj.value("message")`; validate
that both keys exist and that `obj.value(QStringLiteral("level")).isString()`
and `obj.value(QStringLiteral("message")).isString()` before converting to
QString and calling `emit diagnosticReceived(... )`; if either check fails,
avoid emitting a potentially misleading empty/invalid diagnostic (log or emit an
explicit malformed-diagnostic warning instead). Ensure you update the logic
around the `QJsonObject obj = params.toObject();` block and the `emit
diagnosticReceived` call so the values are type-checked first and only converted
when valid.
---
Nitpick comments:
In `@tests/communication/tst_modbuspoll.cpp`:
- Around line 72-82: Add a new unit test mirroring
TestModbusPoll::diagnosticUnknownLevel that invokes the same onAdapterDiagnostic
method on _pModbusPoll but passes QStringLiteral("error") as the diagnostic
level and an appropriate message, installs captureHandler via
qInstallMessageHandler, restores the previous handler, and then asserts the
expected behavior (e.g., QCOMPARE on g_capturedType and QVERIFY on
g_capturedMessage) to validate the protocol-defined "error" level path;
reference TestModbusPoll::diagnosticUnknownLevel and the onAdapterDiagnostic
invocation to implement the new test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a674f537-a009-4533-9b9f-3863c3db0596
⛔ Files ignored due to path filters (2)
adapters/dummymodbusadapter.exeis excluded by!**/*.exeadapters/modbusadapter.exeis excluded by!**/*.exe
📒 Files selected for processing (16)
adapters/describe.jsonadapters/dummymodbusadapteradapters/json-rpc-spec.mdadapters/modbusadaptersrc/ProtocolAdapter/adapterclient.cppsrc/ProtocolAdapter/adapterclient.hsrc/communication/modbuspoll.cppsrc/communication/modbuspoll.hsrc/util/scopelogging.cppsrc/util/scopelogging.htests/CMakeLists.txttests/ProtocolAdapter/tst_adapterclient.cpptests/ProtocolAdapter/tst_adapterclient.htests/communication/CMakeLists.txttests/communication/tst_modbuspoll.cpptests/communication/tst_modbuspoll.h
✅ Files skipped from review due to trivial changes (3)
- tests/CMakeLists.txt
- src/util/scopelogging.cpp
- tests/communication/tst_modbuspoll.h
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/communication/CMakeLists.txt
- src/communication/modbuspoll.h
- tests/ProtocolAdapter/tst_adapterclient.h
- tests/ProtocolAdapter/tst_adapterclient.cpp
| "id": 0, | ||
| "ip": "127.0.0.1", | ||
| "persistent": false, | ||
| "persistent": true, |
There was a problem hiding this comment.
Defaults now diverge from the published contract.
persistent and int32LittleEndian were changed to true, but adapters/json-rpc-spec.md still documents both defaults as false. This creates a behavioural/API mismatch for clients consuming adapter.describe.
Proposed fix (if the default change was unintentional)
- "persistent": true,
+ "persistent": false,
...
- "int32LittleEndian": true,
+ "int32LittleEndian": false,Also applies to: 26-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@adapters/describe.json` at line 15, The defaults in adapters/describe.json
for the fields persistent and int32LittleEndian were changed to true while
adapters/json-rpc-spec.md still documents them as false, causing an API mismatch
for adapter.describe; fix by making them consistent: either revert the two
occurrences of persistent and int32LittleEndian in adapters/describe.json back
to false to match the spec, or update adapters/json-rpc-spec.md to state the new
true defaults—ensure you update both places where those keys appear in
adapters/describe.json and update the adapter.describe section of the spec
accordingly so runtime behavior and documentation match.
Comment on lines
+117
to
+120
| /*! | ||
| * \brief Emitted when an adapter.diagnostic notification is received from the adapter. | ||
| * \param level Severity level string: "debug", "info", or "warning". | ||
| * \param message The diagnostic message from the adapter. |
There was a problem hiding this comment.
Signal docs are stale for diagnostic severity levels.
The comment lists only "debug", "info", and "warning", but the protocol now includes "error". Please update this docstring to avoid misleading consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ProtocolAdapter/adapterclient.h` around lines 117 - 120, Update the
docstring for the signal that is emitted on adapter.diagnostic notifications
(the comment block above the signal declaration in adapterclient.h) to include
the missing "error" severity level; change the severity list from "debug",
"info", or "warning" to include "error" (e.g., "debug", "info", "warning", or
"error") so the documentation matches the current protocol.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ProtocolAdapter/adapterclient.cpp`:
- Around line 117-119: Do not set _state = State::IDLE immediately after calling
_pProcess->stop(); instead leave the state as STOPPING/ERROR until the child
process actually exits, and only transition to State::IDLE inside
onProcessFinished() after processFinished() is observed; update the three
locations that currently clear _state (the Unexpected non-object path emitting
sessionError, the timeout path that emits sessionStopped(), and the other
error/stop branches referenced) to call _pProcess->stop() without resetting
_state, ensure timers/signals are not prematurely stopped so onProcessFinished()
still receives the completion event, and adjust prepareAdapter()/State::STOPPING
handling so fast retries cannot start a new process while the old process is
still running.
In `@src/ProtocolAdapter/adapterprocess.cpp`:
- Around line 51-59: Replace the QTimer::singleShot lambda with a cancellable
timer and a named handler: add an int member (e.g., m_stopTimerId = 0) and use
startTimer(cStopTimeoutMs) instead of QTimer::singleShot to store the returned
timer id; implement a private handler (e.g., void handleStopTimeout() or
override timerEvent and dispatch when timerId == m_stopTimerId) that checks
_pProcess->state() != QProcess::NotRunning and calls _pProcess->kill(); ensure
you call killTimer(m_stopTimerId) and reset m_stopTimerId to 0 from start() and
onProcessFinished() to cancel any pending stop timeout before restarting or
after exit, and remove the lambda to satisfy the "no multi-statement lambda"
guideline.
In `@tests/ProtocolAdapter/tst_adapterprocess.cpp`:
- Around line 47-50: Replace the fixed QTest::qWait(500) sleeps in
tst_adapterprocess.cpp with signal-driven waits: after calling
AdapterProcess::stop() use a QSignalSpy that watches the process finished signal
(the same signal used in processFinishedEmittedOnStop()) and call
spy.wait(cStopTimeoutMs) so the test waits up to cStopTimeoutMs (3000 ms)
instead of a fixed 500ms; similarly, in sendRequestEmitsResponseReceived()
replace the fixed qWait with a QSignalSpy on the response-received signal used
by the test and call spy.wait(cStopTimeoutMs). Ensure you reference the same
signal names the tests currently expect and remove the QTest::qWait calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8fdba55-d8fc-4f30-beca-ec2ac7cbdc41
📒 Files selected for processing (12)
src/ProtocolAdapter/adapterclient.cppsrc/ProtocolAdapter/adapterclient.hsrc/ProtocolAdapter/adapterprocess.cppsrc/ProtocolAdapter/adapterprocess.hsrc/communication/modbuspoll.cppsrc/models/diagnosticmodel.cpptests/ProtocolAdapter/tst_adapterclient.cpptests/ProtocolAdapter/tst_adapterclient.htests/ProtocolAdapter/tst_adapterprocess.cpptests/communication/tst_modbuspoll.cpptests/models/tst_diagnosticmodel.cpptests/models/tst_diagnosticmodel.h
✅ Files skipped from review due to trivial changes (1)
- src/ProtocolAdapter/adapterprocess.h
🚧 Files skipped from review as they are similar to previous changes (2)
- src/communication/modbuspoll.cpp
- tests/ProtocolAdapter/tst_adapterclient.h
Comment on lines
117
to
119
| _state = State::IDLE; | ||
| QTimer::singleShot(0, this, [this]() { _pProcess->stop(); }); | ||
| _pProcess->stop(); | ||
| emit sessionError(QString("Unexpected non-object result for %1").arg(method)); |
There was a problem hiding this comment.
Do not switch back to IDLE until the child has actually exited.
AdapterProcess::stop() now returns immediately, but these paths still clear _state early and the timeout branch even emits sessionStopped() before processFinished(). A fast retry can then re-enter prepareAdapter() while the old process is still alive, and the State::STOPPING error path drops its completion signal because the timer is stopped and onProcessFinished() sees IDLE. Keep a non-idle stopping/error state until the process exit is observed, then transition to IDLE.
Also applies to: 130-132, 147-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ProtocolAdapter/adapterclient.cpp` around lines 117 - 119, Do not set
_state = State::IDLE immediately after calling _pProcess->stop(); instead leave
the state as STOPPING/ERROR until the child process actually exits, and only
transition to State::IDLE inside onProcessFinished() after processFinished() is
observed; update the three locations that currently clear _state (the Unexpected
non-object path emitting sessionError, the timeout path that emits
sessionStopped(), and the other error/stop branches referenced) to call
_pProcess->stop() without resetting _state, ensure timers/signals are not
prematurely stopped so onProcessFinished() still receives the completion event,
and adjust prepareAdapter()/State::STOPPING handling so fast retries cannot
start a new process while the old process is still running.
Comment on lines
51
to
+59
| if (_pProcess->state() != QProcess::NotRunning) | ||
| { | ||
| _pProcess->closeWriteChannel(); | ||
| if (!_pProcess->waitForFinished(3000)) | ||
| { | ||
| _pProcess->kill(); | ||
| _pProcess->waitForFinished(1000); | ||
| } | ||
| QTimer::singleShot(cStopTimeoutMs, this, [this]() { | ||
| if (_pProcess->state() != QProcess::NotRunning) | ||
| { | ||
| _pProcess->kill(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd src/ProtocolAdapter && wc -l adapterprocess.cpp adapterprocess.hRepository: ModbusScope/NotOnlyModbusScope
Length of output: 130
🏁 Script executed:
cd src/ProtocolAdapter && head -80 adapterprocess.cpp | tail -40Repository: ModbusScope/NotOnlyModbusScope
Length of output: 1138
🏁 Script executed:
cd src/ProtocolAdapter && grep -n "void.*start\|void.*stop\|void.*onProcessFinished" adapterprocess.cpp | head -20Repository: ModbusScope/NotOnlyModbusScope
Length of output: 194
🏁 Script executed:
cd src/ProtocolAdapter && cat adapterprocess.hRepository: ModbusScope/NotOnlyModbusScope
Length of output: 3744
🏁 Script executed:
cd src/ProtocolAdapter && sed -n '20,50p' adapterprocess.cppRepository: ModbusScope/NotOnlyModbusScope
Length of output: 1035
🏁 Script executed:
cd src/ProtocolAdapter && sed -n '172,195p' adapterprocess.cppRepository: ModbusScope/NotOnlyModbusScope
Length of output: 332
🏁 Script executed:
cd src/ProtocolAdapter && grep -n "QTimer\|_pTimer\|killTimer\|stopTimer" adapterprocess.cpp adapterprocess.hRepository: ModbusScope/NotOnlyModbusScope
Length of output: 194
Cancel any pending stop timeout before a restart.
The singleShot timer is not cancelled when start() or onProcessFinished() is called. If the child process exits quickly and a new adapter is started within cStopTimeoutMs, the stale callback will fire and kill the fresh process because _pProcess is running again. Store the timer ID returned by singleShot as a member and call killTimer() from start() and onProcessFinished().
Additionally, the lambda contains multiple statements and violates the coding guideline: "Avoid lambda expressions with more than 2 captures or multiple statements; use named functions instead for clarity". Refactor this into a named private slot or function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ProtocolAdapter/adapterprocess.cpp` around lines 51 - 59, Replace the
QTimer::singleShot lambda with a cancellable timer and a named handler: add an
int member (e.g., m_stopTimerId = 0) and use startTimer(cStopTimeoutMs) instead
of QTimer::singleShot to store the returned timer id; implement a private
handler (e.g., void handleStopTimeout() or override timerEvent and dispatch when
timerId == m_stopTimerId) that checks _pProcess->state() != QProcess::NotRunning
and calls _pProcess->kill(); ensure you call killTimer(m_stopTimerId) and reset
m_stopTimerId to 0 from start() and onProcessFinished() to cancel any pending
stop timeout before restarting or after exit, and remove the lambda to satisfy
the "no multi-statement lambda" guideline.
Comment on lines
47
to
+50
| process.stop(); | ||
|
|
||
| /* stop() is non-blocking; give the event loop time to receive the response */ | ||
| QTest::qWait(500); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "tst_adapterprocess.cpp" -o -name "adapterprocess.cpp" -o -name "adapterprocess.h" | head -20Repository: ModbusScope/NotOnlyModbusScope
Length of output: 200
🏁 Script executed:
cat -n tests/ProtocolAdapter/tst_adapterprocess.cpp | sed -n '40,85p'Repository: ModbusScope/NotOnlyModbusScope
Length of output: 1690
🏁 Script executed:
cat -n src/ProtocolAdapter/adapterprocess.cpp | grep -A 10 -B 5 "cStopTimeoutMs\|stop()"Repository: ModbusScope/NotOnlyModbusScope
Length of output: 1611
🏁 Script executed:
cat -n src/ProtocolAdapter/adapterprocess.h | grep -A 15 "stop()"Repository: ModbusScope/NotOnlyModbusScope
Length of output: 673
These fixed 500 ms waits are shorter than the stop contract and introduce race conditions.
AdapterProcess::stop() may legitimately take up to 3000 ms before the forced-kill path runs (line 54 in adapterprocess.cpp). Both sleeps at lines 50 and 73 are insufficient and timing-dependent. processFinishedEmittedOnStop() can fail on slower systems during valid graceful shutdown, and sendRequestEmitsResponseReceived() relies on fixed delay rather than signal arrival. Replace these with signal-driven waits: use QSignalSpy::wait() with timeout matching cStopTimeoutMs (3000 ms) instead of QTest::qWait(500).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ProtocolAdapter/tst_adapterprocess.cpp` around lines 47 - 50, Replace
the fixed QTest::qWait(500) sleeps in tst_adapterprocess.cpp with signal-driven
waits: after calling AdapterProcess::stop() use a QSignalSpy that watches the
process finished signal (the same signal used in processFinishedEmittedOnStop())
and call spy.wait(cStopTimeoutMs) so the test waits up to cStopTimeoutMs (3000
ms) instead of a fixed 500ms; similarly, in sendRequestEmitsResponseReceived()
replace the fixed qWait with a QSignalSpy on the response-received signal used
by the test and call spy.wait(cStopTimeoutMs). Ensure you reference the same
signal names the tests currently expect and remove the QTest::qWait calls.
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
2 participants
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.
Summary by CodeRabbit
New Features
Configuration
Documentation
Bug Fixes
Tests