cpp_test first commit #25
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new C++ test/demo application (cpp_test) for the Firebolt SDK. The application provides an interactive demonstration framework for testing various Firebolt interfaces including Accessibility, Advertising, Device, Display, Lifecycle, Localization, Presentation, and Stats modules.
Changes:
- Added a new test application structure with main entry point and demo framework
- Implemented demo classes for eight Firebolt interface modules
- Added utility classes for HTTP communication, event triggering, and output stream handling
- Integrated build configuration with CMake
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| test/cpp_test/main.cpp | Main entry point with connection management, command-line argument parsing, and interface selection loop |
| test/cpp_test/cpp/utils.h | Utility function declarations for HTTP operations and event verification |
| test/cpp_test/cpp/utils.cpp | Implementation of HTTP helpers using libcurl and event triggering functions |
| test/cpp_test/cpp/outputstream.h | Output stream wrapper class for redirecting output to file or console |
| test/cpp_test/cpp/fireboltdemo.h | Base class interface for Firebolt demo implementations |
| test/cpp_test/cpp/fireboltdemo.cpp | Base class implementation with RPC method discovery and user interaction |
| test/cpp_test/cpp/chooseInterface.h | Interface selection and coordination class header |
| test/cpp_test/cpp/chooseInterface.cpp | Implementation of interface selection with auto-run capability |
| test/cpp_test/cpp/accessibilityDemo.{h,cpp} | Demo implementation for Accessibility module testing |
| test/cpp_test/cpp/advertisingDemo.{h,cpp} | Demo implementation for Advertising module testing |
| test/cpp_test/cpp/deviceDemo.{h,cpp} | Demo implementation for Device module testing |
| test/cpp_test/cpp/displayDemo.{h,cpp} | Demo implementation for Display module testing (missing copyright headers) |
| test/cpp_test/cpp/lifecycleDemo.{h,cpp} | Demo implementation for Lifecycle module with state management |
| test/cpp_test/cpp/localizationDemo.{h,cpp} | Demo implementation for Localization module testing |
| test/cpp_test/cpp/presentationDemo.{h,cpp} | Demo implementation for Presentation module with event triggering |
| test/cpp_test/cpp/statsDemo.{h,cpp} | Demo implementation for Stats module testing |
| test/cpp_test/cpp/firebolt-open-rpc.json.in | Template for embedding OpenRPC specification |
| test/cpp_test/CMakeLists.txt | Build configuration for cpp_test executable with dependencies |
| test/cpp_test/.vscode/launch.json | VSCode debug configuration with hard-coded paths |
| CMakeLists.txt | Added cpp_test subdirectory to demo app build |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (gAutoRun) | ||
| { | ||
| gOutput = OutputStream("firebolt_test_output.txt"); |
There was a problem hiding this comment.
The OutputStream is being recreated with the same filename. This line appears redundant since gOutput was already initialized with this filename at line 57. If the intention is to reset or flush the stream, this should be clarified.
| gOutput = OutputStream("firebolt_test_output.txt"); |
| void PresentationDemo::triggerPresentationStateChange() | ||
| { | ||
| triggerEvent("Presentation.onFocusedChanged", R"({"focused": true})"); | ||
| } No newline at end of file |
There was a problem hiding this comment.
The private method triggerPresentationStateChange() is declared but never called anywhere in the code. Consider removing this unused method or implementing its usage if it was intended to be part of the demo functionality.
| class OutputStream | ||
| { | ||
| std::unique_ptr<std::ofstream> file_; | ||
| std::ostream* out_; | ||
|
|
||
| public: | ||
| OutputStream() | ||
| : out_(&std::cout) | ||
| { | ||
| } | ||
| OutputStream(const std::string& filename) | ||
| : file_(std::make_unique<std::ofstream>(filename)), | ||
| out_(file_.get()) | ||
| { | ||
| } | ||
|
|
||
| template <typename T> OutputStream& operator<<(const T& value) | ||
| { | ||
| (*out_) << value; | ||
| return *this; | ||
| } | ||
|
|
||
| // Support for std::endl and other manipulators | ||
| OutputStream& operator<<(std::ostream& (*manip)(std::ostream&)) | ||
| { | ||
| (*out_) << manip; | ||
| return *this; | ||
| } | ||
| }; No newline at end of file |
There was a problem hiding this comment.
The OutputStream class contains a std::unique_ptr but does not explicitly delete the copy constructor and copy assignment operator. This can lead to issues since the implicitly-generated copy operations would not work correctly with unique_ptr. Either explicitly delete copy operations (= delete) or implement proper copy/move semantics.
| #include <string> | ||
| #include <vector> | ||
| #include "fireboltdemo.h" | ||
|
|
||
| class DisplayDemo : public IFireboltDemo | ||
| { | ||
| public: | ||
| DisplayDemo(); | ||
| ~DisplayDemo() = default; | ||
| void runOption(const int index); | ||
| }; |
There was a problem hiding this comment.
This header file is missing the copyright and license header that is present in all other demo header files in this directory. All other demo headers (accessibilityDemo.h, advertisingDemo.h, deviceDemo.h, lifecycleDemo.h, localizationDemo.h, presentationDemo.h, statsDemo.h) include the Comcast copyright notice and Apache 2.0 license header.
|
|
||
| uint32_t sleepSlot = (waiting > SLEEPSLOT_TIME ? SLEEPSLOT_TIME : waiting); | ||
| // Right, lets sleep in slices of 100 ms | ||
| usleep(sleepSlot); |
There was a problem hiding this comment.
The usleep() function is deprecated in POSIX and may not be available on all platforms. Consider using std::this_thread::sleep_for from instead, which is already included and used elsewhere in this file (line 75). For example: std::this_thread::sleep_for(std::chrono::microseconds(sleepSlot));
| usleep(sleepSlot); | |
| std::this_thread::sleep_for(std::chrono::microseconds(sleepSlot)); |
| { | ||
| "version": "0.2.0", | ||
| "configurations": [ | ||
| { | ||
| "name": "Debug cpp_test", | ||
| "type": "cppdbg", | ||
| "request": "launch", | ||
| "program": "${workspaceFolder}/build/cpp_test", | ||
| "args": [], | ||
| "stopAtEntry": false, | ||
| "cwd": "${workspaceFolder}", | ||
| "environment": [], | ||
| "externalConsole": false, | ||
| "MIMode": "gdb", | ||
| "setupCommands": [ | ||
| { | ||
| "description": "Enable pretty-printing for gdb", | ||
| "text": "-enable-pretty-printing", | ||
| "ignoreFailures": true | ||
| }, | ||
| { | ||
| "description": "Add FireboltTransport source", | ||
| "text": "directory /home/damien/FIREBOLT_RDKCENTRAL2/firebolt-native-transport/src", | ||
| "ignoreFailures": false | ||
| } | ||
| ], | ||
| "environment": [ | ||
| { | ||
| "name": "LD_LIBRARY_PATH", | ||
| "value": "/home/damien/FIREBOLT_RDKCENTRAL2/sysroot/usr/lib:${env:LD_LIBRARY_PATH}" | ||
| } | ||
| ], | ||
| "miDebuggerPath": "/usr/bin/gdb", | ||
|
|
||
| } | ||
| ] | ||
| } No newline at end of file |
There was a problem hiding this comment.
This launch configuration file contains a hard-coded absolute path specific to a developer's local machine (/home/damien/FIREBOLT_RDKCENTRAL2/...). This file should either be excluded from version control (added to .gitignore) or use relative paths/workspace variables to work across different developer environments.
| interfaces = {new AccessibilityDemo(), new AdvertisingDemo(), new DeviceDemo(), new DisplayDemo(), | ||
| new LifecycleDemo(), new LocalizationDemo(), new PresentationDemo(), new StatsDemo()}; |
There was a problem hiding this comment.
Raw pointers are allocated with new but could leak if an exception is thrown during construction of subsequent objects in the initializer list. Consider using std::unique_ptr or std::shared_ptr for automatic memory management. The destructor correctly deletes them, but exception-safe initialization would be more robust.
| add_subdirectory(test/cpp_test) #for now | ||
| endif() | ||
|
|
||
| if (ENABLE_TESTS) | ||
| add_subdirectory(test) |
There was a problem hiding this comment.
The comment "for now" suggests this is a temporary change. If this test directory should be built as part of the demo app conditionally, the comment should clarify the future plan, or this should be added under ENABLE_TESTS instead of ENABLE_DEMO_APP.
| add_subdirectory(test/cpp_test) #for now | |
| endif() | |
| if (ENABLE_TESTS) | |
| add_subdirectory(test) | |
| endif() | |
| if (ENABLE_TESTS) | |
| add_subdirectory(test) | |
| add_subdirectory(test/cpp_test) |
| #include "displayDemo.h" | ||
| #include <iostream> | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #include "json_types/jsondata_device_types.h" | ||
|
|
||
| #include "outputstream.h" | ||
| extern OutputStream gOutput; | ||
|
|
||
| using namespace Firebolt; | ||
| using namespace Firebolt::Display; | ||
|
|
||
| DisplayDemo::DisplayDemo() | ||
| : IFireboltDemo() | ||
| { | ||
| methodsFromRpc("Display"); | ||
| } |
There was a problem hiding this comment.
This implementation file is missing the copyright and license header that is present in all other demo implementation files in this directory. All other demo implementations include the Comcast copyright notice and Apache 2.0 license header.
| #include <chrono> | ||
| #include <iostream> | ||
| #include <string> | ||
| #include <thread> | ||
| #include <unistd.h> | ||
|
|
||
| #include "cpp/chooseInterface.h" | ||
|
|
||
| #include "cpp/outputstream.h" | ||
|
|
||
| OutputStream gOutput; |
There was a problem hiding this comment.
This main.cpp file is missing the copyright and license header that is present in other test main files in the codebase (such as test/ComponentTestsMain.cpp and demo/main.cpp). Consistent copyright headers should be applied across the codebase.
|
Hi @damcav64 : I have cleared off the code match (i.e. will pss on next commit) but there are a few files that a need a header. |
No description provided.