Open
Conversation
WalkthroughThis pull request introduces three new JSON-RPC methods ( Changes
Sequence Diagram(s)sequenceDiagram
participant ModbusPoll
participant AdapterClient
participant Adapter
participant SettingsModel
participant AddRegisterWidget
ModbusPoll->>AdapterClient: onDescribeResult() → requestRegisterSchema()
AdapterClient->>Adapter: JSON-RPC adapter.registerSchema
Adapter-->>AdapterClient: { addressSchema, dataTypes, defaultDataType }
AdapterClient-->>ModbusPoll: registerSchemaResult(schema)
ModbusPoll->>SettingsModel: setAdapterRegisterSchema(adapterId, schema)
SettingsModel->>SettingsModel: Store in AdapterData
SettingsModel-->>ModbusPoll: adapterDataChanged(adapterId)
AddRegisterWidget->>SettingsModel: adapterData(adapterId).registerSchema()
AddRegisterWidget->>AddRegisterWidget: Build SchemaFormWidget from schema
AddRegisterWidget->>AddRegisterWidget: Populate dataType combo from schema
Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 5
🧹 Nitpick comments (5)
src/models/adapterdata.cpp (1)
46-49: Add Doxygen comments for the newly added public methods.Line 46 and Line 91 introduce public API without the source-level brief docs used for the rest of this file.
📝 Proposed fix
+/*! + * \brief Stores the adapter-provided register schema JSON. + * \param schema Register schema object from adapter.registerSchema. + */ void AdapterData::setRegisterSchema(const QJsonObject& schema) { _registerSchema = schema; } @@ +/*! + * \brief Returns the stored adapter register schema JSON. + */ QJsonObject AdapterData::registerSchema() const { return _registerSchema; }As per coding guidelines, "Document all public functions with brief Doxygen comments in the source file".
Also applies to: 91-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/models/adapterdata.cpp` around lines 46 - 49, Add brief Doxygen comments above the new public functions to match the file’s existing style: add a /// `@brief` line (and `@param` / `@return` where applicable) immediately above AdapterData::setRegisterSchema(const QJsonObject& schema) describing what the setter does and its parameter, and do the same for the other newly added public method(s) introduced at lines 91-94 (the register-schema-related getter), including a short `@return` description; follow the same comment formatting used by the other public functions in this source file.tests/ProtocolAdapter/tst_adapterclient.cpp (1)
724-762: Consider addingvalidateRegisterInActiveStatetest.
describeRegisterhas tests for bothAWAITING_CONFIG(line 666) andACTIVE(line 691) states, butvalidateRegisteronly tests theAWAITING_CONFIGstate. Since the implementation (context snippet 4) permits both states, a test forACTIVEwould ensure parity.🧪 Proposed additional test case
void TestAdapterClient::validateRegisterInActiveState() { auto* mock = new MockAdapterProcess(); AdapterClient client(mock); QSignalSpy spyValidate(&client, &AdapterClient::validateRegisterResult); driveToActive(client, mock); client.validateRegister(QStringLiteral("${40001: 16b}")); QCOMPARE(mock->sentRequests.last().method, QStringLiteral("adapter.validateRegister")); mock->injectResponse(5, "adapter.validateRegister", QJsonObject{ { "valid", true } }); QCOMPARE(spyValidate.count(), 1); QCOMPARE(spyValidate.at(0).at(0).toBool(), true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ProtocolAdapter/tst_adapterclient.cpp` around lines 724 - 762, Add a new test that mirrors the existing validateRegister coverage for AWAITING_CONFIG but runs in the ACTIVE state: create a TestAdapterClient::validateRegisterInActiveState that constructs MockAdapterProcess and AdapterClient, attaches a QSignalSpy to AdapterClient::validateRegisterResult, calls driveToActive(client, mock), invokes client.validateRegister(...) and asserts the last sent request method is "adapter.validateRegister", then injects a successful response via mock->injectResponse(...) and verifies the spy received the expected valid=true result; reference validateRegister, validateRegisterResult, driveToActive, MockAdapterProcess and AdapterClient when adding this test to ensure parity with the describeRegister ACTIVE case.src/communication/modbuspoll.cpp (1)
128-131: Add Doxygen comment for the new slot.The new
onRegisterSchemaResultslot is missing a brief Doxygen comment, unlike the other handlers in this file (e.g.,onAdapterDiagnosticat line 133 has one). As per coding guidelines: "Document all public functions with brief Doxygen comments in the source file".📝 Proposed Doxygen comment
+/*! \brief Store the adapter's register schema in settings. + * + * Called when the adapter responds to an adapter.registerSchema request. + * \param schema The full register schema object from the adapter. + */ void ModbusPoll::onRegisterSchemaResult(const QJsonObject& schema) { _pSettingsModel->setAdapterRegisterSchema("modbus", schema); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/communication/modbuspoll.cpp` around lines 128 - 131, Add a brief Doxygen comment above the ModbusPoll::onRegisterSchemaResult slot similar to other handlers (e.g., onAdapterDiagnostic) describing that this slot receives the adapter register schema and passes it to the settings model; place the short /** ... */ comment immediately above the onRegisterSchemaResult declaration/definition, mention parameters (const QJsonObject& schema) and the brief purpose (calls _pSettingsModel->setAdapterRegisterSchema("modbus", schema)), and follow the project's one-line Doxygen style for public functions.tests/models/tst_adapterdata.cpp (1)
134-137: Redundant null-pointer guard afterQVERIFY.
QVERIFY(data != nullptr)at line 133 already aborts the test on failure, making the subsequentif (data == nullptr) return;unreachable. This pattern was likely added for static analysis suppression, but it adds confusion without benefit.🔧 Option: remove the redundant check
const AdapterData* data = model.adapterData("modbus"); QVERIFY(data != nullptr); - if (data == nullptr) - { - return; - } QVERIFY(data->name().isEmpty());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/models/tst_adapterdata.cpp` around lines 134 - 137, Remove the redundant null-pointer guard that follows the test assertion: the QVERIFY(data != nullptr) already aborts on failure, so delete the subsequent if (data == nullptr) { return; } block; locate the QVERIFY(data != nullptr) assertion and remove the explicit post-check for the pointer named data (or replace it with an appropriate static-analysis annotation only if required).src/dialogs/addregisterwidget.cpp (1)
13-18: Add source Doxygen for the public constructor.The constructor definition was changed here, but the source file still has no brief Doxygen block for it. As per coding guidelines, "Document all public functions with brief Doxygen comments in the source file".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/dialogs/addregisterwidget.cpp` around lines 13 - 18, Add a brief Doxygen comment immediately above the AddRegisterWidget constructor definition (AddRegisterWidget::AddRegisterWidget) in the source file: include a one-line \brief describing the constructor and short `@param` tags for SettingsModel* pSettingsModel, const QString& adapterId and QWidget* parent; use the project Doxygen style (e.g. /*! \brief ... */ or ///) so the public constructor is documented per guidelines.
🤖 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/dialogs/addregisterwidget.cpp`:
- Around line 108-123: AddRegisterWidget::generateExpression currently assumes
SchemaFormWidget::values() contains "objectType" and "address" and
unconditionally calls ExpressionGenerator::constructRegisterString, which
hard-wires a Modbus schema; instead, check _pAddressForm->values() for the
presence and validity of "objectType" and "address" before using them (e.g. only
call constructRegisterString when both keys exist and are of expected types) or,
preferable, delegate expression construction to an adapter-facing API on the
address form (e.g. add a method on the form like
createRegisterExpression()/toExpression() that returns an optional string) and
call that from generateExpression so different adapters can supply their own
serialization; ensure you still handle deviceId/typeId extraction via
_pUi->cmbDevice and _pUi->cmbType but do not assume address keys exist when
delegating.
- Around line 54-60: The device combo population must handle an empty device
list: after calling _pSettingsModel->deviceListForAdapter(adapterId) check if
deviceList is empty and, if so, disable or hide the Add action (e.g.,
_pUi->btnAdd->setEnabled(false)) and disable the combo
(_pUi->cmbDevice->setEnabled(false)); also ensure the later code that currently
falls back to Device::cFirstDeviceId uses the combo count/currentIndex to detect
"no selection" and abort/return instead of emitting a register for a
non-existent device (update the selection logic that reads cmbDevice and the
fallback to Device::cFirstDeviceId).
In `@src/dialogs/registerdialog.cpp`:
- Around line 69-71: The code constructs AddRegisterWidget using adapterId even
when adapterIds() is empty (adapterId == QString()), which can leave the form
uninitialised; change the logic so you only create registerPopupMenu when a
valid adapter ID exists — e.g., compute ids via _pSettingsModel->adapterIds(),
find the first non-empty/valid adapter id (or test adapter validity via an
existing model method), and only call new AddRegisterWidget(_pSettingsModel,
adapterId, this) when adapterId is non-empty and valid; otherwise avoid creating
registerPopupMenu (or create a disabled/placeholder widget) and ensure any UI
actions that open it are disabled.
In `@src/util/expressiongenerator.cpp`:
- Around line 22-43: The function objectTypeToAddressPrefix currently defaults
unknown object types to "h"; change it to fail fast instead: validate the input
against the allowed values
("coil","discrete-input","input-register","holding-register") in
objectTypeToAddressPrefix and, on any other value, throw a descriptive exception
(e.g., std::invalid_argument with the offending objectType) or return an
explicit error indicator (empty QString) and log the error — choose the
project-consistent error strategy and update callers to handle the failure.
Ensure the error message includes the invalid objectType so typos/future values
are obvious, and remove the silent fallback to "h".
In `@tests/dialogs/tst_addregisterwidget.cpp`:
- Around line 63-68: The test fixture's shared _settingsModel is retaining state
across tests because registerDevice() adds a device; fix by resetting or
recreating the model for each test in TestAddRegisterWidget::init() (or fully
clearing it in cleanup()). Concretely, replace the current init() setup with
code that reinitializes _settingsModel (e.g. _settingsModel = SettingsModel();)
before calling _settingsModel.setAdapterRegisterSchema(...) and creating
AddRegisterWidget, or add an explicit clear/reset call (e.g.
_settingsModel.clearDevices() or similar API) in cleanup() so registerDevice()
side-effects do not persist between tests. Ensure you modify
TestAddRegisterWidget::init(), _settingsModel usage, and/or
TestAddRegisterWidget::cleanup() accordingly.
---
Nitpick comments:
In `@src/communication/modbuspoll.cpp`:
- Around line 128-131: Add a brief Doxygen comment above the
ModbusPoll::onRegisterSchemaResult slot similar to other handlers (e.g.,
onAdapterDiagnostic) describing that this slot receives the adapter register
schema and passes it to the settings model; place the short /** ... */ comment
immediately above the onRegisterSchemaResult declaration/definition, mention
parameters (const QJsonObject& schema) and the brief purpose (calls
_pSettingsModel->setAdapterRegisterSchema("modbus", schema)), and follow the
project's one-line Doxygen style for public functions.
In `@src/dialogs/addregisterwidget.cpp`:
- Around line 13-18: Add a brief Doxygen comment immediately above the
AddRegisterWidget constructor definition (AddRegisterWidget::AddRegisterWidget)
in the source file: include a one-line \brief describing the constructor and
short `@param` tags for SettingsModel* pSettingsModel, const QString& adapterId
and QWidget* parent; use the project Doxygen style (e.g. /*! \brief ... */ or
///) so the public constructor is documented per guidelines.
In `@src/models/adapterdata.cpp`:
- Around line 46-49: Add brief Doxygen comments above the new public functions
to match the file’s existing style: add a /// `@brief` line (and `@param` / `@return`
where applicable) immediately above AdapterData::setRegisterSchema(const
QJsonObject& schema) describing what the setter does and its parameter, and do
the same for the other newly added public method(s) introduced at lines 91-94
(the register-schema-related getter), including a short `@return` description;
follow the same comment formatting used by the other public functions in this
source file.
In `@tests/models/tst_adapterdata.cpp`:
- Around line 134-137: Remove the redundant null-pointer guard that follows the
test assertion: the QVERIFY(data != nullptr) already aborts on failure, so
delete the subsequent if (data == nullptr) { return; } block; locate the
QVERIFY(data != nullptr) assertion and remove the explicit post-check for the
pointer named data (or replace it with an appropriate static-analysis annotation
only if required).
In `@tests/ProtocolAdapter/tst_adapterclient.cpp`:
- Around line 724-762: Add a new test that mirrors the existing validateRegister
coverage for AWAITING_CONFIG but runs in the ACTIVE state: create a
TestAdapterClient::validateRegisterInActiveState that constructs
MockAdapterProcess and AdapterClient, attaches a QSignalSpy to
AdapterClient::validateRegisterResult, calls driveToActive(client, mock),
invokes client.validateRegister(...) and asserts the last sent request method is
"adapter.validateRegister", then injects a successful response via
mock->injectResponse(...) and verifies the spy received the expected valid=true
result; reference validateRegister, validateRegisterResult, driveToActive,
MockAdapterProcess and AdapterClient when adding this test to ensure parity with
the describeRegister ACTIVE case.
🪄 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: 43887b6d-34d2-446d-9755-490bdc7e915a
📒 Files selected for processing (23)
adapters/json-rpc-spec.mdsrc/ProtocolAdapter/adapterclient.cppsrc/ProtocolAdapter/adapterclient.hsrc/communication/modbuspoll.cppsrc/communication/modbuspoll.hsrc/dialogs/addregisterwidget.cppsrc/dialogs/addregisterwidget.hsrc/dialogs/addregisterwidget.uisrc/dialogs/registerdialog.cppsrc/importexport/mbcregisterdata.cppsrc/models/adapterdata.cppsrc/models/adapterdata.hsrc/models/settingsmodel.cppsrc/models/settingsmodel.hsrc/util/expressiongenerator.cppsrc/util/expressiongenerator.hsrc/util/expressionregex.cpptests/ProtocolAdapter/tst_adapterclient.cpptests/ProtocolAdapter/tst_adapterclient.htests/dialogs/tst_addregisterwidget.cpptests/dialogs/tst_addregisterwidget.htests/models/tst_adapterdata.cpptests/models/tst_adapterdata.h
Comment on lines
+54
to
60
| /* Populate device combo */ | ||
| _pUi->cmbDevice->clear(); | ||
| const auto deviceList = _pSettingsModel->deviceList(); | ||
| const auto deviceList = _pSettingsModel->deviceListForAdapter(adapterId); | ||
| for (deviceId_t devId : std::as_const(deviceList)) | ||
| { | ||
| _pUi->cmbDevice->addItem(QString(tr("Device %1").arg(devId)), devId); | ||
| } |
There was a problem hiding this comment.
Do not allow Add when no device matches this adapter.
If deviceListForAdapter(adapterId) is empty, the combo stays blank, but Line 116 later falls back to Device::cFirstDeviceId and still emits an expression. That lets the dialog create a register for a device that does not exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dialogs/addregisterwidget.cpp` around lines 54 - 60, The device combo
population must handle an empty device list: after calling
_pSettingsModel->deviceListForAdapter(adapterId) check if deviceList is empty
and, if so, disable or hide the Add action (e.g.,
_pUi->btnAdd->setEnabled(false)) and disable the combo
(_pUi->cmbDevice->setEnabled(false)); also ensure the later code that currently
falls back to Device::cFirstDeviceId uses the combo count/currentIndex to detect
"no selection" and abort/return instead of emitting a register for a
non-existent device (update the selection logic that reads cmbDevice and the
fallback to Device::cFirstDeviceId).
Comment on lines
108
to
+123
| QString AddRegisterWidget::generateExpression() | ||
| { | ||
| deviceId_t deviceId; | ||
| Type type; | ||
| ObjectType objectType; | ||
|
|
||
| QVariant typeData = _pUi->cmbType->currentData(); | ||
| if (typeData.canConvert<Type>()) | ||
| { | ||
| type = typeData.value<Type>(); | ||
| } | ||
| else | ||
| { | ||
| type = Type::UNSIGNED_16; | ||
| } | ||
| const QJsonObject addressValues = _pAddressForm->values(); | ||
| const QString objectType = addressValues["objectType"].toString(); | ||
| const int address = addressValues["address"].toInt(); | ||
|
|
||
| QVariant objectTypeData = _pUi->cmbObjectType->currentData(); | ||
| if (objectTypeData.canConvert<ObjectType>()) | ||
| { | ||
| objectType = objectTypeData.value<ObjectType>(); | ||
| } | ||
| else | ||
| { | ||
| objectType = ObjectType::UNKNOWN; | ||
| } | ||
|
|
||
| auto registerAddr = ModbusAddress(static_cast<quint32>(_pUi->spinAddress->value()), objectType); | ||
| const QString typeId = _pUi->cmbType->currentData().toString(); | ||
|
|
||
| QVariant devData = _pUi->cmbDevice->currentData(); | ||
| deviceId_t deviceId = Device::cFirstDeviceId; | ||
| const QVariant devData = _pUi->cmbDevice->currentData(); | ||
| if (devData.canConvert<deviceId_t>()) | ||
| { | ||
| deviceId = devData.value<deviceId_t>(); | ||
| } | ||
| else | ||
| { | ||
| deviceId = 0; | ||
| } | ||
|
|
||
| return ExpressionGenerator::constructRegisterString(registerAddr.fullAddress(), type, deviceId); | ||
| return ExpressionGenerator::constructRegisterString(objectType, address, typeId, deviceId); |
There was a problem hiding this comment.
This serialisation path is still hard-wired to a Modbus-shaped schema.
SchemaFormWidget::values() only returns keys that are actually present, but this code unconditionally reads objectType and address and serialises them straight into a Modbus register string. Any adapter that needs different address fields, or omits either key, can render a form but cannot be round-tripped correctly here. Please either tighten the RPC contract to those exact keys or move expression construction behind an adapter-facing API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dialogs/addregisterwidget.cpp` around lines 108 - 123,
AddRegisterWidget::generateExpression currently assumes
SchemaFormWidget::values() contains "objectType" and "address" and
unconditionally calls ExpressionGenerator::constructRegisterString, which
hard-wires a Modbus schema; instead, check _pAddressForm->values() for the
presence and validity of "objectType" and "address" before using them (e.g. only
call constructRegisterString when both keys exist and are of expected types) or,
preferable, delegate expression construction to an adapter-facing API on the
address form (e.g. add a method on the form like
createRegisterExpression()/toExpression() that returns an optional string) and
call that from generateExpression so different adapters can supply their own
serialization; ensure you still handle deviceId/typeId extraction via
_pUi->cmbDevice and _pUi->cmbType but do not assume address keys exist when
delegating.
Comment on lines
+69
to
+71
| const QStringList ids = _pSettingsModel->adapterIds(); | ||
| const QString adapterId = ids.isEmpty() ? QString() : ids.first(); | ||
| auto registerPopupMenu = new AddRegisterWidget(_pSettingsModel, adapterId, this); |
There was a problem hiding this comment.
Avoid constructing AddRegisterWidget with an empty or non-schema adapter ID.
Line 70 falls back to an empty ID, and Line 71 still creates the popup. That can leave the add-register form uninitialised (empty schema/device state) instead of selecting a usable adapter.
💡 Proposed fix
- const QStringList ids = _pSettingsModel->adapterIds();
- const QString adapterId = ids.isEmpty() ? QString() : ids.first();
- auto registerPopupMenu = new AddRegisterWidget(_pSettingsModel, adapterId, this);
- connect(registerPopupMenu, &AddRegisterWidget::graphDataConfigured, this, &RegisterDialog::addRegister);
-
- _registerPopupAction = std::make_unique<QWidgetAction>(this);
- _registerPopupAction->setDefaultWidget(registerPopupMenu);
- _pUi->btnAdd->addAction(_registerPopupAction.get());
+ QString adapterId;
+ const QStringList ids = _pSettingsModel->adapterIds();
+ for (const QString& id : ids)
+ {
+ const AdapterData* pAdapter = _pSettingsModel->adapterData(id);
+ if (pAdapter != nullptr && !pAdapter->registerSchema().isEmpty())
+ {
+ adapterId = id;
+ break;
+ }
+ }
+
+ if (!adapterId.isEmpty())
+ {
+ auto* registerPopupMenu = new AddRegisterWidget(_pSettingsModel, adapterId, this);
+ connect(registerPopupMenu, &AddRegisterWidget::graphDataConfigured, this, &RegisterDialog::addRegister);
+
+ _registerPopupAction = std::make_unique<QWidgetAction>(this);
+ _registerPopupAction->setDefaultWidget(registerPopupMenu);
+ _pUi->btnAdd->addAction(_registerPopupAction.get());
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/dialogs/registerdialog.cpp` around lines 69 - 71, The code constructs
AddRegisterWidget using adapterId even when adapterIds() is empty (adapterId ==
QString()), which can leave the form uninitialised; change the logic so you only
create registerPopupMenu when a valid adapter ID exists — e.g., compute ids via
_pSettingsModel->adapterIds(), find the first non-empty/valid adapter id (or
test adapter validity via an existing model method), and only call new
AddRegisterWidget(_pSettingsModel, adapterId, this) when adapterId is non-empty
and valid; otherwise avoid creating registerPopupMenu (or create a
disabled/placeholder widget) and ensure any UI actions that open it are
disabled.
Comment on lines
+22
to
+43
| /*! | ||
| * \brief Returns the single-character address prefix for an object type string. | ||
| * \param objectType One of "coil", "discrete-input", "input-register", "holding-register". | ||
| * \return The corresponding prefix character ("c", "d", "i", or "h"). | ||
| * Returns "h" for unknown values as a safe default. | ||
| */ | ||
| QString objectTypeToAddressPrefix(const QString& objectType) | ||
| { | ||
| if (objectType == QStringLiteral("coil")) | ||
| { | ||
| QString suffix = ExpressionGenerator::typeSuffix(type); | ||
| QString connStr = devId != Device::cFirstDeviceId ? QString("@%1").arg(devId) : QString(); | ||
|
|
||
| return QString("${%1%2%3}").arg(registerAddress, connStr, suffix); | ||
| return QStringLiteral("c"); | ||
| } | ||
| if (objectType == QStringLiteral("discrete-input")) | ||
| { | ||
| return QStringLiteral("d"); | ||
| } | ||
| if (objectType == QStringLiteral("input-register")) | ||
| { | ||
| return QStringLiteral("i"); | ||
| } | ||
| return QStringLiteral("h"); | ||
| } |
There was a problem hiding this comment.
Reject unknown object types instead of defaulting to h.
The fallback in objectTypeToAddressPrefix() silently turns typos or future adapter values into holding-register expressions. That polls the wrong register instead of failing fast.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/util/expressiongenerator.cpp` around lines 22 - 43, The function
objectTypeToAddressPrefix currently defaults unknown object types to "h"; change
it to fail fast instead: validate the input against the allowed values
("coil","discrete-input","input-register","holding-register") in
objectTypeToAddressPrefix and, on any other value, throw a descriptive exception
(e.g., std::invalid_argument with the offending objectType) or return an
explicit error indicator (empty QString) and log the error — choose the
project-consistent error strategy and update callers to handle the failure.
Ensure the error message includes the invalid objectType so typos/future values
are obvious, and remove the silent fallback to "h".
Comment on lines
63
to
68
| void TestAddRegisterWidget::init() | ||
| { | ||
| _pRegWidget = new AddRegisterWidget(&_settingsModel); | ||
| _settingsModel.setAdapterRegisterSchema("modbus", buildTestRegisterSchema()); | ||
| _settingsModel.deviceSettings(Device::cFirstDeviceId)->setAdapterId("modbus"); | ||
| _pRegWidget = new AddRegisterWidget(&_settingsModel, QStringLiteral("modbus")); | ||
| } |
There was a problem hiding this comment.
Reset the SettingsModel fixture in init().
registerDevice() adds a second device to the shared _settingsModel, and that state survives into every later test because the model is a test-class member. The suite is now order-dependent; please rebuild the model per test or fully clear it in cleanup().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/dialogs/tst_addregisterwidget.cpp` around lines 63 - 68, The test
fixture's shared _settingsModel is retaining state across tests because
registerDevice() adds a device; fix by resetting or recreating the model for
each test in TestAddRegisterWidget::init() (or fully clearing it in cleanup()).
Concretely, replace the current init() setup with code that reinitializes
_settingsModel (e.g. _settingsModel = SettingsModel();) before calling
_settingsModel.setAdapterRegisterSchema(...) and creating AddRegisterWidget, or
add an explicit clear/reset call (e.g. _settingsModel.clearDevices() or similar
API) in cleanup() so registerDevice() side-effects do not persist between tests.
Ensure you modify TestAddRegisterWidget::init(), _settingsModel usage, and/or
TestAddRegisterWidget::cleanup() accordingly.
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.
Summary by CodeRabbit
Release Notes
New Features
Improvements