Initial test update #166
Initial test update #166
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR significantly expands the test coverage for the FirmwareUpdate plugin. The changes include adding new test cases, helper methods, mock implementations, and improved code organization.
- Added comprehensive test coverage for firmware update operations including various device types, firmware types, and error scenarios
- Introduced NotificationHandlerMock for testing event notifications
- Added helper methods for test setup/teardown and event triggering
- Extended test cases for state management, auto-reboot functionality, and edge cases
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Core::JSONRPC::Message message; | ||
| string response; | ||
| WrapsImplMock *p_wrapsImplMock = nullptr ; | ||
| WrapsImplMock *p_wrapsImplMock = nullptr; |
There was a problem hiding this comment.
[nitpick] Inconsistent spacing: multiple spaces after variable type and before assignment operator. Should use single spaces for consistency.
| , INIT_CONX(1, 0) | ||
| , workerPool(Core::ProxyType<WorkerPoolImplementation>::Create( | ||
| 2, Core::Thread::DefaultStackSize(), 16)) | ||
| , notificationMock(std::make_unique<NotificationHandlerMock>()) |
There was a problem hiding this comment.
Inconsistent indentation: using tab character while other lines use spaces. Should maintain consistent indentation style throughout the file.
| , notificationMock(std::make_unique<NotificationHandlerMock>()) | |
| , notificationMock(std::make_unique<NotificationHandlerMock>()) |
| { | ||
|
|
||
| p_wrapsImplMock = new testing::NiceMock <WrapsImplMock>; | ||
| p_wrapsImplMock = new testing::NiceMock<WrapsImplMock>; |
There was a problem hiding this comment.
Inconsistent indentation: mixing tabs and spaces. Lines 101, 103, 105-106 use tabs while surrounding code uses spaces for indentation.
|
|
||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| EXPECT_EQ(string(""), plugin->Initialize(&service)); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); |
There was a problem hiding this comment.
[nitpick] Hard-coded sleep durations in tests can lead to flaky tests. Consider using more deterministic synchronization mechanisms or making the sleep duration configurable.
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Using system() call can be a security risk and is not portable. Consider using C++ filesystem library (std::filesystem::create_directories) instead.
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/tmp/path/to/test_firmware.bin"); | ||
| system("rm -rf /tmp/path"); |
There was a problem hiding this comment.
Using system() with rm -rf is dangerous and not portable. Consider using C++ filesystem library (std::filesystem::remove_all) instead.
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/usb_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/tmp/usb_firmware.bin"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/usb_firmware.bin")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("test_firmware.bin"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("test_firmware.bin")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
Disable the PACKAGER and MAINTENANCEMANAGER plugins in the L1 tests workflow.
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| , workerPool(Core::ProxyType<WorkerPoolImplementation>::Create( | ||
| 2, Core::Thread::DefaultStackSize(), 16)) | ||
| , notificationMock(std::make_unique<NotificationHandlerMock>()) | ||
| , flashInProgress(false) |
There was a problem hiding this comment.
Trailing whitespace at end of line should be removed.
| , flashInProgress(false) | |
| , flashInProgress(false) |
| ON_CALL(*p_iarmBusImplMock, IARM_Bus_Call) | ||
| .WillByDefault(::testing::Invoke( | ||
| [this](const char* ownerName, const char* methodName, void* arg, size_t argLen) { | ||
| flashInProgress = true; |
There was a problem hiding this comment.
The flashInProgress flag is unconditionally set to true for all IARM_Bus_Call invocations in the constructor. This default behavior may interfere with tests that don't actually trigger firmware updates. Consider only setting this flag in tests that genuinely require tracking flash progress, or check the methodName parameter to set it conditionally.
| flashInProgress = true; | |
| // Only set flashInProgress for actual firmware update methods | |
| if (methodName != nullptr && | |
| (strcmp(methodName, "InitiateFirmwareFlash") == 0 || | |
| strcmp(methodName, "StartFirmwareUpdate") == 0)) { | |
| flashInProgress = true; | |
| } |
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Using system() calls in tests can be unreliable and creates a dependency on shell availability. Consider using C++ filesystem API (std::filesystem::create_directories) for better portability and error handling.
| TEST_F(FirmwareUpdateTest, UpdateFirmware_PostFlash_ValidPCI_Success) | ||
| { | ||
|
|
||
| createTestFirmwareFile(); | ||
|
|
||
| std::ofstream deviceProps("/etc/device.properties"); | ||
| deviceProps << "DEVICE_TYPE=mediaclient" << std::endl; | ||
| deviceProps.close(); | ||
|
|
||
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); | ||
| } |
There was a problem hiding this comment.
Test creates /etc/device.properties but doesn't verify if write succeeds or clean up on test failure. The cleanup at line 257 should be in TearDown() or use RAII pattern to ensure cleanup happens even if test fails.
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Core::JSONRPC::Message message; | ||
| string response; | ||
| WrapsImplMock *p_wrapsImplMock = nullptr ; | ||
| WrapsImplMock *p_wrapsImplMock = nullptr; |
There was a problem hiding this comment.
[nitpick] Inconsistent spacing in pointer declaration. The original line had extra spaces before '= nullptr' which were removed, but this line still has extra spaces after 'p_wrapsImplMock'. Consider consistent spacing: WrapsImplMock* p_wrapsImplMock = nullptr;
| WrapsImplMock *p_wrapsImplMock = nullptr; | |
| WrapsImplMock* p_wrapsImplMock = nullptr; |
| p_wrapsImplMock = new testing::NiceMock<WrapsImplMock>; | ||
| Wraps::setImpl(p_wrapsImplMock); | ||
| p_iarmBusImplMock = new NiceMock <IarmBusImplMock>; | ||
| p_iarmBusImplMock = new NiceMock<IarmBusImplMock>; | ||
| IarmBus::setImpl(p_iarmBusImplMock); | ||
| p_rfcApiImplMock = new testing::NiceMock<RfcApiImplMock>; | ||
| RfcApi::setImpl(p_rfcApiImplMock); |
There was a problem hiding this comment.
Inconsistent indentation: lines use a mix of tabs and spaces. Lines 101, 102, 104 use tabs followed by spaces, while lines 103, 105, 106 use only tabs. The entire block should use consistent indentation (preferably spaces to match the surrounding code).
| RfcApi::setImpl(nullptr); | ||
| if (p_rfcApiImplMock != nullptr) | ||
| { | ||
| delete p_rfcApiImplMock; | ||
| p_rfcApiImplMock = nullptr; | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation: using tab characters instead of spaces. This block should use consistent spacing to match the surrounding cleanup code (lines 162-167, 177-182).
| RfcApi::setImpl(nullptr); | |
| if (p_rfcApiImplMock != nullptr) | |
| { | |
| delete p_rfcApiImplMock; | |
| p_rfcApiImplMock = nullptr; | |
| } | |
| RfcApi::setImpl(nullptr); | |
| if (p_rfcApiImplMock != nullptr) | |
| { | |
| delete p_rfcApiImplMock; | |
| p_rfcApiImplMock = nullptr; | |
| } |
| // Helper method for testing events | ||
| void triggerFirmwareEvent(const Exchange::IFirmwareUpdate::State state, | ||
| const Exchange::IFirmwareUpdate::SubState substate) { | ||
| // Register notification handler if not already registered | ||
| Core::hresult result = FirmwareUpdateImpl->Register(notificationMock.get()); | ||
| EXPECT_TRUE(result == Core::ERROR_NONE || result == Core::ERROR_ALREADY_CONNECTED); | ||
|
|
||
| // Trigger update state change through plugin handler | ||
| string stateString = (state == Exchange::IFirmwareUpdate::State::FLASHING_SUCCEEDED) ? "FLASHING_SUCCEEDED" : | ||
| (state == Exchange::IFirmwareUpdate::State::FLASHING_FAILED) ? "FLASHING_FAILED" : "IDLE"; | ||
|
|
||
| string substateString = (substate == Exchange::IFirmwareUpdate::SubState::NOT_APPLICABLE) ? "NOT_APPLICABLE" : "GENERIC_ERROR"; | ||
|
|
||
| string updateStatusRequest = "{\"state\":\"" + stateString + "\",\"substate\":\"" + substateString + "\"}"; | ||
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("onUpdateStateChange"), updateStatusRequest, response)); | ||
| } | ||
|
|
There was a problem hiding this comment.
The helper method triggerFirmwareEvent is defined but appears unused based on the visible test cases. If this method is intended for future use, consider adding a comment explaining its purpose. If it's not used, it should be removed.
| // Helper method for testing events | |
| void triggerFirmwareEvent(const Exchange::IFirmwareUpdate::State state, | |
| const Exchange::IFirmwareUpdate::SubState substate) { | |
| // Register notification handler if not already registered | |
| Core::hresult result = FirmwareUpdateImpl->Register(notificationMock.get()); | |
| EXPECT_TRUE(result == Core::ERROR_NONE || result == Core::ERROR_ALREADY_CONNECTED); | |
| // Trigger update state change through plugin handler | |
| string stateString = (state == Exchange::IFirmwareUpdate::State::FLASHING_SUCCEEDED) ? "FLASHING_SUCCEEDED" : | |
| (state == Exchange::IFirmwareUpdate::State::FLASHING_FAILED) ? "FLASHING_FAILED" : "IDLE"; | |
| string substateString = (substate == Exchange::IFirmwareUpdate::SubState::NOT_APPLICABLE) ? "NOT_APPLICABLE" : "GENERIC_ERROR"; | |
| string updateStatusRequest = "{\"state\":\"" + stateString + "\",\"substate\":\"" + substateString + "\"}"; | |
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("onUpdateStateChange"), updateStatusRequest, response)); | |
| } |
| void triggerProgressEvent(uint32_t progress) { | ||
| // Register notification handler if not already registered | ||
| Core::hresult result = FirmwareUpdateImpl->Register(notificationMock.get()); | ||
| EXPECT_TRUE(result == Core::ERROR_NONE || result == Core::ERROR_ALREADY_CONNECTED); | ||
|
|
||
| // Trigger progress update through plugin handler | ||
| string progressRequest = "{\"progress\":" + std::to_string(progress) + "}"; | ||
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("onFlashingProgressChange"), progressRequest, response)); | ||
| } |
There was a problem hiding this comment.
The helper method triggerProgressEvent is defined but appears unused based on the visible test cases. If this method is intended for future use, consider adding a comment explaining its purpose. If it's not used, it should be removed.
| void triggerProgressEvent(uint32_t progress) { | |
| // Register notification handler if not already registered | |
| Core::hresult result = FirmwareUpdateImpl->Register(notificationMock.get()); | |
| EXPECT_TRUE(result == Core::ERROR_NONE || result == Core::ERROR_ALREADY_CONNECTED); | |
| // Trigger progress update through plugin handler | |
| string progressRequest = "{\"progress\":" + std::to_string(progress) + "}"; | |
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("onFlashingProgressChange"), progressRequest, response)); | |
| } |
| } | ||
| #endif | ||
|
|
||
| //Newtest ends |
There was a problem hiding this comment.
Comment has incorrect spacing and capitalization. Should be '// New test ends' or '// New tests end' with proper spacing after '//'.
| //Newtest ends | |
| // New tests end |
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
Refactor FirmwareUpdateTest constructor and cleanup methods. Simplify mock expectations and improve readability.
Removed unused mock calls for fclose and fgets in tests.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| TEST_F(FirmwareUpdateTest, UpdateFirmware_ValidPCIFirmware) | ||
| { | ||
| const char* filePath = "/tmp/ELTE11MWR_MIDDLEWARE_DEV_default_20241122145614.bin"; | ||
| std::ofstream outfile(filePath); | ||
| if (outfile.is_open()) { | ||
| outfile << "dummy firmware content"; | ||
| outfile.close(); | ||
| } | ||
|
|
||
| std::ofstream versionFile("/version.txt"); | ||
| if (versionFile.is_open()) { | ||
| versionFile << "imagename:DIFFERENT_IMAGE\n"; | ||
| versionFile.close(); | ||
| } | ||
|
|
||
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/ELTE11MWR_MIDDLEWARE_DEV_default_20241122145614.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
| EXPECT_TRUE(response.find("\"success\":true") != string::npos); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
| } |
There was a problem hiding this comment.
The test creates a temporary file and version.txt without cleanup. Add file cleanup in teardown or at test end to prevent test pollution and ensure test isolation. Multiple tests create similar files that may interfere with each other.
| TEST_F(FirmwareUpdateTest, UpdateFirmware_SymbolicLink) | ||
| { | ||
| const char* realPath = "/tmp/real_firmware.bin"; | ||
| const char* linkPath = "/tmp/link_firmware.bin"; | ||
|
|
||
| std::ofstream outfile(realPath); | ||
| if (outfile.is_open()) { | ||
| outfile << "dummy firmware content"; | ||
| outfile.close(); | ||
| } | ||
|
|
||
| symlink(realPath, linkPath); | ||
|
|
||
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/link_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| unlink(linkPath); | ||
| unlink(realPath); | ||
| } |
There was a problem hiding this comment.
The symlink() call may fail if the link already exists from a previous test run, causing the test to pass incorrectly. Check the return value of symlink() or remove any existing link before creation to ensure test reliability.
| TEST_F(FirmwareUpdateTest, UpdateFirmware_DirectoryInsteadOfFile) | ||
| { | ||
| mkdir("/tmp/firmware_dir", 0755); | ||
|
|
||
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/firmware_dir\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| rmdir("/tmp/firmware_dir"); | ||
| } |
There was a problem hiding this comment.
The mkdir() call may fail if the directory already exists, causing the test to verify incorrect behavior. Check the return value of mkdir() or use mkdir with error checking to ensure the test setup is correct.
| TEST_F(FirmwareUpdateTest, UpdateFirmware_InvalidFilePath_SpecialCharacters) | ||
| { | ||
| const char* filePath = "/tmp/ELTE11MWR_@#$%^&*.bin"; | ||
| std::ofstream outfile(filePath); | ||
| if (outfile.is_open()) { | ||
| outfile << "dummy firmware content"; | ||
| outfile.close(); | ||
| } | ||
|
|
||
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/ELTE11MWR_@#$%^&*.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
| } |
There was a problem hiding this comment.
Test creates a file but does not clean it up. Add cleanup code (e.g., std::remove(filePath)) after the test assertion to maintain test isolation and prevent leftover files.
| TEST_F(FirmwareUpdateTest, UpdateFirmware_InvalidFilePath_WithSpaces) | ||
| { | ||
| const char* filePath = "/tmp/ELTE11MWR MIDDLEWARE.bin"; | ||
| std::ofstream outfile(filePath); | ||
| if (outfile.is_open()) { | ||
| outfile << "dummy firmware content"; | ||
| outfile.close(); | ||
| } | ||
|
|
||
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/ELTE11MWR MIDDLEWARE.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
| } |
There was a problem hiding this comment.
Test creates a file but does not clean it up. Add cleanup code (e.g., std::remove(filePath)) after the test assertion to maintain test isolation and prevent leftover files.
| outfile.close(); | ||
| } | ||
|
|
||
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/ELTE11MWR_MIDDLEWARE_DEV_default.txt\",\"firmwareType\":\"PCI\"}"), response)); |
There was a problem hiding this comment.
Test creates a file but does not clean it up. Add cleanup code (e.g., std::remove(filePath)) after the test assertion to maintain test isolation and prevent leftover files.
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/ELTE11MWR_MIDDLEWARE_DEV_default.txt\",\"firmwareType\":\"PCI\"}"), response)); | |
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/ELTE11MWR_MIDDLEWARE_DEV_default.txt\",\"firmwareType\":\"PCI\"}"), response)); | |
| std::remove(filePath); |
| const char* filePath = "/ELTE11MWR_MIDDLEWARE_DEV_default.bin"; | ||
| std::ofstream outfile(filePath); | ||
| if (outfile.is_open()) { | ||
| outfile << "dummy firmware content"; | ||
| outfile.close(); | ||
| } | ||
|
|
||
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/ELTE11MWR_MIDDLEWARE_DEV_default.bin\",\"firmwareType\":\"PCI\"}"), response)); |
There was a problem hiding this comment.
Test attempts to create a file in the root directory which requires elevated permissions and will likely fail. The test should either check file creation success or skip cleanup if creation fails, or use a mock/temp directory instead.
| const char* filePath = "/ELTE11MWR_MIDDLEWARE_DEV_default.bin"; | |
| std::ofstream outfile(filePath); | |
| if (outfile.is_open()) { | |
| outfile << "dummy firmware content"; | |
| outfile.close(); | |
| } | |
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/ELTE11MWR_MIDDLEWARE_DEV_default.bin\",\"firmwareType\":\"PCI\"}"), response)); | |
| // Create the file in /tmp to avoid permission issues, but test with root path | |
| const char* tmpFilePath = "/tmp/ELTE11MWR_MIDDLEWARE_DEV_default.bin"; | |
| std::ofstream outfile(tmpFilePath); | |
| if (outfile.is_open()) { | |
| outfile << "dummy firmware content"; | |
| outfile.close(); | |
| } | |
| // Pass the root path to the handler to test rejection | |
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/ELTE11MWR_MIDDLEWARE_DEV_default.bin\",\"firmwareType\":\"PCI\"}"), response)); | |
| // Cleanup: remove the file if it was created | |
| std::remove(tmpFilePath); |
| file.close(); | ||
|
|
||
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "mkdir("/tmp/firmware_dir", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/tmp/path/to/test_firmware.bin"); | ||
| system("rm -rf /tmp/path"); |
There was a problem hiding this comment.
Using system() with string literals is generally acceptable, but the test should verify the commands succeeded. Add error checking or use std::filesystem for safer directory operations.
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
Removed expectation for firmware update handler invocation.
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
…b.com/rdkcentral/entservices-softwareupdate into topic/firmwareUpdate-L1-tests-copilot
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Unless required by applicable law or agreed to in writing,software | ||
| * Unless required by applicable law or agreed to in writing,software |
There was a problem hiding this comment.
Duplicate line: The license comment line "Unless required by applicable law or agreed to in writing,software" appears twice. Additionally, there's a missing space after the comma before "software". This should be one line that reads: "Unless required by applicable law or agreed to in writing, software"
| * Unless required by applicable law or agreed to in writing,software | |
| * Unless required by applicable law or agreed to in writing,software | |
| * Unless required by applicable law or agreed to in writing, software |
| result.success = true; | ||
| status =Core::ERROR_NONE; | ||
|
|
||
| LOGINFO("[%s:%d] UpdateFirmware completed successfully, returning with status: Core::ERROR_NONE", __FUNCTION__, __LINE__); |
There was a problem hiding this comment.
[nitpick] Excessive logging: Adding LOGINFO statements at every return point and decision branch creates log noise and makes debugging harder. Consider consolidating logging to key decision points only (e.g., function entry, validation failures with specific reasons, and critical state changes). The FUNCTION and LINE macros are already included in most log statements, making this redundant information.
| LOGINFO("[%s:%d] UpdateFirmware completed successfully, returning with status: Core::ERROR_NONE", __FUNCTION__, __LINE__); |
| MOCK_METHOD(void, OnUpdateStateChange, (const Exchange::IFirmwareUpdate::State state, | ||
| const Exchange::IFirmwareUpdate::SubState substate), (override)); | ||
| MOCK_METHOD(void, OnFlashingStateChange, (const uint32_t percentageComplete), (override)); | ||
|
|
There was a problem hiding this comment.
Trailing whitespace: Remove the trailing spaces after the semicolon on this line.
| * Unless required by applicable law or agreed to in writing,software | ||
| * Unless required by applicable law or agreed to in writing,software |
There was a problem hiding this comment.
Missing space after the comma before "software". Should be: "Unless required by applicable law or agreed to in writing, software"
| * Unless required by applicable law or agreed to in writing,software | |
| * Unless required by applicable law or agreed to in writing,software | |
| * Unless required by applicable law or agreed to in writing, software | |
| * Unless required by applicable law or agreed to in writing, software |
| , INIT_CONX(1, 0) | ||
| , workerPool(Core::ProxyType<WorkerPoolImplementation>::Create( | ||
| 2, Core::Thread::DefaultStackSize(), 16)) | ||
| , notificationMock(std::make_unique<NotificationHandlerMock>()) |
There was a problem hiding this comment.
Inconsistent indentation: This line uses a tab character instead of spaces. The codebase appears to use spaces for indentation (4 spaces based on surrounding code). This should be aligned with the rest of the initialization list using spaces.
| , notificationMock(std::make_unique<NotificationHandlerMock>()) | |
| , notificationMock(std::make_unique<NotificationHandlerMock>()) |
| p_iarmBusImplMock = new NiceMock<IarmBusImplMock>; | ||
| IarmBus::setImpl(p_iarmBusImplMock); | ||
| p_rfcApiImplMock = new testing::NiceMock<RfcApiImplMock>; | ||
| RfcApi::setImpl(p_rfcApiImplMock); |
There was a problem hiding this comment.
Inconsistent indentation: These lines use tab characters for indentation instead of spaces. Convert tabs to spaces to match the rest of the codebase (appears to use 4 or 8 spaces based on context).
| RfcApi::setImpl(p_rfcApiImplMock); | |
| RfcApi::setImpl(p_rfcApiImplMock); |
|
|
||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| EXPECT_EQ(string(""), plugin->Initialize(&service)); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); |
There was a problem hiding this comment.
Inconsistent indentation: This line uses tab characters for indentation. Convert to spaces to match the rest of the codebase.
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | |
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); |
| // Implement IReferenceCounted interface | ||
| MOCK_METHOD(void, AddRef, (), (const, override)); | ||
| MOCK_METHOD(uint32_t, Release, (), (const, override)); | ||
|
|
There was a problem hiding this comment.
Trailing whitespace: Remove the trailing spaces after the semicolon on this line.
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #if 0 | ||
| TEST_F(FirmwareUpdateTest, UpdateFirmware_ValidPCI) | ||
| { | ||
| createTestFirmwareFile(); | ||
|
|
||
| string request = "{\"firmwareFilepath\":\"" + TEST_FIRMWARE_PATH + "\",\"firmwareType\":\"PCI\"}"; | ||
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("updateFirmware"), request, response)); | ||
| } | ||
|
|
||
| TEST_F(FirmwareUpdateTest, UpdateFirmware_ValidDRI) | ||
| { | ||
| createTestFirmwareFile(); | ||
|
|
||
| string request = "{\"firmwareFilepath\":\"" + TEST_FIRMWARE_PATH + "\",\"firmwareType\":\"DRI\"}"; | ||
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("updateFirmware"), request, response)); | ||
| } | ||
| #endif | ||
|
|
||
| TEST_F(FirmwareUpdateTest, UpdateFirmware_EmptyFirmwareFilepath) | ||
| { | ||
| string request = "{\"firmwareFilepath\":\"\",\"firmwareType\":\"PCI\"}"; | ||
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), request, response)); | ||
| } | ||
|
|
||
| TEST_F(FirmwareUpdateTest, UpdateFirmware_EmptyFirmwareType) | ||
| { | ||
| createTestFirmwareFile(); | ||
| string request = "{\"firmwareFilepath\":\"" + TEST_FIRMWARE_PATH + "\",\"firmwareType\":\"\"}"; | ||
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), request, response)); | ||
| } | ||
|
|
||
| TEST_F(FirmwareUpdateTest, UpdateFirmware_InvalidFirmwareType) | ||
| { | ||
| createTestFirmwareFile(); | ||
| string request = "{\"firmwareFilepath\":\"" + TEST_FIRMWARE_PATH + "\",\"firmwareType\":\"INVALID\"}"; | ||
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), request, response)); | ||
| } | ||
|
|
||
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FileNotExists) | ||
| { | ||
| string request = "{\"firmwareFilepath\":\"/nonexistent/firmware.bin\",\"firmwareType\":\"PCI\"}"; | ||
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), request, response)); | ||
| } | ||
| #if 0 | ||
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FileAccessDenied) | ||
| { | ||
| createTestFirmwareFile(); | ||
| string request = "{\"firmwareFilepath\":\"" + TEST_FIRMWARE_PATH + "\",\"firmwareType\":\"PCI\"}"; | ||
| EXPECT_EQ(Core::ERROR_PRIVILIGED_REQUEST, handler.Invoke(connection, _T("updateFirmware"), request, response)); | ||
| } | ||
|
|
||
| TEST_F(FirmwareUpdateTest, UpdateFirmware_UpToDateFirmware) | ||
| { | ||
| std::ofstream versionFile("/version.txt"); | ||
| versionFile << "imagename:test_firmware" << std::endl; | ||
| versionFile.close(); | ||
|
|
||
| createTestFirmwareFile(); | ||
| string request = "{\"firmwareFilepath\":\"" + TEST_FIRMWARE_PATH + "\",\"firmwareType\":\"PCI\"}"; | ||
| EXPECT_EQ(Core::ERROR_GENERAL, handler.Invoke(connection, _T("updateFirmware"), request, response)); | ||
|
|
||
| std::remove("/version.txt"); | ||
| } | ||
|
|
||
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashingInProgress) | ||
| { | ||
|
|
||
| createTestFirmwareFile(); | ||
|
|
||
| string request = "{\"firmwareFilepath\":\"" + TEST_FIRMWARE_PATH + "\",\"firmwareType\":\"PCI\"}"; | ||
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("updateFirmware"), request, response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
|
|
||
| EXPECT_EQ(Core::ERROR_GENERAL, handler.Invoke(connection, _T("updateFirmware"), request, response)); | ||
| } | ||
|
|
||
| TEST_F(FirmwareUpdateTest, UpdateFirmware_IARMConnectionFailed) | ||
| { | ||
| EXPECT_CALL(*p_iarmBusImplMock, IARM_Bus_IsConnected(::testing::_, ::testing::_)) | ||
| .WillOnce(::testing::DoAll( | ||
| ::testing::SetArgPointee<1>(0), | ||
| ::testing::Return(IARM_RESULT_IPCCORE_FAIL))); | ||
|
|
||
| createTestFirmwareFile(); | ||
| string request = "{\"firmwareFilepath\":\"" + TEST_FIRMWARE_PATH + "\",\"firmwareType\":\"PCI\"}"; | ||
| EXPECT_EQ(Core::ERROR_GENERAL, handler.Invoke(connection, _T("updateFirmware"), request, response)); | ||
| } | ||
| #endif | ||
|
|
||
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FirmwareFilepathNotExist) | ||
| { | ||
| EXPECT_EQ(Core::ERROR_INVALID_PARAMETER, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/nonexistent_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
| } | ||
| #if 0 | ||
| TEST_F(FirmwareUpdateTest, UpdateFirmware_ValidSI) | ||
| { | ||
| createTestFirmwareFile(); | ||
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"SI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
More commented-out tests: Lines 979-1067 and 1073-1081 contain additional large blocks of commented-out test code. Same concern as before - either remove them or document why they're disabled.
| WrapsImplMock *p_wrapsImplMock = nullptr; | ||
| IarmBusImplMock *p_iarmBusImplMock = nullptr; |
There was a problem hiding this comment.
[nitpick] Inconsistent whitespace: Line 79 has trailing spaces removed (changing from nullptr ; to nullptr) but line 80 still has the same pattern with spaces. Consider applying consistent formatting across both lines.
| if (flashInProgress) { | ||
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } |
There was a problem hiding this comment.
Potential race condition: The flashInProgress flag check in TearDown() is not thread-safe. If the flash thread is still running and modifying this flag, there's a race condition. Consider using proper synchronization or checking the actual thread state instead.
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
[nitpick] Security concern: Test uses chmod with hardcoded permissions 0755 on /rebootNow.sh. This creates an executable file with world-readable permissions. While this is a test environment, it's better practice to use more restrictive permissions (e.g., 0700) and ensure proper cleanup.
| chmod("/rebootNow.sh", 0755); | |
| chmod("/rebootNow.sh", 0700); |
|
|
||
| EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); |
There was a problem hiding this comment.
[nitpick] Hardcoded magic number: Multiple tests use std::chrono::milliseconds(200) as a sleep duration. This magic number should be extracted to a named constant (e.g., FLASH_OPERATION_TIMEOUT_MS) for better maintainability and to make it easier to adjust if needed.
| TEST_LOG("Pass created FirmwareUpdateImpl: %p &FirmwareUpdateImpl: %p", | ||
| static_cast<void*>(FirmwareUpdateImpl.operator->()), | ||
| static_cast<void*>(&FirmwareUpdateImpl)); |
There was a problem hiding this comment.
[nitpick] Typo in comment: "Corrected spelling of 'receive' to 'receive'" appears in the header but the actual change here is adding static_cast<void*> to properly cast the pointer for printf-style formatting. The comment about TEST_LOG improvement is accurate, but there's no spelling correction happening in this change.
| std::ofstream maintenanceFile("/opt/swupdate_maintenance_upgrade"); | ||
| maintenanceFile.close(); | ||
|
|
||
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); |
There was a problem hiding this comment.
Inconsistent test expectations: This test expects ERROR_FIRMWAREUPDATE_INPROGRESS (line 305), but the test name suggests it's testing the scenario where the reboot script doesn't exist. The expectation seems incorrect - shouldn't this fail with a different error code since the reboot script is missing?
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | |
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_REBOOTSCRIPT_MISSING, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); |
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Use of system() call is unsafe: Line 584 uses system("mkdir -p /tmp/path/to") which is vulnerable to command injection if any variables were used (though none are here). Consider using C++ filesystem library (std::filesystem::create_directories) instead for better safety and portability.
| ON_CALL(*p_iarmBusImplMock, IARM_Bus_Call) | ||
| .WillByDefault(::testing::Invoke( | ||
| [this](const char* ownerName, const char* methodName, void* arg, size_t argLen) { | ||
| flashInProgress = true; | ||
| return IARM_RESULT_SUCCESS; | ||
| })); |
There was a problem hiding this comment.
Potential resource leak: The flashInProgress flag is set to true in the mock but never automatically reset to false. If a test fails before calling cancelFirmwareUpdate, subsequent tests may be affected. Consider resetting this flag in the TearDown() method to ensure test isolation.
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | ||
| } |
There was a problem hiding this comment.
[nitpick] Hardcoded sleep duration may cause flaky tests: The 100ms sleep after plugin initialization may not be sufficient on slower systems or under heavy load. Consider using a more robust synchronization mechanism or making the timeout configurable.
| std::this_thread::sleep_for(std::chrono::milliseconds(100)); | |
| } | |
| // Wait for plugin to be ready by polling for the status file, with timeout | |
| const int max_wait_ms = 5000; | |
| const int poll_interval_ms = 50; | |
| int waited_ms = 0; | |
| while (waited_ms < max_wait_ms) { | |
| std::ifstream statusFile(FIRMWARE_UPDATE_STATE); | |
| if (statusFile.good()) { | |
| break; | |
| } | |
| std::this_thread::sleep_for(std::chrono::milliseconds(poll_interval_ms)); | |
| waited_ms += poll_interval_ms; | |
| } |
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nsleep 1\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream rebootScript("/rebootNow.sh"); | ||
| rebootScript << "#!/bin/bash\nexit 0\n"; | ||
| rebootScript.close(); | ||
| chmod("/rebootNow.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/rebootNow.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| TEST_F(FirmwareUpdateTest, GetUpdateState_InitialState) | ||
| { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::ofstream imageFlasher("/lib/rdk/imageFlasher.sh"); | ||
| imageFlasher << "#!/bin/bash\nexit 0\n"; | ||
| imageFlasher.close(); | ||
| chmod("/lib/rdk/imageFlasher.sh", 0755); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "chmod("/lib/rdk/imageFlasher.sh", 493U)" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| TEST_F(FirmwareUpdateTest, UpdateFirmware_FlashImage_FileWithPathSeparator) | ||
| { | ||
|
|
||
| system("mkdir -p /tmp/path/to"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value
Calling "system" without checking return value (as is done elsewhere 3 out of 3 times).
Medium Impact, CWE-252
CHECKED_RETURN
| EXPECT_EQ(ERROR_FIRMWAREUPDATE_INPROGRESS, handler.Invoke(connection, _T("updateFirmware"), _T("{\"firmwareFilepath\":\"/tmp/test_firmware.bin\",\"firmwareType\":\"PCI\"}"), response)); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | ||
| std::remove("/etc/device.properties"); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/etc/device.properties")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| handler.Invoke(connection, _T("cancelFirmwareUpdate"), _T("{}"), response); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| std::this_thread::sleep_for(std::chrono::milliseconds(500)); | ||
| } | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
|
|
||
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove("/tmp/FirmwareUpdateStatus.txt")" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
| } | ||
| void SetUp() override { | ||
| std::remove(FIRMWARE_UPDATE_STATE); | ||
| std::remove(TEST_FIRMWARE_PATH.c_str()); |
There was a problem hiding this comment.
Coverity Issue - Unchecked return value from library
Calling "remove(::TEST_FIRMWARE_PATH[abi:cxx11].c_str())" without checking return value. This library function may fail and return an error code.
Medium Impact, CWE-252
CHECKED_RETURN
No description provided.