RDKEMW-16911 - L1 unit tests for xdialserver#199
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an Autotools-driven L1 (unit-level) test harness for xdialserver, including a GitHub Actions workflow to build/run tests (with optional valgrind + coverage) in CI.
Changes:
- Introduces
run_L1TestsGoogleTest/GoogleMock binary with L1 test suites for server, platform, and utility layers. - Adds test stubs/mocks (incl. GSSDP) and CI-generated shadow headers to decouple tests from external SDK/framework headers.
- Adds Autotools plumbing (
configure.ac,Makefile.amfiles) plus a CI workflow to build, run, and publish artifacts.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/mocks/xdialserver/Makefile.am | Placeholder Makefile for shared mock area. |
| tests/README.md | Documents L1 test harness layout, build, CI, and pitfalls. |
| tests/Makefile.am | Adds conditional test subdir build via --enable-l1tests. |
| tests/L1Tests/utils/test_gdialUtil.cpp | Adds unit tests for gdial-util.c. |
| tests/L1Tests/utils/gdial_util_stubs.h | Utility stubs header (currently placeholder). |
| tests/L1Tests/utils/gdial_util_stubs.cpp | Utility stubs TU (currently placeholder). |
| tests/L1Tests/tests/gdialserver_ut.cpp | Adds a template/placeholder test TU. |
| tests/L1Tests/test_main.cpp | GTest/GMock main entrypoint for run_L1Tests. |
| tests/L1Tests/stubs/xdialserver_test_stubs.h | Aggregated stub declaration header for shadow headers. |
| tests/L1Tests/stubs/xdialserver_test_stubs.cpp | Provides stubs for logging symbols used by GDIAL_LOG* macros. |
| tests/L1Tests/stubs/gdial_cpp_test_stubs.hpp | Minimal C++ shims for WPEFramework-ish types used by gdial.cpp. |
| tests/L1Tests/server/test_gdialserver_ut.cpp | Tests for server/gdialserver_ut.cpp interactive command flow. |
| tests/L1Tests/server/test_gdialSsdp.cpp | SSDP tests (init guards + dd.xml callback behavior). |
| tests/L1Tests/server/test_gdialShield.cpp | Tests for gdial-shield server wrapping behavior. |
| tests/L1Tests/server/test_gdialService.cpp | Tests for gdialService/gdialServiceImpl flows with conditional start. |
| tests/L1Tests/server/test_gdialServer.cpp | Tests for gdial-app (state, lifecycle, additional-dial-data helpers). |
| tests/L1Tests/server/test_gdialRest.cpp | REST server/builder tests using libsoup servers/sessions. |
| tests/L1Tests/server/gdial_rest_stubs.h | REST stub header (currently placeholder). |
| tests/L1Tests/server/gdial_rest_stubs.cpp | REST stub TU (currently placeholder). |
| tests/L1Tests/plat/test_gdialPlatUtil.cpp | Tests for platform util functions + logging behavior. |
| tests/L1Tests/plat/test_gdialPlatDev.cpp | Tests for platform dev power/network standby callbacks. |
| tests/L1Tests/plat/test_gdialPlatApp.cpp | Tests for gdial-plat-app guards, sync calls, and async handles. |
| tests/L1Tests/plat/test_gdialPlat.cpp | Tests for gdial_app_registry behavior + disposal. |
| tests/L1Tests/plat/test_gdialCpp.cpp | Pulls real gdial.cpp into a test TU with symbol renames and exercises key paths. |
| tests/L1Tests/plat/gdial_plat_stubs.h | Platform stubs header (currently placeholder). |
| tests/L1Tests/plat/gdial_plat_stubs.cpp | Platform stub TU intentionally exporting no symbols. |
| tests/L1Tests/plat/gdial_os_stubs.cpp | Stubs for the “OS” layer under gdial-plat-app.c and for gdial.hpp APIs. |
| tests/L1Tests/mocks/gssdp_mock.c | Minimal GSSDP mocks to allow SSDP paths to execute under test. |
| tests/L1Tests/mocks/Makefile.am | Placeholder Makefile for mocks subdir. |
| tests/L1Tests/mocks/IarmBusMock.h | Placeholder IARM bus mock header. |
| tests/L1Tests/mocks/IarmBusMock.cpp | Placeholder IARM bus mock TU. |
| tests/L1Tests/Makefile.am | Build recipe for run_L1Tests (sources, include paths, link flags). |
| configure.ac | Adds Autotools configuration, dependency checks, and --enable-l1tests. |
| Makefile.am | Top-level Automake entry to build tests subdir. |
| .github/workflows/L1-tests.yml | CI workflow to build/run tests (valgrind + coverage + artifacts). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Replace this with actual unit tests for xdialserver components | ||
| */ | ||
| TEST_F(GDialServerTest, ExampleTest) { | ||
| EXPECT_TRUE(true); |
There was a problem hiding this comment.
This file is an “example/template” test and currently asserts EXPECT_TRUE(true), which adds no signal and can mask accidental test registration/build issues. Consider removing it, marking it with GTEST_SKIP() until real cases exist, or replacing it with a minimal real behavioral test for an xdialserver component.
| EXPECT_TRUE(true); | |
| GTEST_SKIP() << "Placeholder test: add real GDialServer behavioral coverage."; |
| workflow_call: | ||
| secrets: | ||
| RDKCM_RDKE: | ||
| required: true |
There was a problem hiding this comment.
workflow_call declares RDKCM_RDKE as a required secret, but the only step that would use it (checkout of entservices-testframework) is commented out. As-is, any caller must provide an unused secret. Either make the secret optional or re-enable/remove the dependent step so the requirement matches actual usage.
| required: true | |
| required: false |
| (void)gdial_args; | ||
| (void)actualprocessName; | ||
| ++g_get_instance_calls; | ||
| return reinterpret_cast<gdialServiceFake*>(0x1); |
There was a problem hiding this comment.
gdialServiceFake::getInstance() returns reinterpret_cast<gdialServiceFake*>(0x1), which is undefined behavior once gdialserver_ut.cpp calls instance methods (member functions still receive/expect a valid this). Return the address of a real static/heap gdialServiceFake instance instead, and ensure destroyInstance() matches that ownership model.
| return reinterpret_cast<gdialServiceFake*>(0x1); | |
| static gdialServiceFake instance; | |
| return &instance; |
| opt.manufacturer = g_strdup("L1Maker"); | ||
| opt.model_name = g_strdup("L1Model"); |
There was a problem hiding this comment.
This test allocates opt.manufacturer / opt.model_name with g_strdup, but gdial_ssdp_destroy() does not free those fields (it only frees friendly_name, uuid, and iface_name). Under valgrind this will show up as leaked test allocations. Free opt.manufacturer and opt.model_name in the test after gdial_ssdp_destroy() (or avoid allocating them if not needed).
| opt.manufacturer = g_strdup("L1Maker"); | |
| opt.model_name = g_strdup("L1Model"); |
| GDialOptions opt = {}; | ||
| opt.iface_name = g_strdup("lo"); | ||
| opt.friendly_name = g_strdup("L1Friendly"); | ||
| opt.manufacturer = g_strdup("L1Maker"); | ||
| opt.model_name = g_strdup("L1Model"); | ||
| opt.uuid = g_strdup("12345678-abcd-abcd-1234-123456789abc"); |
There was a problem hiding this comment.
Same leak as the other SSDP HTTP test: opt.manufacturer / opt.model_name are g_strdup'd but not freed by gdial_ssdp_destroy(). Free those fields in the test cleanup to avoid valgrind-reported leaks.
| - name: Set up CMake | ||
| uses: jwlawson/actions-setup-cmake@v1.13 | ||
| with: | ||
| cmake-version: '3.16.x' | ||
|
|
There was a problem hiding this comment.
The workflow defines two separate “Set up CMake” steps (one at the start and one again a few steps later). This duplication increases runtime and maintenance cost; keep a single setup step unless different versions/configs are required.
| - name: Set up CMake | |
| uses: jwlawson/actions-setup-cmake@v1.13 | |
| with: | |
| cmake-version: '3.16.x' |
| g_destroy_instance_calls = 0; | ||
| g_activation_calls = 0; | ||
| g_register_calls = 0; | ||
| } |
There was a problem hiding this comment.
The global call counters (g_get_instance_calls, g_destroy_instance_calls, etc.) are never reset between tests, and reset_stub_state() is currently unused. This can make expectations order-dependent if another test in the same binary exercises these stubs first. Reset the counters in SetUp() (and/or TearDown()) for this fixture.
| } | |
| } | |
| class StubStateResetListener : public ::testing::EmptyTestEventListener { | |
| public: | |
| void OnTestStart(const ::testing::TestInfo&) override { | |
| reset_stub_state(); | |
| } | |
| void OnTestEnd(const ::testing::TestInfo&) override { | |
| reset_stub_state(); | |
| } | |
| }; | |
| struct StubStateResetListenerRegistrar { | |
| StubStateResetListenerRegistrar() { | |
| ::testing::UnitTest::GetInstance()->listeners().Append(new StubStateResetListener()); | |
| } | |
| }; | |
| StubStateResetListenerRegistrar g_stub_state_reset_listener_registrar; |
No description provided.