[Tests] Add thunder_test_support static library for plugin integration testing#2084
[Tests] Add thunder_test_support static library for plugin integration testing#2084smanes0213 wants to merge 13 commits intomasterfrom
Conversation
|
There was a problem hiding this comment.
Pull request overview
Adds a new thunder_test_support static library intended to enable in-process integration testing of Thunder plugins by embedding PluginHost::Server into test binaries (optionally built via ENABLE_TEST_RUNTIME).
Changes:
- Introduces
Thunder::TestCore::ThunderTestRuntimeAPI to bootstrap an embedded Thunder server, invoke JSON-RPC in-process, and query COM-RPC interfaces. - Adds a new
Tests/test_support/CMake target (thunder_test_support) that statically compiles key Thunder server sources (excludingPluginHost.cpp/main()). - Adds end-user documentation describing architecture, build flags, and usage patterns for integration tests.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/test_support/ThunderTestRuntime.h | Public API for starting/stopping an embedded server and calling JSON-RPC / COM-RPC from tests |
| Tests/test_support/ThunderTestRuntime.cpp | Implements temp-dir lifecycle, config generation/parsing, server bootstrap, and invocation helpers |
| Tests/test_support/Module.cpp | Declares module name for tracing/logging for the static library |
| Tests/test_support/CMakeLists.txt | Builds/installs the new thunder_test_support static library and links required Thunder components |
| Tests/CMakeLists.txt | Adds ENABLE_TEST_RUNTIME option and conditionally includes test_support |
| docs/ThunderTestSupport/ThunderTestSupport.md | Adds detailed documentation for the test support library |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| void ThunderTestRuntime::CleanupDirectories() const | ||
| { | ||
| if (!_tempDir.empty()) { | ||
| Core::Directory(_tempDir.c_str()).Destroy(); |
| std::ostringstream json; | ||
| json << "{" | ||
| << "\"port\":0," | ||
| << "\"binding\":\"127.0.0.1\"," | ||
| << "\"idletime\":180," | ||
| << "\"persistentpath\":\"" << _tempDir << "persistent/\"," | ||
| << "\"volatilepath\":\"" << _tempDir << "volatile/\"," | ||
| << "\"datapath\":\"" << _tempDir << "data/\"," | ||
| << "\"systempath\":\"" << systemPath << "\"," | ||
| << "\"proxystubpath\":\"" << proxyStubPath << "\"," | ||
| << "\"communicator\":\"" << _tempDir << "communicator\"," | ||
| << "\"plugins\":["; | ||
|
|
||
| for (size_t i = 0; i < plugins.size(); ++i) { | ||
| const auto& p = plugins[i]; | ||
| if (i > 0) json << ","; | ||
| json << "{" | ||
| << "\"callsign\":\"" << p.callsign << "\"," | ||
| << "\"locator\":\"" << p.locator << "\"," | ||
| << "\"classname\":\"" << p.classname << "\"," | ||
| << "\"startuporder\":" << p.startuporder << "," | ||
| << "\"autostart\":" << (p.autostart ? "true" : "false"); | ||
|
|
||
| if (!p.configuration.empty()) { | ||
| json << ",\"configuration\":" << p.configuration; |
| Core::Directory(_tempDir.c_str()).Create(); | ||
| Core::Directory((_tempDir + "persistent/").c_str()).Create(); | ||
| Core::Directory((_tempDir + "volatile/").c_str()).Create(); | ||
| Core::Directory((_tempDir + "data/").c_str()).Create(); |
| #ifndef MODULE_NAME | ||
| #define MODULE_NAME Application | ||
| #endif | ||
|
|
| // | ||
| // TEST_F(MyTest, JsonRpc) { | ||
| // string resp; | ||
| // _runtime.InvokeJSONRPC("Callsign", "Method", params, resp); |
| │ │ ├── PluginServer │ │ │ │ │ ├── Controller │ │ | ||
| │ │ ├── SystemInfo │ │ │ │ │ ├── PluginServer │ │ | ||
| │ │ └── HTTP/WS Listener │ │ │ │ │ ├── SystemInfo │ │ | ||
| │ │ ↓ │ │ │ │ │ └── (no HTTP listener) │ │ | ||
| │ │ Plugin .so (dynamic load) │ │ │ │ └── Plugin .so (dynamic load) │ │ | ||
| │ └───────────────────────────────┘ │ │ └─────────────────────────────────┘ │ | ||
| │ │ │ │ | ||
| │ Communication: HTTP/WS/COMRPC │ │ Communication: Direct in-process calls │ | ||
| └─────────────────────────────────────┘ └─────────────────────────────────────────┘ |
|
Hi @smanes0213 : This is draft so not looking closely, but if you append: to the end of NOTICE in top level, I will clear off blackduck. Thank you. |
Hi @mhughesacn: The changes are not finalised yet, I have removed the license header for now. Will add the header once everything is confirmed. |
There was a problem hiding this comment.
Maybe we want a minimal main.cpp example that boots the runtime and exercises a basic JSON-RPC call against the built-in Controller — no external plugin .so needed. This would serve as a smoke test to verify the library links and initialises correctly end-to-end.
|
|
||
| // Invoke a JSON-RPC method on a loaded plugin. | ||
| // Method format: "Callsign.Version.method" (e.g. "Counter.1.increment") | ||
| uint32_t InvokeJSONRPC(const string& callsign, const string& method, |
There was a problem hiding this comment.
Was there a specific reason for having both callsign and method as separate parameters here?
As it stands, the callsign is already embedded in the fully-qualified method string ("Counter.1.increment"), so the caller ends up saying the same thing twice — and there's nothing stopping them from passing mismatched values, which would produce a confusing failure.
Would it make sense to drop the callsign parameter and derive it from method in the implementation instead?
see Core::JSONRPC::Message , const string callsign = Core::JSONRPC::Message::Callsign(method);
| string CommunicatorPath() const; | ||
|
|
||
| // Stop the server, release config, and clean up temp directories. | ||
| void Shutdown(); |
There was a problem hiding this comment.
Was there a reason for pairing Initialize() with Shutdown() rather than following one of Thunder's own naming conventions?
Thunder is consistent elsewhere — IPlugin uses Initialize()/Deinitialize(), and PluginHost::Server uses Open()/Close(). Using Shutdown() introduces a third scheme that fits neither, which feels slightly off for a library that lives inside the Thunder repo and targets Thunder plugin developers who are already familiar with those conventions.
Would Initialize()/Deinitialize() or Open()/Close() be a better fit here?
|
|
||
| #include <core/core.h> | ||
|
|
||
| MODULE_NAME_DECLARATION(BUILD_REFERENCE) |
There was a problem hiding this comment.
Thunder has a macro for static archives — MODULE_NAME_ARCHIVE_DECLARATION. Using it in Module.cpp instead of MODULE_NAME_DECLARATION(BUILD_REFERENCE) avoids conflicting definitions of ModuleBuildRef(), GetModuleServices() and SetModuleServices() with the consumer's own MODULE_NAME_DECLARATION. It also removes the need for the #ifndef guard here.
#define MODULE_NAME ThunderTestRuntime
#include <core/core.h>
MODULE_NAME_ARCHIVE_DECLARATION| #define MODULE_NAME Application | ||
| #endif | ||
|
|
||
| #include <plugins/plugins.h> |
There was a problem hiding this comment.
Nit: <plugins/plugins.h> can be replaced with <core/core.h> and <plugins/IShell.h> — the header only needs IShell and Core this avoids pulling in the entire plugins umbrella header into every consumer TU.
original -Wno-psabi only setting set CXX_STANDARD on thunder_test_support target so it respects the ENABLE_CXX17 flag like all other Thunder targets Pass -DENABLE_CXX17=OFF in the smoke test CI workflow to avoid the SocketServer.h IteratorType move constructor bug
There was a problem hiding this comment.
Pull request overview
This PR introduces a new thunder_test_support static library that embeds Thunder’s PluginHost::Server into a linkable archive so GTest-based binaries can boot an in-process Thunder runtime for plugin integration testing (without launching the daemon).
Changes:
- Adds
ThunderTestRuntimewrapper (API + implementation) to generate a minimal config, start/stop the embedded server, and invoke JSON-RPC/COM-RPC. - Adds a smoke-test executable that links against the new library and exercises the built-in Controller plugin.
- Integrates build/install/docs/CI support behind a new
ENABLE_TEST_RUNTIMECMake option.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/test_support/ThunderTestRuntime.h | Public test-runtime API (plugin list + JSON-RPC/COM-RPC helpers). |
| Tests/test_support/ThunderTestRuntime.cpp | Runtime implementation: temp dirs, generated config, server Open/Close, invocation helpers. |
| Tests/test_support/CMakeLists.txt | Builds/installs thunder_test_support and adds whole-archive link behavior. |
| Tests/test_support/Module.cpp | Static-archive module symbol setup for Thunder logging/module macros. |
| Tests/test_support/tests/SmokeTest.cpp | Smoke tests that boot the embedded server and call Controller JSON-RPC. |
| Tests/test_support/tests/Module.cpp | Module declaration for the smoke-test binary. |
| Tests/test_support/tests/CMakeLists.txt | Builds the smoke-test executable and links GTest + test support. |
| Tests/CMakeLists.txt | Adds ENABLE_TEST_RUNTIME option + subdirectory. |
| docs/ThunderTestSupport/ThunderTestSupport.md | Documentation for the new library, usage, and rationale. |
| .github/workflows/Test_Thunder_Test_Support.yml | CI workflow to build and run the smoke test (Debug/Release). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| << "\"locator\":" << JsonEscape(p.locator) << "," | ||
| << "\"classname\":" << JsonEscape(p.classname) << "," | ||
| << "\"startuporder\":" << p.startuporder << "," | ||
| << "\"autostart\":" << (p.autostart ? "true" : "false"); |
There was a problem hiding this comment.
Plugin::Config does not have an autostart field (it uses startmode), so this JSON key will be ignored during config parsing and plugins will always use the default startmode (Activated). This breaks the documented behavior of PluginConfig::autostart=false. Map this flag to "startmode":"Deactivated" (and optionally "startmode":"Activated" when true) or drop the field and expose startmode directly in PluginConfig.
| << "\"autostart\":" << (p.autostart ? "true" : "false"); | |
| << "\"startmode\":" << JsonEscape(p.autostart ? "Activated" : "Deactivated"); |
| << "\"datapath\":" << JsonEscape(_tempDir + "data/") << "," | ||
| << "\"systempath\":" << JsonEscape(systemPath) << "," | ||
| << "\"proxystubpath\":" << JsonEscape(proxyStubPath) << "," | ||
| << "\"communicator\":" << JsonEscape(_tempDir + "communicator") << "," |
There was a problem hiding this comment.
The generated communicator NodeId omits the |<mode> suffix (e.g. |0777) used by Thunder configs to set domain-socket permissions. With the current value, the socket file permissions depend on umask and may prevent out-of-process plugin processes from connecting in some environments. Consider appending |0777 (or making it configurable) to match Thunder’s default /tmp/communicator|0777.
| << "\"communicator\":" << JsonEscape(_tempDir + "communicator") << "," | |
| << "\"communicator\":" << JsonEscape(_tempDir + "communicator|0777") << "," |
| json << "{" | ||
| << "\"port\":0," | ||
| << "\"binding\":\"127.0.0.1\"," | ||
| << "\"idletime\":180," |
There was a problem hiding this comment.
Using "port":0 will bind the listener to an OS-assigned port, but Thunder’s Config (and thus Accessor()/URLs derived from it) will still report port 0 because SocketPort::Open() does not update NodeId after binding. If any plugin/test relies on the configured accessor URL/port, this will be incorrect. Consider choosing a free port up front (or querying the bound port via getsockname() and updating the config/binder/URL accordingly) so the runtime reports the real port.
| # Enforce --whole-archive for this target so consumers automatically pull in | ||
| # Thunder's static initializers (MODULE_NAME_DECLARATION, SERVICE_REGISTRATION). | ||
| # Scoped to this archive only via $<TARGET_FILE:...> — won't affect other libs. | ||
| target_link_options(thunder_test_support | ||
| INTERFACE | ||
| -Wl,--whole-archive $<TARGET_FILE:thunder_test_support> -Wl,--no-whole-archive | ||
| ) |
There was a problem hiding this comment.
--whole-archive/--no-whole-archive are GNU ld/LLD flags and will break on non-ELF linkers (e.g. Apple ld, MSVC) if ENABLE_TEST_RUNTIME is enabled. Please guard these link options by platform/toolchain (e.g. only on Linux/Android with GNU/Clang) and provide the equivalent flags where needed (e.g. -Wl,-force_load on Apple) or document the limitation.
| # Enforce --whole-archive for this target so consumers automatically pull in | |
| # Thunder's static initializers (MODULE_NAME_DECLARATION, SERVICE_REGISTRATION). | |
| # Scoped to this archive only via $<TARGET_FILE:...> — won't affect other libs. | |
| target_link_options(thunder_test_support | |
| INTERFACE | |
| -Wl,--whole-archive $<TARGET_FILE:thunder_test_support> -Wl,--no-whole-archive | |
| ) | |
| # Enforce whole-archive semantics for this target so consumers automatically pull in | |
| # Thunder's static initializers (MODULE_NAME_DECLARATION, SERVICE_REGISTRATION). | |
| # Use linker-specific flags only on supported platforms/toolchains. | |
| if(APPLE) | |
| target_link_options(thunder_test_support | |
| INTERFACE | |
| "-Wl,-force_load,$<TARGET_FILE:thunder_test_support>" | |
| ) | |
| elseif((CMAKE_SYSTEM_NAME STREQUAL "Linux" OR CMAKE_SYSTEM_NAME STREQUAL "Android") AND | |
| (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")) | |
| # Scoped to this archive only via $<TARGET_FILE:...> — won't affect other libs. | |
| target_link_options(thunder_test_support | |
| INTERFACE | |
| -Wl,--whole-archive $<TARGET_FILE:thunder_test_support> -Wl,--no-whole-archive | |
| ) | |
| else() | |
| message(WARNING | |
| "Automatic whole-archive linking for thunder_test_support is not configured for " | |
| "${CMAKE_SYSTEM_NAME} with ${CMAKE_CXX_COMPILER_ID}. Consumers may need to link " | |
| "the archive with platform-specific whole-archive/force-load options to ensure " | |
| "Thunder static initializers are included." | |
| ) | |
| endif() |
| install(TARGETS thunder_test_support | ||
| ARCHIVE DESTINATION lib | ||
| ) | ||
|
|
||
| install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/ThunderTestRuntime.h | ||
| DESTINATION include/thunder_test_support | ||
| ) |
There was a problem hiding this comment.
Install rules hardcode lib/include instead of using GNUInstallDirs variables (${CMAKE_INSTALL_LIBDIR}, ${CMAKE_INSTALL_INCLUDEDIR}) used elsewhere in the project. Using the standard variables improves portability across distros/multilib layouts.
| │ │ ↓ │ │ │ │ │ └── (no HTTP listener) │ │ | ||
| │ │ Plugin .so (dynamic load) │ │ │ │ └── Plugin .so (dynamic load) │ │ | ||
| │ └───────────────────────────────┘ │ │ └─────────────────────────────────┘ │ | ||
| │ │ │ │ | ||
| │ Communication: HTTP/WS/COMRPC │ │ Communication: Direct in-process calls │ |
There was a problem hiding this comment.
The diagram states “(no HTTP listener)”, but PluginHost::Server::Open() always calls _connections.Open(...) and starts the HTTP/WS listener (even if bound to localhost/port 0). Please update the documentation to reflect that the listener is still started (and clarify that tests typically bypass it via in-process dispatcher calls).
| │ │ ↓ │ │ │ │ │ └── (no HTTP listener) │ │ | |
| │ │ Plugin .so (dynamic load) │ │ │ │ └── Plugin .so (dynamic load) │ │ | |
| │ └───────────────────────────────┘ │ │ └─────────────────────────────────┘ │ | |
| │ │ │ │ | |
| │ Communication: HTTP/WS/COMRPC │ │ Communication: Direct in-process calls │ | |
| │ │ ↓ │ │ │ │ │ └── HTTP/WS Listener │ │ | |
| │ │ Plugin .so (dynamic load) │ │ │ │ └── Plugin .so (dynamic load) │ │ | |
| │ └───────────────────────────────┘ │ │ └─────────────────────────────────┘ │ | |
| │ │ │ │ | |
| │ Communication: HTTP/WS/COMRPC │ │ Communication: In-process dispatcher │ | |
| │ │ │ calls (listener still started) │ |
| ### Why port 0? | ||
|
|
||
| Using port 0 in the config tells the OS to assign an available port, avoiding conflicts when multiple test processes run simultaneously. | ||
|
|
There was a problem hiding this comment.
The “Why port 0?” section implies port 0 is a clean solution for parallel runs, but with the current implementation the configured accessor/URL will still show port 0 (the actual bound port is not propagated back into Config). This can confuse readers and may break plugins/tests that depend on the configured port. Consider documenting this caveat or switching to a strategy that discovers and records the assigned port.
| **Caveat:** with the current implementation, the OS-assigned port is used only for binding; it is not propagated back into `Config`. As a result, config-derived accessors/URLs may still show port `0` rather than the actual bound port. This is still useful for avoiding bind conflicts in parallel runs, but tests and plugins should not rely on the configured port value when port `0` is used. |
| string callsign; // Plugin callsign (e.g. "Counter") | ||
| string locator; // Shared library name (e.g. "libThunderCounterImplementation.so") | ||
| string classname; // Class name registered via SERVICE_REGISTRATION | ||
| bool autostart = true; // Whether to auto-activate on startup | ||
| int startuporder = 50; // Activation priority (lower = earlier) | ||
| string configuration; // Optional JSON object for plugin-specific config |
There was a problem hiding this comment.
PluginConfig::autostart suggests a supported way to prevent activation, but Thunder’s plugin config uses startmode (Activated/Deactivated/Unavailable) rather than an autostart boolean. Unless BuildConfigJSON() maps this flag to startmode, the public API will be misleading. Consider replacing autostart with a startmode enum in PluginConfig (or clearly document the mapping).
| string callsign; // Plugin callsign (e.g. "Counter") | |
| string locator; // Shared library name (e.g. "libThunderCounterImplementation.so") | |
| string classname; // Class name registered via SERVICE_REGISTRATION | |
| bool autostart = true; // Whether to auto-activate on startup | |
| int startuporder = 50; // Activation priority (lower = earlier) | |
| string configuration; // Optional JSON object for plugin-specific config | |
| enum class StartMode { | |
| Activated, | |
| Deactivated, | |
| Unavailable | |
| }; | |
| string callsign; // Plugin callsign (e.g. "Counter") | |
| string locator; // Shared library name (e.g. "libThunderCounterImplementation.so") | |
| string classname; // Class name registered via SERVICE_REGISTRATION | |
| StartMode startmode = StartMode::Activated; // Thunder plugin start mode | |
| int startuporder = 50; // Activation priority (lower = earlier) | |
| string configuration; // Optional JSON object for plugin-specific config |
| target_include_directories(thunder_test_support | ||
| PUBLIC | ||
| ${CMAKE_CURRENT_SOURCE_DIR} | ||
| PRIVATE | ||
| ${THUNDER_SOURCE_DIR} | ||
| $<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/Source/plugins/generated/jsonrpc> | ||
| $<BUILD_INTERFACE:${CMAKE_BINARY_DIR}/Source/Thunder/generated> | ||
| ) | ||
|
|
||
| target_link_libraries(thunder_test_support | ||
| PUBLIC | ||
| ${NAMESPACE}Core::${NAMESPACE}Core | ||
| ${NAMESPACE}Cryptalgo::${NAMESPACE}Cryptalgo | ||
| ${NAMESPACE}COM::${NAMESPACE}COM | ||
| ${NAMESPACE}Messaging::${NAMESPACE}Messaging | ||
| ${NAMESPACE}WebSocket::${NAMESPACE}WebSocket | ||
| ${NAMESPACE}Plugins::${NAMESPACE}Plugins | ||
| ${NAMESPACE}COMProcess::${NAMESPACE}COMProcess | ||
| Threads::Threads | ||
| ) | ||
|
|
||
| if(WARNING_REPORTING) | ||
| target_sources(thunder_test_support PRIVATE ${THUNDER_SOURCE_DIR}/WarningReportingCategories.cpp) | ||
| endif() | ||
|
|
||
| if(PROCESSCONTAINERS) | ||
| target_link_libraries(thunder_test_support | ||
| PUBLIC | ||
| ${NAMESPACE}ProcessContainers::${NAMESPACE}ProcessContainers) | ||
| target_compile_definitions(thunder_test_support | ||
| PUBLIC | ||
| PROCESSCONTAINERS_ENABLED=1) | ||
| endif() | ||
|
|
||
| if(HIBERNATESUPPORT) | ||
| target_link_libraries(thunder_test_support PUBLIC | ||
| ${NAMESPACE}Hibernate::${NAMESPACE}Hibernate) | ||
| target_compile_definitions(thunder_test_support PUBLIC | ||
| HIBERNATE_SUPPORT_ENABLED=1) | ||
| endif() | ||
|
|
||
| # Enforce whole-archive semantics for this target so consumers automatically pull in | ||
| # Thunder's static initializers (MODULE_NAME_DECLARATION, SERVICE_REGISTRATION). | ||
| # Use linker-specific flags only on supported platforms/toolchains. | ||
| if(APPLE) | ||
| target_link_options(thunder_test_support | ||
| INTERFACE | ||
| "-Wl,-force_load,$<TARGET_FILE:thunder_test_support>" | ||
| ) | ||
| elseif((CMAKE_SYSTEM_NAME STREQUAL "Linux" OR CMAKE_SYSTEM_NAME STREQUAL "Android") AND | ||
| (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")) | ||
| # Scoped to this archive only via $<TARGET_FILE:...> — won't affect other libs. | ||
| target_link_options(thunder_test_support | ||
| INTERFACE | ||
| -Wl,--whole-archive $<TARGET_FILE:thunder_test_support> -Wl,--no-whole-archive | ||
| ) | ||
| else() | ||
| message(WARNING | ||
| "Automatic whole-archive linking for thunder_test_support is not configured for " | ||
| "${CMAKE_SYSTEM_NAME} with ${CMAKE_CXX_COMPILER_ID}. Consumers may need to link " | ||
| "the archive with platform-specific whole-archive/force-load options to ensure " | ||
| "Thunder static initializers are included." | ||
| ) | ||
| endif() | ||
|
|
||
| # --- Install rules --- | ||
| install(TARGETS thunder_test_support | ||
| ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} | ||
| ) | ||
|
|
||
| install(FILES ${CMAKE_CURRENT_SOURCE_DIR}/ThunderTestRuntime.h | ||
| DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/thunder_test_support | ||
| ) |
There was a problem hiding this comment.
The docs and this CMake target describe consumers linking against the thunder_test_support CMake target to pick up INTERFACE whole-archive link options, but the install rules here only install the .a and header—there is no target export / CMake package config for thunder_test_support, and the include dirs also lack an INSTALL_INTERFACE. If this library is intended for external consumers via find_package, it should be exported and install-interface include dirs should be set; otherwise the docs should avoid implying the imported target exists / carries link options.
| // Create unique temp directory for this test run | ||
| char tempTemplate[] = "/tmp/thunder_test_XXXXXX"; | ||
| char* tempResult = mkdtemp(tempTemplate); | ||
| if (tempResult == nullptr) { | ||
| return Core::ERROR_GENERAL; | ||
| } | ||
| _tempDir = string(tempResult) + "/"; |
There was a problem hiding this comment.
Initialize() hardcodes a POSIX /tmp/... path and uses mkdtemp(), which will not build on non-POSIX platforms (e.g., Windows) if ENABLE_TEST_RUNTIME is enabled. Either gate this feature to POSIX in CMake (with a clear message when enabled on unsupported platforms) or provide a platform-specific temp directory implementation.
| endif() | ||
|
|
||
| if(ENABLE_TEST_RUNTIME) | ||
| add_subdirectory(test_support) |
There was a problem hiding this comment.
ENABLE_TEST_RUNTIME can currently be enabled on any platform, but Tests/test_support/ThunderTestRuntime.cpp uses POSIX-only APIs (mkdtemp) and hardcoded /tmp paths. Consider adding a platform guard here (e.g., only add the test_support subdirectory on POSIX platforms, or emit a fatal error if the option is ON on unsupported systems) to avoid configuration/build failures.
| add_subdirectory(test_support) | |
| if(UNIX) | |
| add_subdirectory(test_support) | |
| else() | |
| message(FATAL_ERROR "ENABLE_TEST_RUNTIME is only supported on POSIX/UNIX platforms because Tests/test_support uses POSIX-only APIs.") | |
| endif() |
|
|
||
| ```cpp | ||
| #include <gtest/gtest.h> | ||
| #include "ThunderTestRuntime.h" |
There was a problem hiding this comment.
The usage snippet includes #include "ThunderTestRuntime.h", but the install rule places the header under ${CMAKE_INSTALL_INCLUDEDIR}/thunder_test_support/ThunderTestRuntime.h. For installed consumers, the include should match the installed layout (e.g. include via the thunder_test_support/ prefix) unless you also ensure the install exports an include directory that points directly at the thunder_test_support folder.
| #include "ThunderTestRuntime.h" | |
| #include <thunder_test_support/ThunderTestRuntime.h> |
| #### 2. Test fixture | ||
|
|
||
| ```cpp | ||
| #include <gtest/gtest.h> |
There was a problem hiding this comment.
The example test fixture does not show defining MODULE_NAME before including ThunderTestRuntime.h, but the installed header currently includes <core/core.h> which emits a hard error unless MODULE_NAME is defined in that translation unit. Either update the example to show the required #define MODULE_NAME ... (or including a local Module.h that defines it) or adjust the public header to avoid pulling in <core/core.h> so consumers don't need to manage MODULE_NAME in every .cpp.
| #include <gtest/gtest.h> | |
| #include <gtest/gtest.h> | |
| // `ThunderTestRuntime.h` currently pulls in Thunder core headers that require | |
| // `MODULE_NAME` to be defined in this translation unit. If your project already | |
| // has a local Module.h that defines it, include that instead of defining it here. | |
| #define MODULE_NAME MyPluginTest |
|
|
||
| #include <core/core.h> |
There was a problem hiding this comment.
ThunderTestRuntime.h includes <core/core.h>, which hard-requires MODULE_NAME to be defined in every translation unit that includes this header. This makes the public API cumbersome to consume and also contradicts the usage example here (it doesn't define MODULE_NAME). Consider removing the <core/core.h> include from the public header (include only the minimal core headers you need, e.g. for Core::ProxyType and error codes), or explicitly document that consumers must #define MODULE_NAME before including this header in each .cpp that includes it.
| #include <core/core.h> | |
| #ifndef MODULE_NAME | |
| #define MODULE_NAME ThunderTestRuntime | |
| #define THUNDER_TEST_RUNTIME_DEFINED_MODULE_NAME | |
| #endif | |
| #include <core/core.h> | |
| #ifdef THUNDER_TEST_RUNTIME_DEFINED_MODULE_NAME | |
| #undef THUNDER_TEST_RUNTIME_DEFINED_MODULE_NAME | |
| #undef MODULE_NAME | |
| #endif |
| PRIVATE | ||
| NAMESPACE=${NAMESPACE} | ||
| APPLICATION_NAME=ThunderTestRuntime | ||
| MODULE_NAME=ThunderTestRuntime |
There was a problem hiding this comment.
MODULE_NAME is set both via target_compile_definitions(... MODULE_NAME=ThunderTestRuntime) and again via #define MODULE_NAME ThunderTestRuntime in Tests/test_support/Module.cpp. This can trigger macro redefinition warnings and also forces the module name for all compiled Thunder server sources in this archive. Prefer defining MODULE_NAME only in the dedicated Module.cpp (and drop the compile definition here), similar to how the main Thunder target sets APPLICATION_NAME/THREADPOOL_COUNT but not MODULE_NAME.
| MODULE_NAME=ThunderTestRuntime |
|
|
||
| set(THUNDER_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../../Source/Thunder") | ||
|
|
||
| set(THREADPOOL_COUNT "4" CACHE STRING "The number of threads in the thread pool for test runtime") |
There was a problem hiding this comment.
THREADPOOL_COUNT is already defined as a CACHE variable in Source/Thunder/CMakeLists.txt, and this set(... CACHE ...) in a subdirectory will reuse/overwrite the same cache entry (including its help text). To avoid confusing cache UI/help strings, consider not re-declaring it here and just consuming the existing cache variable (or use a uniquely named cache var specific to test support).
| set(THREADPOOL_COUNT "4" CACHE STRING "The number of threads in the thread pool for test runtime") | |
| if(NOT DEFINED THREADPOOL_COUNT) | |
| set(THREADPOOL_COUNT "4") | |
| endif() |
|
Hi @smanes0213 : I have cleared off the blackduck issue in the tool but please add headers to the source code in this PR. Thank you! |
sebaszm
left a comment
sebaszm
left a comment
There was a problem hiding this comment.
See comments!
Additionally we need to think how to incorporate JSON-RPC event handlig into this solution and remove the need of knowing the callsign of the tested plugin (or at least repeating it with every JSON-RPC call).
Also please ensure this works with Thunder 5.3. (Autostart and 'wpeframework' in paths suggest it was checked with 4.4.) Thanks!
| // Uses port 0 (OS-assigned) and binds to localhost only. | ||
| // Helper: JSON-escape a string value and return it quoted (e.g. "foo\"bar" → "\"foo\\\"bar\"") | ||
| // Uses Core::JSON::String serialization to handle escaping correctly. | ||
| static string JsonEscape(const string& value) |
There was a problem hiding this comment.
Suggest to use Core::JSON methods instead of creating JSON by hand. It will be cleaner, more flexible less error-prone and removes the need for manual escaping.
| << "\"locator\":" << JsonEscape(p.locator) << "," | ||
| << "\"classname\":" << JsonEscape(p.classname) << "," | ||
| << "\"startuporder\":" << p.startuporder << "," | ||
| << "\"autostart\":" << (p.autostart ? "true" : "false"); |
There was a problem hiding this comment.
There is no autostart in 5.x in anymore. Perhaps it's then better to actually reuse the plugin Configuration as defined in plugns/Configuration.h, that would've eliminate such issues.
|
|
||
| // Create unique temp directory for this test run | ||
| char tempTemplate[] = "/tmp/thunder_test_XXXXXX"; | ||
| char* tempResult = mkdtemp(tempTemplate); |
There was a problem hiding this comment.
For OS-abstraction use Thunder Core functionality (also to remove posix dependency).
| // Determine system path for plugin .so files | ||
| string sysPath = systemPath; | ||
| if (sysPath.empty()) { | ||
| sysPath = "/usr/lib/wpeframework/plugins/"; |
There was a problem hiding this comment.
Such path must not be hardcoded! On 5.x the path is different.
| // Default: look next to system path | ||
| _proxyStubPath = sysPath + "../proxystubs/"; | ||
| } | ||
| if (_proxyStubPath.back() != '/') { |
There was a problem hiding this comment.
Use Thunder-provided functionality for normalizing paths!
|
|
||
| // Helper to escape a string for safe inclusion as a JSON string value. | ||
| // It escapes quotes, backslashes, and control characters (< 0x20). | ||
| static std::string JsonEscape(const std::string& input) |
There was a problem hiding this comment.
Use Thunder Core JSON for JSON handling, no need for manual escaping.
| { | ||
| const string communicatorPath = _tempDir + "communicator|0777"; | ||
|
|
||
| std::ostringstream json; |
There was a problem hiding this comment.
Use Thunder Core JSON for JSON manipulation!
| public: | ||
| // Describes a plugin to be loaded by the test runtime. | ||
| // Maps directly to a Thunder plugin JSON config entry. | ||
| struct PluginConfig { |
There was a problem hiding this comment.
Can't Thunder Configuration be reused here, insted of crafting another one?
| if (dot == string::npos) { | ||
| return Core::ERROR_INVALID_SIGNATURE; | ||
| } | ||
| string callsign = method.substr(0, dot); |
There was a problem hiding this comment.
There are Thunder Core JSON Message helper methods for this.
| // | ||
| // TEST_F(MyTest, JsonRpc) { | ||
| // string resp; | ||
| // _runtime.InvokeJSONRPC("Callsign.1.method", params, resp); |
There was a problem hiding this comment.
Remove all .1 from JSON-RPC method designators, we don't endorse versioning.
| string callsign = Core::JSONRPC::Message::Callsign(method); | ||
| string methodName = Core::JSONRPC::Message::Method(method); | ||
|
|
||
| if (callsign.empty() == true) { |
There was a problem hiding this comment.
Invoke() validates that the callsign is non-empty, but it doesn’t validate that the extracted method name is non-empty (e.g., "Controller." yields an empty method). In that case the error returned becomes ERROR_UNKNOWN_KEY via the exists check, which is misleading. Consider treating an empty methodName as an invalid signature and returning Core::ERROR_INVALID_SIGNATURE (or similar) early.
| if (callsign.empty() == true) { | |
| if ((callsign.empty() == true) || (methodName.empty() == true)) { |
| find_package(GTest REQUIRED) | ||
|
|
||
| add_executable(thunder_test_runtime_smoke | ||
| SmokeTest.cpp | ||
| Module.cpp | ||
| ) | ||
|
|
||
| target_link_libraries(thunder_test_runtime_smoke | ||
| PRIVATE | ||
| thunder_test_support | ||
| GTest::GTest | ||
| GTest::Main | ||
| ) |
There was a problem hiding this comment.
find_package(GTest REQUIRED) makes configuring the project fail whenever ENABLE_TEST_RUNTIME=ON but GTest isn’t installed, even though the static library itself doesn’t require GTest. Consider using find_package(GTest QUIET) (or gating this subdirectory behind an option) and only adding the smoke-test target when GTest is found, so consumers can still build/install thunder_test_support without GTest.
| find_package(GTest REQUIRED) | |
| add_executable(thunder_test_runtime_smoke | |
| SmokeTest.cpp | |
| Module.cpp | |
| ) | |
| target_link_libraries(thunder_test_runtime_smoke | |
| PRIVATE | |
| thunder_test_support | |
| GTest::GTest | |
| GTest::Main | |
| ) | |
| find_package(GTest QUIET) | |
| if(GTest_FOUND) | |
| add_executable(thunder_test_runtime_smoke | |
| SmokeTest.cpp | |
| Module.cpp | |
| ) | |
| target_link_libraries(thunder_test_runtime_smoke | |
| PRIVATE | |
| thunder_test_support | |
| GTest::GTest | |
| GTest::Main | |
| ) | |
| endif() |
| JsonObject config; | ||
| JsonArray pluginList; | ||
|
|
There was a problem hiding this comment.
This implementation relies on the global aliases JsonObject/JsonArray/JsonValue, which are not available when DISABLE_USE_COMPLEMENTARY_CODE_SET is enabled (it defines __DISABLE_USE_COMPLEMENTARY_CODE_SET__). In that build mode this file won’t compile. Use the fully-qualified Core JSON types (e.g., Thunder::Core::JSON::VariantContainer / Thunder::Core::JSON::ArrayType<...> / Thunder::Core::JSON::Variant) instead of the global aliases so the test runtime builds with and without the complementary code set.
| const string communicatorPath = _tempDir + "communicator|0777"; | ||
|
|
||
| JsonObject config; | ||
| JsonArray pluginList; | ||
|
|
||
| config["port"] = 0; | ||
| config["binding"] = "127.0.0.1"; | ||
| config["idletime"] = 180; | ||
| config["persistentpath"] = _tempDir + "persistent/"; | ||
| config["volatilepath"] = _tempDir + "volatile/"; | ||
| config["datapath"] = _tempDir + "data/"; | ||
| config["systempath"] = systemPath; | ||
| config["proxystubpath"] = proxyStubPath; | ||
| config["communicator"] = communicatorPath; | ||
|
|
There was a problem hiding this comment.
The communicator socket is configured with permissions 0777 ("communicator|0777"), which makes the UNIX domain socket world-accessible. Even for tests, this is unnecessarily permissive on multi-user systems and can allow other users/processes to connect to the embedded runtime. Consider defaulting to a more restrictive mode (e.g., 0700 or respecting umask) and/or making the permission configurable via Initialize().
| // Initialize the messaging subsystem (must happen before Server creation, | ||
| // mirrors the MessagingInitialization() call in the real PluginHost main()). | ||
| Messaging::MessageUnit::Settings::Config messagingConfig; | ||
| uint32_t messagingResult = Messaging::MessageUnit::Instance().Open( | ||
| _config->VolatilePath(), messagingConfig, false, Messaging::MessageUnit::flush::OFF); | ||
|
|
||
| if (messagingResult != Core::ERROR_NONE) { | ||
| TRACE_L1("ThunderTestRuntime: Failed to initialize messaging subsystem (0x%08X)", messagingResult); | ||
| } | ||
|
|
There was a problem hiding this comment.
Messaging::MessageUnit::Instance().Open(...) is called unconditionally. MessageUnit::Open asserts that it’s not already open (it expects internal buffers to be null), so this will trigger assertions if the test process (or another runtime instance) already initialized messaging. Also, Deinitialize() always closes the global singleton, which can break other Thunder components in the same process. Consider tracking whether this runtime performed the open and only closing in that case, or adding an Initialize() option to skip messaging initialization when it’s managed by the caller.
Adds a static library (libthunder_test_support.a) that embeds the Thunder PluginHost::Server into a linkable archive. Test binaries link against this library to boot a real Thunder runtime without launching the standalone daemon, enabling integration testing of Thunder plugins using GTest/GMock.