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"); |
| #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.
| 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 <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. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 16 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct curl_slist* headers = nullptr; | ||
| headers = curl_slist_append(headers, "Content-Type: application/json"); | ||
| curl_easy_setopt(curl, CURLOPT_HTTPHEADER, headers); | ||
|
|
||
| std::string response; | ||
| curl_easy_setopt( | ||
| curl, CURLOPT_WRITEFUNCTION, | ||
| +[](char* ptr, size_t size, size_t nmemb, std::string* data) | ||
| { | ||
| data->append(ptr, size * nmemb); | ||
| return size * nmemb; | ||
| }); | ||
| curl_easy_setopt(curl, CURLOPT_WRITEDATA, &response); | ||
|
|
||
| CURLcode res = curl_easy_perform(curl); | ||
| curl_easy_cleanup(curl); |
There was a problem hiding this comment.
Memory allocated with curl_slist_append for the headers list is never freed. This creates a memory leak. After curl_easy_perform and before curl_easy_cleanup, you should call curl_slist_free_all(headers) to release the header list.
| struct curl_slist* headers = nullptr; | ||
| headers = curl_slist_append(headers, "Content-Type: application/json"); | ||
| curl_easy_setopt(curl, CURLOPT_HTTPHEADER, headers); | ||
|
|
||
| std::string response; | ||
| curl_easy_setopt( | ||
| curl, CURLOPT_WRITEFUNCTION, | ||
| +[](char* ptr, size_t size, size_t nmemb, std::string* data) | ||
| { | ||
| data->append(ptr, size * nmemb); | ||
| return size * nmemb; | ||
| }); | ||
| curl_easy_setopt(curl, CURLOPT_WRITEDATA, &response); | ||
|
|
||
| CURLcode res = curl_easy_perform(curl); | ||
| curl_easy_cleanup(curl); |
There was a problem hiding this comment.
Memory allocated with curl_slist_append for the headers list is never freed. This creates a memory leak. After curl_easy_perform and before curl_easy_cleanup, you should call curl_slist_free_all(headers) to release the header list.
| struct curl_slist* headers = nullptr; | ||
| headers = curl_slist_append(headers, "Content-Type: application/json"); |
There was a problem hiding this comment.
The return value of curl_slist_append is not checked for nullptr. If memory allocation fails, this would result in no headers being set, which could lead to unexpected behavior. Consider checking if curl_slist_append returns nullptr and handling the error appropriately.
| OutputStream(const std::string& filename) | ||
| : file_(std::make_unique<std::ofstream>(filename)), | ||
| out_(file_.get()) | ||
| { | ||
| } |
There was a problem hiding this comment.
The OutputStream constructor doesn't verify if the ofstream was successfully opened. If the file cannot be created or opened (e.g., due to permission issues or invalid path), out_ will point to an invalid stream and subsequent write operations will silently fail. Consider checking file_.is_open() and handling the error appropriately, or at least documenting this behavior.
| for (const auto& method : json_["methods"]) | ||
| { | ||
| if (method.contains("name") && method["name"].get<std::string>().rfind(interfaceStr, 0) == 0) | ||
| { | ||
|
|
||
| names_.push_back(method["name"]); | ||
| descriptions_.push_back(method["summary"]); |
There was a problem hiding this comment.
Accessing json_["methods"] without checking if the "methods" key exists will throw an exception if it's missing. The same applies to method["name"] and method["summary"]. Consider using the .at() method with exception handling or .value() with a default value to make the code more robust.
| for (const auto& method : json_["methods"]) | |
| { | |
| if (method.contains("name") && method["name"].get<std::string>().rfind(interfaceStr, 0) == 0) | |
| { | |
| names_.push_back(method["name"]); | |
| descriptions_.push_back(method["summary"]); | |
| // Safely access the "methods" array in the JSON. | |
| auto methodsIt = json_.find("methods"); | |
| if (methodsIt == json_.end() || !methodsIt->is_array()) | |
| { | |
| return methodNames; | |
| } | |
| for (const auto& method : *methodsIt) | |
| { | |
| // Use .value() with defaults to avoid exceptions if fields are missing or of wrong type. | |
| std::string methodName = method.value("name", ""); | |
| if (!methodName.empty() && methodName.rfind(interfaceStr, 0) == 0) | |
| { | |
| names_.push_back(methodName); | |
| descriptions_.push_back(method.value("summary", "")); |
| { | ||
| nlohmann::json eventMessage; | ||
| eventMessage["method"] = method; | ||
| eventMessage["result"] = nlohmann::json::parse(params); |
There was a problem hiding this comment.
The nlohmann::json::parse call can throw an exception if params is malformed JSON, but there's no exception handling. If the params string is invalid, the program will terminate. Consider wrapping this in a try-catch block and handling the error gracefully.
| eventMessage["result"] = nlohmann::json::parse(params); | |
| try | |
| { | |
| eventMessage["result"] = nlohmann::json::parse(params); | |
| } | |
| catch (const nlohmann::json::parse_error& e) | |
| { | |
| FAIL() << "Failed to parse JSON params for event '" << method | |
| << "': " << e.what() << std::endl; | |
| return; | |
| } |
|
|
||
| if (gAutoRun) | ||
| { | ||
| gOutput = OutputStream("firebolt_test_output.txt"); |
There was a problem hiding this comment.
The OutputStream is being re-initialized with the same filename that was already set on line 75. This creates a new file handle and may lead to resource leaks since the previous file handle isn't explicitly closed before reassignment. Consider removing this redundant initialization or restructuring the code to avoid reopening the same file.
| gOutput = OutputStream("firebolt_test_output.txt"); |
.github/workflows/ci.yml
Outdated
| bash -c " \ | ||
| set -e \ | ||
| && cd /workspace/build/test/cpp_test \ | ||
| && ./cpp_test \ |
There was a problem hiding this comment.
The cpp_test executable is being run without any arguments, but it appears to require a connection to a WebSocket server at ws://127.0.0.1:9998 (see main.cpp line 90). The test will likely fail because no mock server or actual server is set up in the CI environment. Consider either adding the -auto flag for automated testing, setting up a mock server, or handling connection failures gracefully in CI.
| && ./cpp_test \ | |
| && ./cpp_test -auto \ |
| } | ||
| } | ||
|
|
||
| std::string url = "ws://127.0.0.1:9998"; |
There was a problem hiding this comment.
The hardcoded WebSocket URL should be configurable via command-line argument or environment variable. This would make the test more flexible and allow it to run in different environments without code changes.
|
|
||
| void IFireboltDemo::loadRpc() | ||
| { | ||
| json_ = nlohmann::json::parse(JSON_DATA); |
There was a problem hiding this comment.
The JSON parsing (nlohmann::json::parse) can throw an exception if JSON_DATA is malformed, but there's no exception handling. If the JSON is invalid, the program will terminate. Consider wrapping this in a try-catch block and handling the error gracefully.
| json_ = nlohmann::json::parse(JSON_DATA); | |
| try | |
| { | |
| json_ = nlohmann::json::parse(JSON_DATA); | |
| } | |
| catch (const nlohmann::json::parse_error& e) | |
| { | |
| gOutput << "Failed to parse JSON_DATA: " << e.what() << std::endl; | |
| // json_ remains in its previous (default) state | |
| } | |
| catch (const std::exception& e) | |
| { | |
| gOutput << "Unexpected error while parsing JSON_DATA: " << e.what() << std::endl; | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| Result<void> r = Firebolt::IFireboltAccessor::Instance().LifecycleInterface().close(type); | ||
| validateResult(r); |
There was a problem hiding this comment.
The return value of validateResult is being ignored here. While other similar calls in this file (lines 59 and 102) also ignore it, the inconsistency with the rest of the codebase (where the return value is checked in if statements) is problematic. Either check the return value or document why it's safe to ignore it in these specific cases.
| interfaces = {new AccessibilityDemo(), new AdvertisingDemo(), new DeviceDemo(), new DisplayDemo(), | ||
| new LifecycleDemo(), new LocalizationDemo(), new PresentationDemo(), new StatsDemo()}; | ||
| } | ||
|
|
||
| ChooseInterface::~ChooseInterface() | ||
| { | ||
| for (auto interfacePtr : interfaces) | ||
| { | ||
| delete interfacePtr; | ||
| } | ||
| } | ||
|
|
||
| void ChooseInterface::runOption(const int index) | ||
| { | ||
| IFireboltDemo* selectedInterface = interfaces[index]; |
There was a problem hiding this comment.
There's a potential mismatch between the size of names_ vector and the interfaces vector. The names_ vector is populated dynamically from JSON (excluding "Internal"), while interfaces is a fixed-size list. If the order or count doesn't match, accessing interfaces[index] with an index from names_ could cause out-of-bounds access or incorrect interface selection.
| std::vector<std::string> methodNames; | ||
|
|
||
| std::string interfaceStr = interfaceName + "."; | ||
| for (const auto& method : json_["methods"]) |
There was a problem hiding this comment.
Direct access to json_["methods"] without checking if the key exists can cause an exception or unexpected behavior if the JSON structure is different than expected. Consider using json_.contains("methods") before iterating, or use json_.value("methods", nlohmann::json::array()) to provide a default empty array.
| for (const auto& method : json_["methods"]) | |
| auto methods = json_.value("methods", nlohmann::json::array()); | |
| for (const auto& method : methods) |
|
|
||
| #include "cpp/outputstream.h" | ||
|
|
||
| OutputStream gOutput; |
There was a problem hiding this comment.
Global variable gOutput is declared without proper encapsulation. Consider using a singleton pattern or passing this as a parameter through the call stack to avoid global state, which makes testing and maintenance more difficult.
| CURLcode res = curl_easy_perform(curl); | ||
| curl_easy_cleanup(curl); | ||
|
|
||
| return (res == CURLE_OK) ? response : ""; |
There was a problem hiding this comment.
The curl_slist created with curl_slist_append is not freed, causing a memory leak. After curl_easy_perform, you should call curl_slist_free_all(headers) before curl_easy_cleanup.
| OutputStream(const std::string& filename) | ||
| : file_(std::make_unique<std::ofstream>(filename)), | ||
| out_(file_.get()) | ||
| { | ||
| } |
There was a problem hiding this comment.
The OutputStream constructor that takes a filename doesn't check if the file was successfully opened. If file creation fails, out_ will point to an invalid stream, causing undefined behavior when used. Add a check after file_ construction and handle the error appropriately.
.github/workflows/ci.yml
Outdated
| path: ${{ github.workspace }}/build/test/cpp_test/firebolt_test_output.txt | ||
There was a problem hiding this comment.
There is a trailing tab character at the end of this line. This is inconsistent with the rest of the file which uses spaces. Remove the tab character for consistency.
| path: ${{ github.workspace }}/build/test/cpp_test/firebolt_test_output.txt | |
| path: ${{ github.workspace }}/build/test/cpp_test/firebolt_test_output.txt |
|
|
||
| if (ENABLE_DEMO_APP) | ||
| add_subdirectory(demo) | ||
| add_subdirectory(test/cpp_test) #for now |
There was a problem hiding this comment.
The comment "#for now" suggests this is a temporary solution. If this test suite should be part of the demo app temporarily, this should be documented with a TODO or issue reference explaining when and how it will be moved to the proper location. If it's permanent, remove the comment.
| add_subdirectory(test/cpp_test) #for now | |
| add_subdirectory(test/cpp_test) # TODO: Move cpp_test into the main test suite directory once the demo app test layout is finalized. |
| bool gConnected = false; | ||
| bool gAutoRun = false; | ||
|
|
||
| void connectionChanged(const bool connected, const Firebolt::Error error) | ||
| { | ||
| std::cout << "Change in connection: connected: " << connected << " error: " << static_cast<int>(error) << std::endl; | ||
| gConnected = connected; |
There was a problem hiding this comment.
The global variable gConnected is accessed from both the connection callback (line 37) and the waitOnConnectionReady function (line 62) without synchronization. This creates a race condition if the callback is invoked from a different thread. Consider using std::atomic or protecting access with a mutex.
| std::vector<std::string> IFireboltDemo::methodsFromRpc(const std::string& interfaceName) | ||
| { | ||
| std::vector<std::string> methodNames; | ||
|
|
||
| std::string interfaceStr = interfaceName + "."; | ||
| for (const auto& method : json_["methods"]) | ||
| { | ||
| if (method.contains("name") && method["name"].get<std::string>().rfind(interfaceStr, 0) == 0) | ||
| { | ||
|
|
||
| names_.push_back(method["name"]); | ||
| descriptions_.push_back(method["summary"]); | ||
| gOutput << "Found method: " << names_.back() << ":" << descriptions_.back() << std::endl; | ||
| } | ||
| } | ||
|
|
||
| return methodNames; |
There was a problem hiding this comment.
The function methodsFromRpc returns a methodNames vector that is always empty. The function populates names_ and descriptions_ member variables but never adds anything to methodNames. Either the return value should be removed (changing return type to void), or the function should return names_ instead.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (*out_) << value; | ||
| return *this; | ||
| } | ||
|
|
||
| // Support for std::endl and other manipulators | ||
| OutputStream& operator<<(std::ostream& (*manip)(std::ostream&)) | ||
| { | ||
| (*out_) << manip; |
There was a problem hiding this comment.
Potential null pointer dereference: The code checks if out_ is valid before dereferencing with (*out_) << value, but out_ could be null if the file failed to open in the constructor on line 36-38. The file_ unique_ptr could be valid but the ofstream could be in a failed state, making file_.get() return a non-null pointer to an unopened stream. Consider checking the stream state (e.g., out_->good()) before writing.
| (*out_) << value; | |
| return *this; | |
| } | |
| // Support for std::endl and other manipulators | |
| OutputStream& operator<<(std::ostream& (*manip)(std::ostream&)) | |
| { | |
| (*out_) << manip; | |
| if (out_ && out_->good()) { | |
| (*out_) << value; | |
| } | |
| return *this; | |
| } | |
| // Support for std::endl and other manipulators | |
| OutputStream& operator<<(std::ostream& (*manip)(std::ostream&)) | |
| { | |
| if (out_ && out_->good()) { | |
| (*out_) << manip; | |
| } |
| void runOption(const int index); | ||
|
|
||
| private: | ||
| // void triggerPresentationStateChange(); |
There was a problem hiding this comment.
Commented-out private member should be removed or uncommented. If the triggerPresentationStateChange method is not yet implemented, either implement it or remove the commented declaration to keep the code clean.
| // void triggerPresentationStateChange(); |
| LifecycleDemo::LifecycleDemo() | ||
| : IFireboltDemo() | ||
| { | ||
| methodsFromRpc("Lifecycle2"); |
There was a problem hiding this comment.
The methodsFromRpc call uses "Lifecycle2" as the interface name, which is inconsistent with other demo files that don't have version numbers in their interface names (e.g., "Device", "Display", "Accessibility"). If "Lifecycle2" is the correct interface name in the OpenRPC spec, this is fine. Otherwise, ensure consistency with interface naming conventions. You may want to verify this is intentional.
| - name: Get version | ||
| run: | | ||
| VERSION=$(git describe --tags --dirty --match "v[0-9]*") | ||
| echo "VERSION=$VERSION" >> $GITHUB_ENV | ||
| echo "version: $VERSION" |
There was a problem hiding this comment.
Unused variable: The VERSION environment variable is set but never used in subsequent steps. Either use this variable for versioning purposes (e.g., in artifact names or build metadata) or remove these lines to keep the workflow clean.
| add_subdirectory(test/cpp_test) #for now | ||
| endif() | ||
|
|
||
| if (ENABLE_TESTS) | ||
| add_subdirectory(test) |
There was a problem hiding this comment.
The cpp_test subdirectory is unconditionally added when ENABLE_DEMO_APP is ON. The inline comment "for now" suggests this is temporary, but cpp_test appears to be a test directory, not a demo application. Consider either: 1) Moving this under the ENABLE_TESTS condition (lines 97-99), or 2) Creating a separate ENABLE_CPP_TEST option. Mixing test code with demo code in this way violates separation of concerns.
| 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 "outputstream.h" | ||
| extern OutputStream gOutput; | ||
|
|
||
| // #include "json_types/jsondata_device_types.h" |
There was a problem hiding this comment.
Commented-out code should be removed. If the include of jsondata_device_types.h is not needed, remove the comment. If it might be needed in the future, add a TODO comment explaining why.
| // #include "json_types/jsondata_device_types.h" |
| - name: Get version | ||
| run: | | ||
| VERSION=$(git describe --tags --dirty --match "v[0-9]*") | ||
| echo "VERSION=$VERSION" >> $GITHUB_ENV | ||
| echo "version: $VERSION" |
There was a problem hiding this comment.
Unused variable: The VERSION environment variable is set but never used in subsequent steps. Either use this variable for versioning purposes (e.g., in artifact names or build metadata) or remove these lines to keep the workflow clean.
|
|
||
| std::this_thread::sleep_for(std::chrono::seconds(1)); | ||
|
|
||
| // ::testing::InitGoogleTest(&argc, argv); |
There was a problem hiding this comment.
Commented-out code should be removed rather than left in the codebase. If GoogleTest integration is planned for the future, document this as a TODO comment with context. Otherwise, remove this line to keep the code clean.
| // ::testing::InitGoogleTest(&argc, argv); | |
| // TODO: Initialize GoogleTest here when integrating unit tests with this binary. |
|
|
||
| #define FAIL() std::cout | ||
|
|
There was a problem hiding this comment.
The FAIL() macro is misleading. It's defined as std::cout but the name suggests it would cause a test failure (like in GoogleTest). This macro only prints to console without actually failing. Consider either: 1) Renaming to something like ERROR_LOG() to accurately reflect its behavior, or 2) Implementing actual failure logic if this is meant to be a test framework. The current implementation could confuse developers expecting test failure behavior.
| #define FAIL() std::cout | |
| #include <cstdlib> | |
| namespace | |
| { | |
| class FailLogger | |
| { | |
| public: | |
| FailLogger() = default; | |
| ~FailLogger() | |
| { | |
| // Ensure that any use of FAIL() results in a hard failure. | |
| std::abort(); | |
| } | |
| template <typename T> | |
| FailLogger& operator<<(const T& value) | |
| { | |
| std::cerr << value; | |
| return *this; | |
| } | |
| using Manipulator = std::ostream& (*)(std::ostream&); | |
| FailLogger& operator<<(Manipulator manip) | |
| { | |
| manip(std::cerr); | |
| return *this; | |
| } | |
| }; | |
| inline FailLogger makeFailLogger() | |
| { | |
| return FailLogger{}; | |
| } | |
| } // namespace | |
| #define FAIL() makeFailLogger() |
| } | ||
|
|
||
| void AccessibilityDemo::runOption(const int index) | ||
| { |
There was a problem hiding this comment.
Missing bounds checking: The code accesses names_[index] without verifying that index is within valid bounds. If index is out of range, this will cause undefined behavior. While the caller is expected to provide valid indices, defensive programming would add a bounds check or at least an assertion to catch programming errors early.
| { | |
| { | |
| if (index < 0 || static_cast<size_t>(index) >= names_.size()) | |
| { | |
| outStream_ << "Invalid Accessibility option index: " << index << std::endl; | |
| return; | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::string httpPost(const std::string& url, const std::string& postData) | ||
| { | ||
| CURL* curl = curl_easy_init(); | ||
| if (!curl) | ||
| return ""; | ||
| curl_easy_setopt(curl, CURLOPT_URL, url.c_str()); | ||
| curl_easy_setopt(curl, CURLOPT_POST, 1L); | ||
| curl_easy_setopt(curl, CURLOPT_POSTFIELDS, postData.c_str()); | ||
| curl_easy_setopt(curl, CURLOPT_POSTFIELDSIZE, postData.size()); | ||
|
|
||
| struct curl_slist* headers = nullptr; | ||
| headers = curl_slist_append(headers, "Content-Type: application/json"); | ||
| curl_easy_setopt(curl, CURLOPT_HTTPHEADER, headers); | ||
|
|
||
| std::string response; | ||
| curl_easy_setopt( | ||
| curl, CURLOPT_WRITEFUNCTION, | ||
| +[](char* ptr, size_t size, size_t nmemb, std::string* data) | ||
| { | ||
| data->append(ptr, size * nmemb); | ||
| return size * nmemb; | ||
| }); | ||
| curl_easy_setopt(curl, CURLOPT_WRITEDATA, &response); | ||
|
|
||
| CURLcode res = curl_easy_perform(curl); | ||
| curl_easy_cleanup(curl); | ||
|
|
||
| return (res == CURLE_OK) ? response : ""; | ||
| } |
There was a problem hiding this comment.
The httpPost function leaks memory by not freeing the curl_slist headers. After curl_easy_perform, you should call curl_slist_free_all(headers) before curl_easy_cleanup to avoid memory leaks.
| gOutput = OutputStream("firebolt_test_output.txt"); | ||
|
|
||
| gOutput << "Logging to file instead of console" << std::endl; | ||
|
|
||
| // check args for -auto option | ||
| for (int i = 1; i < argc; ++i) | ||
| { | ||
| std::cout << "Arg: " << argv[i] << std::endl; | ||
| if (std::string(argv[i]) == "-auto") | ||
| { | ||
| gAutoRun = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| std::string url = "ws://127.0.0.1:9998"; | ||
| createFireboltInstance(url); | ||
|
|
||
| std::this_thread::sleep_for(std::chrono::seconds(1)); | ||
|
|
||
| // ::testing::InitGoogleTest(&argc, argv); | ||
| if (!waitOnConnectionReady()) | ||
| { | ||
| std::cout << "Test not able to connect with server..." << std::endl; | ||
| return -1; | ||
| } | ||
|
|
||
| if (gAutoRun) | ||
| { | ||
| gOutput = OutputStream("firebolt_test_output.txt"); | ||
| ChooseInterface chooseInterface; | ||
| chooseInterface.autoRun(); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
The OutputStream is initialized twice in the main function when gAutoRun is true: once at line 75 and again at line 104. The second initialization at line 104 will replace the first one, making the initial assignment at line 75 redundant when running in auto mode. Consider restructuring this logic to avoid the unnecessary initialization.
| interfaces = {new AccessibilityDemo(), new AdvertisingDemo(), new DeviceDemo(), new DisplayDemo(), | ||
| new LifecycleDemo(), new LocalizationDemo(), new PresentationDemo(), new StatsDemo()}; | ||
| } | ||
|
|
||
| ChooseInterface::~ChooseInterface() | ||
| { | ||
| for (auto interfacePtr : interfaces) | ||
| { | ||
| delete interfacePtr; | ||
| } | ||
| } |
There was a problem hiding this comment.
The interfaces vector stores raw pointers allocated with new, which are manually deleted in the destructor. This is error-prone and violates modern C++ best practices. Consider using std::unique_ptr or std::shared_ptr to manage these objects automatically. This would eliminate the need for manual deletion and prevent potential memory leaks if exceptions occur during construction.
| bool waitOnConnectionReady() | ||
| { | ||
| uint32_t waiting = 10000; | ||
| static constexpr uint32_t SLEEPSLOT_TIME = 100; |
There was a problem hiding this comment.
The static constexpr uint32_t SLEEPSLOT_TIME is declared inside the function. While this works in C++11 and later, it's more conventional to declare such constants at namespace scope or as a class member for better visibility and potential reuse. Consider moving it outside the function scope.
| #pragma once | ||
| #include <string_view> | ||
|
|
||
| constexpr std::string_view JSON_DATA = R"(@JSON_CONTENT@)"; No newline at end of file |
There was a problem hiding this comment.
The JSON_DATA string_view is defined in the generated header file as a raw string literal. This approach embeds the entire JSON file content into the binary. For large JSON files, this could significantly increase binary size. Consider whether loading the JSON file at runtime might be more appropriate, or document that this is intentional for distribution convenience.
| std::vector<std::string> IFireboltDemo::methodsFromRpc(const std::string& interfaceName) | ||
| { | ||
| std::vector<std::string> methodNames; | ||
|
|
||
| std::string interfaceStr = interfaceName + "."; | ||
| for (const auto& method : json_["methods"]) | ||
| { | ||
| if (method.contains("name") && method["name"].get<std::string>().rfind(interfaceStr, 0) == 0) | ||
| { | ||
|
|
||
| names_.push_back(method["name"]); | ||
| descriptions_.push_back(method["summary"]); | ||
| gOutput << "Found method: " << names_.back() << ":" << descriptions_.back() << std::endl; | ||
| } | ||
| } | ||
|
|
||
| return methodNames; | ||
| } |
There was a problem hiding this comment.
The methodsFromRpc function declares a local variable methodNames that is never used and returns an empty vector. Either this variable should be populated and returned, or the function should have a void return type. The current implementation suggests incomplete functionality.
| void verifyEventReceived(std::mutex& mtx, std::condition_variable& cv, bool& eventReceived) | ||
| { | ||
|
|
||
| // Wait for the event to be received or timeout after 5 seconds | ||
| std::unique_lock<std::mutex> lock(mtx); | ||
| if (!cv.wait_for(lock, std::chrono::seconds(5), [&] { return eventReceived; })) | ||
| { | ||
| FAIL() << "Did not receive event within timeout"; | ||
| } | ||
| } |
There was a problem hiding this comment.
The verifyEventReceived function has a hardcoded timeout of 5 seconds, while the similar function in test/utils.cpp uses a configurable EventWaitTime constant set to 2 seconds. This inconsistency could lead to different timeout behaviors. Consider using a consistent timeout value or making it configurable.
| void IFireboltDemo::paramFromConsole(const std::string& name, const std::string& def, std::string& value) | ||
| { | ||
| gOutput << "Enter " << name << " (default: " << def << "): "; | ||
| std::string input; | ||
| std::getline(std::cin, input); | ||
| if (input.empty()) | ||
| { | ||
| value = def; | ||
| } | ||
| else | ||
| { | ||
| value = input; | ||
| } | ||
| } |
There was a problem hiding this comment.
The paramFromConsole function uses std::getline to read user input, but there's no validation or sanitization of the input value. While this may be acceptable for a demo application, consider documenting any expected input constraints or adding basic validation if certain parameters have format requirements.
| std::string httpGet(const std::string& url) | ||
| { | ||
| CURL* curl = curl_easy_init(); | ||
| if (!curl) | ||
| return ""; | ||
| curl_easy_setopt(curl, CURLOPT_URL, url.c_str()); | ||
|
|
||
| struct curl_slist* headers = nullptr; | ||
| headers = curl_slist_append(headers, "Content-Type: application/json"); | ||
| curl_easy_setopt(curl, CURLOPT_HTTPHEADER, headers); | ||
|
|
||
| std::string response; | ||
| curl_easy_setopt( | ||
| curl, CURLOPT_WRITEFUNCTION, | ||
| +[](char* ptr, size_t size, size_t nmemb, std::string* data) | ||
| { | ||
| data->append(ptr, size * nmemb); | ||
| return size * nmemb; | ||
| }); | ||
| curl_easy_setopt(curl, CURLOPT_WRITEDATA, &response); | ||
|
|
||
| CURLcode res = curl_easy_perform(curl); | ||
| curl_easy_cleanup(curl); | ||
|
|
||
| return (res == CURLE_OK) ? response : ""; | ||
| } |
There was a problem hiding this comment.
The httpGet and httpPost functions leak memory by not freeing the curl_slist headers. After curl_easy_perform, you should call curl_slist_free_all(headers) before curl_easy_cleanup to avoid memory leaks.
| std::string httpGet(const std::string& url) | ||
| { | ||
| CURL* curl = curl_easy_init(); | ||
| if (!curl) | ||
| return ""; | ||
| curl_easy_setopt(curl, CURLOPT_URL, url.c_str()); | ||
|
|
||
| struct curl_slist* headers = nullptr; | ||
| headers = curl_slist_append(headers, "Content-Type: application/json"); | ||
| curl_easy_setopt(curl, CURLOPT_HTTPHEADER, headers); | ||
|
|
||
| std::string response; | ||
| curl_easy_setopt( | ||
| curl, CURLOPT_WRITEFUNCTION, | ||
| +[](char* ptr, size_t size, size_t nmemb, std::string* data) | ||
| { | ||
| data->append(ptr, size * nmemb); | ||
| return size * nmemb; | ||
| }); | ||
| curl_easy_setopt(curl, CURLOPT_WRITEDATA, &response); | ||
|
|
||
| CURLcode res = curl_easy_perform(curl); | ||
| curl_easy_cleanup(curl); | ||
|
|
||
| return (res == CURLE_OK) ? response : ""; | ||
| } | ||
|
|
||
| // curl http post helper function using | ||
| std::string httpPost(const std::string& url, const std::string& postData) | ||
| { | ||
| CURL* curl = curl_easy_init(); | ||
| if (!curl) | ||
| return ""; | ||
| curl_easy_setopt(curl, CURLOPT_URL, url.c_str()); | ||
| curl_easy_setopt(curl, CURLOPT_POST, 1L); | ||
| curl_easy_setopt(curl, CURLOPT_POSTFIELDS, postData.c_str()); | ||
| curl_easy_setopt(curl, CURLOPT_POSTFIELDSIZE, postData.size()); | ||
|
|
||
| struct curl_slist* headers = nullptr; | ||
| headers = curl_slist_append(headers, "Content-Type: application/json"); | ||
| curl_easy_setopt(curl, CURLOPT_HTTPHEADER, headers); | ||
|
|
||
| std::string response; | ||
| curl_easy_setopt( | ||
| curl, CURLOPT_WRITEFUNCTION, | ||
| +[](char* ptr, size_t size, size_t nmemb, std::string* data) | ||
| { | ||
| data->append(ptr, size * nmemb); | ||
| return size * nmemb; | ||
| }); | ||
| curl_easy_setopt(curl, CURLOPT_WRITEDATA, &response); | ||
|
|
||
| CURLcode res = curl_easy_perform(curl); | ||
| curl_easy_cleanup(curl); | ||
|
|
||
| return (res == CURLE_OK) ? response : ""; | ||
| } | ||
|
|
||
| void triggerRaw(const std::string& payload) | ||
| { | ||
| httpPost("http://localhost:3333/api/v1/raw", payload); | ||
| } | ||
|
|
||
| void triggerEvent(const std::string& method, const std::string& params) | ||
| { | ||
| nlohmann::json eventMessage; | ||
| eventMessage["method"] = method; | ||
| eventMessage["result"] = nlohmann::json::parse(params); | ||
|
|
||
| httpPost("http://localhost:3333/api/v1/event", eventMessage.dump()); | ||
| } | ||
|
|
||
| void verifyEventSubscription(const Firebolt::Result<Firebolt::SubscriptionId>& id) | ||
| { | ||
| if (id) | ||
| { | ||
| std::cout << "Event Subscription is successful." << std::endl; | ||
| } | ||
| else | ||
| { | ||
| FAIL() << "Event Subscription failed. " + toError(id); | ||
| } | ||
| } | ||
| void verifyUnsubscribeResult(const Firebolt::Result<void>& result) | ||
| { | ||
|
|
||
| if (result.error() == Firebolt::Error::None) | ||
| { | ||
| std::cout << "Unsubscribe success" << std::endl; | ||
| } | ||
| else | ||
| { | ||
| FAIL() << "Unsubscribe failed." + toError(result); | ||
| } | ||
| } | ||
| void verifyEventReceived(std::mutex& mtx, std::condition_variable& cv, bool& eventReceived) | ||
| { | ||
|
|
||
| // Wait for the event to be received or timeout after 5 seconds | ||
| std::unique_lock<std::mutex> lock(mtx); | ||
| if (!cv.wait_for(lock, std::chrono::seconds(5), [&] { return eventReceived; })) | ||
| { | ||
| FAIL() << "Did not receive event within timeout"; | ||
| } | ||
| } |
There was a problem hiding this comment.
This code duplicates the httpGet, httpPost, triggerEvent, triggerRaw, verifyEventSubscription, verifyUnsubscribeResult, and verifyEventReceived functions that already exist in test/utils.cpp. Consider extracting these common utilities into a shared header/source file that both the gtest-based tests and this demo application can use, to avoid code duplication and maintenance burden.
No description provided.