Update SetInteriorVehicleDataRequestTests #2
Conversation
Check if read only parameters are cut off before sending a message to HMI.
| @@ -186,38 +188,36 @@ TEST_F(SetInteriorVehicleDataRequestTest, | |||
| TEST_F( | |||
| SetInteriorVehicleDataRequestTest, | |||
| Execute_ValidWithSettableAndReadOnlyParams_ExpectCutReadOnlyAndResendToHMI) { | |||
| // Arrange | |||
There was a problem hiding this comment.
@mvorobio What is the reason to remove such comments?
There was a problem hiding this comment.
@AByzhynar These comments are useless enough, because a pattern they introduce is sometimes difficult to follow. E.g. expectations from the ASSERT section could be placed around the ACT section.
In other words, such comments rarely give useful info but add unnecessary lines of code and sometimes even confuse.
But it's my personal opinion only. I wouldn't argue if these comments are required by some reasons.
There was a problem hiding this comment.
@mvorobio Please return them back. It is the full responsibility of the developer to put the code to the right section.
There was a problem hiding this comment.
@AByzhynar please check 794597b
|
|
||
| // Expectations |
There was a problem hiding this comment.
@mvorobio What is the reason to remove such comments?
There was a problem hiding this comment.
@AByzhynar please check 794597b
|
|
||
| EXPECT_CALL( | ||
| mock_rpc_service_, | ||
| ManageHMICommand( | ||
| HMIResultCodeIs(hmi_apis::FunctionID::RC_SetInteriorVehicleData), _)) | ||
| .WillOnce(Return(true)); | ||
|
|
||
| // Act |
There was a problem hiding this comment.
@mvorobio What is the reason to remove such comments?
There was a problem hiding this comment.
@AByzhynar please check 794597b
|
|
||
| EXPECT_CALL(app_mngr_, RemoveHMIFakeParameters(_, _)); | ||
| EXPECT_CALL(mock_policy_handler_, CheckModule(kPolicyAppId, _)) | ||
| .WillOnce(Return(true)); | ||
|
|
||
| EXPECT_CALL( | ||
| mock_rpc_service_, | ||
| ManageHMICommand( |
There was a problem hiding this comment.
@mvorobio Do we also need to check what is contained inside message?
There was a problem hiding this comment.
@ydementieiev please check 794597b
| @@ -186,38 +188,36 @@ TEST_F(SetInteriorVehicleDataRequestTest, | |||
| TEST_F( | |||
| SetInteriorVehicleDataRequestTest, | |||
| Execute_ValidWithSettableAndReadOnlyParams_ExpectCutReadOnlyAndResendToHMI) { | |||
| // Arrange | |||
There was a problem hiding this comment.
@mvorobio Please return them back. It is the full responsibility of the developer to put the code to the right section.
| climate_control_data[message_params::kFanSpeed] = 10; | ||
| msg_params[kModuleData][kModuleType] = mobile_apis::ModuleType::CLIMATE; | ||
| auto& control_data = msg_params[kModuleData][kClimateControlData]; | ||
| const uint64_t fan_speed = 10; |
There was a problem hiding this comment.
@mvorobio Don't use implicit conversions. Please add postfix u to the number.
There was a problem hiding this comment.
@AByzhynar please check 30f301a
I'd prefer const auto fan_speed = static_cast<uint64_t>(10);
| ASSERT_TRUE(command->Init()); | ||
| command->Run(); | ||
|
|
||
| EXPECT_TRUE(control_data.keyExists(kFanSpeed) && |
There was a problem hiding this comment.
@mvorobio please transform this expression to 2 separate expectations.
There was a problem hiding this comment.
@AByzhynar this is a single expectation - just ensures that a key exists prior an attempt to retrieve a value.
| ASSERT_TRUE(command->Init()); | ||
| command->Run(); | ||
|
|
||
| EXPECT_TRUE(control_data.keyExists(kFanSpeed) && | ||
| fan_speed == control_data[kFanSpeed].asUInt()); |
There was a problem hiding this comment.
@AByzhynar please check 30f301a
I think it doesn't matter due to fan_speed is constant.
| msg_params[kModuleData][kModuleType] = mobile_apis::ModuleType::RADIO; | ||
| auto& control_data = msg_params[kModuleData][kRadioControlData]; | ||
| control_data[kState] = true; | ||
| const auto radio_enable = true; |
There was a problem hiding this comment.
@mvorobio can't see any reason to use auto for explicit bool value.
There was a problem hiding this comment.
@AByzhynar please check 30f301a
There is no obvious benefit of auto instead of explicit bool in this case. But I think use of auto where it is possible at least helps think about objects not their types.
| ASSERT_TRUE(command->Init()); | ||
| command->Run(); | ||
|
|
||
| EXPECT_FALSE(control_data.keyExists(kState)); | ||
| EXPECT_TRUE(control_data.keyExists(kRadioEnable) && |
There was a problem hiding this comment.
@mvorobio please transform this expression to 2 separate expectations.
There was a problem hiding this comment.
@AByzhynar please see above.
|
The changes moved to LuxoftSDL#19 |
This PR is not ready for review.
Risk
This PR makes no API changes.
Summary
The PR adds a check whether read only parameters are cut off before sending a message to
HMI in unit tests for SetInteriorVehicleDataRequest.
Tasks Remaining:
CLA