diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000..e80616f --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,84 @@ +# firebolt-cpp-client Copilot Instructions + +Scope: This file applies to the firebolt-cpp-client repository. + +## Primary goals + +- Preserve API contract correctness across interface, implementation, tests, and OpenRPC fixtures. +- Keep generated surfaces and bespoke conventions aligned. +- Prefer minimal, targeted changes. + +## High-signal workflow + +1. For API-facing changes, update all of the following in one pass: + - `include/firebolt/*.h` + - `src/*_impl.h` and `src/*_impl.cpp` + - `test/unit/*Test.cpp` and `test/component/*Test.cpp` + - `docs/openrpc/the-spec/firebolt-open-rpc.json` when component tests depend on fixture shape. +2. Run component tests after edits. +3. If behavior is generator-owned, patch generator code in sibling repo and regenerate module artifacts. + +## Test commands + +- Local one-shot (current preferred): + - `./run-component-tests-local.sh` + - `./run-component-tests-local.sh --skip-image-build` +- Legacy wrappers may still exist in conversation history; prefer the local script in this repo. +- Unit-only: + - `./run-unit-tests.sh` + +## Actions module rules (important) + +- `Actions.intent` is getter-only: + - takes no parameters + - returns `Result` +- `Actions.onIntent` callback payload is a string value. +- Component event trigger for `Actions.onIntent` should use a string JSON payload (for example `"launch"`), not an object. + +## Generated-code conventions that must be preserved + +- `*Impl` classes should delete copy constructor and copy assignment: + - `ClassName(const ClassName&) = delete;` + - `ClassName& operator=(const ClassName&) = delete;` +- Unless explicitly justified as safe, `*Impl` classes should also delete move operations: + - `ClassName(ClassName&&) = delete;` + - `ClassName& operator=(ClassName&&) = delete;` +- Keep include hygiene strict: + - include `` when using `std::move` + - remove unused includes such as `` when not used +- Keep test names in consistent CamelCase for filtering. + +## Component test expectations + +- Red schema validation lines in logs can be expected for negative-path tests. +- Negative tests must still verify runtime behavior (callbacks not delivered for invalid payloads), not just compile-time surface checks. + +## OpenRPC fixture expectations + +- Module descriptions must match actual API behavior. +- Getter-style methods should carry property tags consistent with the rest of the file (for example `property:readonly` where applicable). +- Keep notifier/subscriber metadata aligned (`x-notifier`, `x-subscriber-for`). + +## Regeneration notes (sibling repo) + +When a change is generator-owned, use `firebolt-sdk-gen` and apply module-scoped output back into this repo. + +Typical flow: + +- From `../firebolt-sdk-gen`: + - `./sync-plan-checklist.sh --profile core --module actions --apply --no-accessor-touchpoints --target-root ../firebolt-cpp-client` + +This keeps migration incremental and avoids unrelated accessor touchpoint churn. + +## CI parity reminders + +- CI uses Dockerized build/test flow and mock-firebolt integration. +- Keep changes compatible with: + - `.github/workflows/ci.yml` + - `.github/scripts/run-component-tests.sh` + +## PR hygiene + +- If a review asks for include-file fixes, prefer precise header/source edits and re-run component tests. +- Do not relax negative tests just to suppress red validation logs. +- Keep commit messages scoped and explicit (example: `fix(actions): address include review comments`). diff --git a/CHANGELOG.md b/CHANGELOG.md index 799eb4e..58e532d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ### Added - New APIs - - `Actions.intent` + - `Actions.intent` (no parameters, returns string) - `Actions.onIntent` event ### Changed diff --git a/docs/openrpc/the-spec/firebolt-open-rpc.json b/docs/openrpc/the-spec/firebolt-open-rpc.json index 82b8600..9ad9084 100644 --- a/docs/openrpc/the-spec/firebolt-open-rpc.json +++ b/docs/openrpc/the-spec/firebolt-open-rpc.json @@ -5,6 +5,7 @@ "version": "", "x-module-descriptions": { "Accessibility": "The `Accessibility` module provides access to the user/device settings for closed captioning and voice guidance.\n\nApps **SHOULD** attempt o respect these settings, rather than manage and persist seprate settings, which would be different per-app.", + "Actions": "Methods for getting and observing app intents.", "Advertising": "A module for platform provided advertising settings and functionality.", "Device": "A module for querying about the device and it's capabilities.", "Discovery": "Your App likely wants to integrate with the Platform's discovery capabilities. For example to add a \"Watch Next\" tile that links to your app from the platform's home screen.\n\nGetting access to this information requires to connect to lower level APIs made available by the platform. Since implementations differ between operators and platforms, the Firebolt SDK offers a Discovery module, that exposes a generic, agnostic interface to the developer.\n\nUnder the hood, an underlaying transport layer will then take care of calling the right APIs for the actual platform implementation that your App is running on.\n\nThe Discovery plugin is used to _send_ information to the Platform.\n\n### Localization\nApps should provide all user-facing strings in the device's language, as specified by the Firebolt `Localization.language` property.\n\nApps should provide prices in the same currency presented in the app. If multiple currencies are supported in the app, the app should provide prices in the user's current default currency.", @@ -48,6 +49,85 @@ } ] }, + { + "name": "Actions.intent", + "summary": "Returns the current intent.", + "tags": [ + { + "name": "property:readonly" + }, + { + "name": "capabilities", + "x-uses": [ + "xrn:firebolt:capability:actions:intent" + ] + } + ], + "params": [], + "result": { + "name": "intent", + "summary": "The current intent.", + "schema": { + "type": "string" + } + }, + "examples": [ + { + "name": "Get the current intent", + "result": { + "name": "Default Result", + "value": "launch" + } + } + ] + }, + { + "name": "Actions.onIntent", + "tags": [ + { + "name": "event", + "x-notifier": "Actions.onIntent", + "x-subscriber-for": "Actions.intent" + }, + { + "name": "capabilities", + "x-uses": [ + "xrn:firebolt:capability:actions:intent" + ] + } + ], + "summary": "Notifies when the current intent changes.", + "params": [ + { + "name": "listen", + "schema": { + "type": "boolean" + } + } + ], + "result": { + "name": "intent", + "summary": "The current intent.", + "schema": { + "type": "string" + } + }, + "examples": [ + { + "name": "Listen for intent changes", + "params": [ + { + "name": "listen", + "value": true + } + ], + "result": { + "name": "Default Result", + "value": "launch" + } + } + ] + }, { "name": "Accessibility.audioDescription", "summary": "Returns the audio description setting of the device", diff --git a/include/firebolt/actions.h b/include/firebolt/actions.h index 9c39198..f76efbd 100644 --- a/include/firebolt/actions.h +++ b/include/firebolt/actions.h @@ -22,34 +22,31 @@ #ifndef FIREBOLT_ACTIONS_H #define FIREBOLT_ACTIONS_H -#include #include #include #include #include #include #include +#include -namespace Firebolt::Actions -{ +namespace Firebolt::Actions { -class IActions -{ +class IActions { public: virtual ~IActions() = default; - virtual Result intent(const std::string& intent) const = 0; + virtual Result intent() const = 0; virtual Result subscribeOnIntent(std::function&& notification) = 0; - virtual Result subscribeOnIntentChanged(std::function&& notification) - { + virtual Result subscribeOnIntentChanged(std::function&& notification) { return subscribeOnIntent(std::move(notification)); } virtual Result unsubscribe(SubscriptionId id) = 0; virtual void unsubscribeAll() = 0; -}; // class IActions +}; // class IActions } // namespace Firebolt::Actions diff --git a/run-component-tests-local.sh b/run-component-tests-local.sh new file mode 100755 index 0000000..1752276 --- /dev/null +++ b/run-component-tests-local.sh @@ -0,0 +1,151 @@ +#!/usr/bin/env bash + +set -euo pipefail + +ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +PROTOCOL="rpc_v2" +MOCK_DIR="/tmp/mock-firebolt-firebolt-cpp-client" +IMAGE_TAG="firebolt-client-ci:local" +SKIP_IMAGE_BUILD="false" + +usage() { + cat < RPC protocol (default: rpc_v2) + --mock-dir Host directory for mock-firebolt cache + (default: /tmp/mock-firebolt-firebolt-cpp-client) + --image-tag Docker image tag to use/build + (default: firebolt-client-ci:local) + --skip-image-build Reuse existing image tag; do not run docker build + --help Show this help + +Examples: + ./run-component-tests-local.sh + ./run-component-tests-local.sh --protocol legacy + ./run-component-tests-local.sh --skip-image-build --image-tag firebolt-client-ci:local +EOF +} + +while [[ $# -gt 0 ]]; do + case "$1" in + --protocol) + [[ $# -ge 2 ]] || { echo "Missing value for --protocol" >&2; exit 1; } + PROTOCOL="$2" + shift + ;; + --mock-dir) + [[ $# -ge 2 ]] || { echo "Missing value for --mock-dir" >&2; exit 1; } + MOCK_DIR="$2" + shift + ;; + --image-tag) + [[ $# -ge 2 ]] || { echo "Missing value for --image-tag" >&2; exit 1; } + IMAGE_TAG="$2" + shift + ;; + --skip-image-build) + SKIP_IMAGE_BUILD="true" + ;; + --help|-h) + usage + exit 0 + ;; + *) + echo "Unknown option: $1" >&2 + usage + exit 1 + ;; + esac + shift +done + +if [[ "$PROTOCOL" != "rpc_v2" && "$PROTOCOL" != "legacy" ]]; then + echo "Invalid --protocol '$PROTOCOL'. Expected 'rpc_v2' or 'legacy'." >&2 + exit 1 +fi + +cd "$ROOT_DIR" + +if [[ ! -f .transport.version ]]; then + echo "Missing .transport.version in $ROOT_DIR" >&2 + exit 1 +fi + +TRANSPORT_VERSION="$(cat .transport.version)" +MOCK_SHA1SUM="1fec7b75190e75ac8ea8ebf9f3e00c0a070b2566" +MOCK_BRANCH="topic/changes-for-bidirectional" +NODE_VERSION="24.11.0" + +mkdir -p "$MOCK_DIR" + +echo "[1/4] Docker image: $IMAGE_TAG" +if [[ "$SKIP_IMAGE_BUILD" == "false" ]]; then + docker build \ + -f "$ROOT_DIR/.github/Dockerfile" \ + -t "$IMAGE_TAG" \ + --build-arg "DEPS_TRANSPORT_V=$TRANSPORT_VERSION" \ + --build-arg "DEPS_TRANSPORT_PROTOCOL=$PROTOCOL" \ + "$ROOT_DIR" +else + echo "Skipping image build (--skip-image-build)" +fi + +echo "[2/4] Preparing mock-firebolt in $MOCK_DIR" +docker run --rm --user "$(id -u):$(id -g)" \ + -v "$ROOT_DIR:/workspace" \ + -v "$MOCK_DIR:/mock-host" \ + "$IMAGE_TAG" \ + bash -c ' + set -e + if [ ! -d /mock-host/.git ]; then + git clone --depth 1 --branch '"$MOCK_BRANCH"' \ + https://github.com/rdkcentral/mock-firebolt.git /mock-host + fi + cd /mock-host + git fetch --depth 1 origin '"$MOCK_SHA1SUM"' + git -c advice.detachedHead=false checkout '"$MOCK_SHA1SUM"' + source /usr/local/nvm/nvm.sh + nvm use --delete-prefix '"$NODE_VERSION"' >/dev/null + cd server + npm ci + ' + +BUILD_SUBDIR="build-docker" + +echo "[3/4] Building ctApp (build dir: $BUILD_SUBDIR)" +docker run --rm --user "$(id -u):$(id -g)" \ + -v "$ROOT_DIR:/workspace" \ + "$IMAGE_TAG" \ + bash -c ' + set -e + cd /workspace + rm -rf '"$BUILD_SUBDIR"' + cmake -B '"$BUILD_SUBDIR"' -S . -DCMAKE_BUILD_TYPE=Debug -DENABLE_TESTS=ON + cp docs/openrpc/the-spec/firebolt-open-rpc.json '"$BUILD_SUBDIR"'/test/ + cmake --build '"$BUILD_SUBDIR"' --parallel + chmod +x '"$BUILD_SUBDIR"'/test/ctApp + ' + +echo "[4/4] Running component tests" +docker run --rm --user "$(id -u):$(id -g)" \ + -v "$ROOT_DIR:/workspace" \ + -v "$MOCK_DIR:/mock" \ + "$IMAGE_TAG" \ + /workspace/.github/scripts/run-component-tests.sh \ + --mock /mock \ + --protocol "$PROTOCOL" \ + --config /workspace/.github/mock-firebolt/config.json \ + --openrpc /workspace/docs/openrpc/the-spec/firebolt-open-rpc.json \ + --app-openrpc /workspace/docs/openrpc/the-spec/firebolt-app-open-rpc.json \ + --test-exe /workspace/"$BUILD_SUBDIR"/test/ctApp + +echo "Done." diff --git a/src/actions_impl.cpp b/src/actions_impl.cpp index 431496f..aba7b5b 100644 --- a/src/actions_impl.cpp +++ b/src/actions_impl.cpp @@ -22,36 +22,27 @@ #include "actions_impl.h" #include "json_types/actions.h" #include -#include -namespace Firebolt::Actions -{ +namespace Firebolt::Actions { ActionsImpl::ActionsImpl(Firebolt::Helpers::IHelper& helper) - : helper_(helper), - subscriptionManager_(helper, this) -{ -} + : helper_(helper) + , subscriptionManager_(helper, this) +{} -Result ActionsImpl::intent(const std::string& intent) const -{ - nlohmann::json params; - params["intent"] = intent; - return helper_.invoke("Actions.intent", params); +Result ActionsImpl::intent() const { + return helper_.get("Actions.intent"); } -Result ActionsImpl::subscribeOnIntent(std::function&& notification) -{ +Result ActionsImpl::subscribeOnIntent(std::function&& notification) { return subscriptionManager_.subscribe("Actions.onIntent", std::move(notification)); } -Result ActionsImpl::unsubscribe(SubscriptionId id) -{ +Result ActionsImpl::unsubscribe(SubscriptionId id) { return subscriptionManager_.unsubscribe(id); } -void ActionsImpl::unsubscribeAll() -{ +void ActionsImpl::unsubscribeAll() { subscriptionManager_.unsubscribeAll(); } diff --git a/src/actions_impl.h b/src/actions_impl.h index e7eaf61..229b60d 100644 --- a/src/actions_impl.h +++ b/src/actions_impl.h @@ -25,18 +25,16 @@ #include "firebolt/actions.h" #include -namespace Firebolt::Actions -{ +namespace Firebolt::Actions { -class ActionsImpl : public IActions -{ +class ActionsImpl : public IActions { public: explicit ActionsImpl(Firebolt::Helpers::IHelper& helper); ActionsImpl(const ActionsImpl&) = delete; ActionsImpl& operator=(const ActionsImpl&) = delete; ~ActionsImpl() override = default; - Result intent(const std::string& intent) const override; + Result intent() const override; Result subscribeOnIntent(std::function&& notification) override; diff --git a/src/json_types/actions.h b/src/json_types/actions.h index 60c76ce..6025350 100644 --- a/src/json_types/actions.h +++ b/src/json_types/actions.h @@ -23,16 +23,14 @@ #define FIREBOLT_ACTIONS_JSON_H #pragma once -#include "firebolt/actions.h" -#include -#include #include +#include +#include +#include "firebolt/actions.h" -namespace Firebolt::Actions -{ +namespace Firebolt::Actions { -namespace JsonData -{ +namespace JsonData { } // namespace JsonData diff --git a/test/component/actionsGeneratedTest.cpp b/test/component/actionsGeneratedTest.cpp index 6ce5e5d..e14323e 100644 --- a/test/component/actionsGeneratedTest.cpp +++ b/test/component/actionsGeneratedTest.cpp @@ -18,7 +18,9 @@ #include "firebolt/firebolt.h" #include "utils.h" +#include #include +#include class ActionsGeneratedCTest : public ::testing::Test { @@ -32,8 +34,9 @@ class ActionsGeneratedCTest : public ::testing::Test TEST_F(ActionsGeneratedCTest, Intent) { - auto result = Firebolt::IFireboltAccessor::Instance().ActionsInterface().intent("launch"); - EXPECT_TRUE(result) << toError(result); + auto result = Firebolt::IFireboltAccessor::Instance().ActionsInterface().intent(); + ASSERT_TRUE(result) << toError(result); + EXPECT_EQ(*result, "launch"); } TEST_F(ActionsGeneratedCTest, SubscribeOnIntent) @@ -52,9 +55,10 @@ TEST_F(ActionsGeneratedCTest, SubscribeOnIntent) ASSERT_TRUE(id) << toError(id); verifyEventSubscription(id); - triggerEvent("Actions.onIntent", R"({"value":"launch"})"); + triggerEvent("Actions.onIntent", R"("launch")"); verifyEventReceived(mtx, cv, eventReceived); auto result = Firebolt::IFireboltAccessor::Instance().ActionsInterface().unsubscribe(id.value()); verifyUnsubscribeResult(result); } + diff --git a/test/unit/actionsGeneratedTest.cpp b/test/unit/actionsGeneratedTest.cpp index 86006f3..51b3ee3 100644 --- a/test/unit/actionsGeneratedTest.cpp +++ b/test/unit/actionsGeneratedTest.cpp @@ -16,8 +16,8 @@ * SPDX-License-Identifier: Apache-2.0 */ -#include "actions_impl.h" #include "mock_helper.h" +#include "actions_impl.h" #include class ActionsGeneratedUTest : public ::testing::Test @@ -34,23 +34,24 @@ TEST_F(ActionsGeneratedUTest, Constructs) TEST_F(ActionsGeneratedUTest, UnsubscribeForwardsToHelper) { - EXPECT_CALL(mockHelper, unsubscribe(7)).WillOnce(::testing::Return(Firebolt::Result{Firebolt::Error::None})); + EXPECT_CALL(mockHelper, unsubscribe(7)) + .WillOnce(::testing::Return(Firebolt::Result{Firebolt::Error::None})); auto result = impl.unsubscribe(7); ASSERT_TRUE(result) << "unsubscribe should return success when helper succeeds"; } -TEST_F(ActionsGeneratedUTest, ForwardsintentTransportErrors) + +TEST_F(ActionsGeneratedUTest, ForwardsIntentTransportErrors) { - EXPECT_CALL(mockHelper, invoke("Actions.intent", ::testing::_)) - .WillOnce(::testing::Invoke( - [](const std::string& /*method*/, const nlohmann::json& params) - { - EXPECT_TRUE(params.is_object()) << "Expected params object for method call"; - EXPECT_TRUE(params.contains("intent")) << "Missing expected param key: intent"; - return Firebolt::Result{Firebolt::Error::General}; - })); - - auto result = impl.intent({}); - EXPECT_FALSE(result) << "Expected error propagation when helper invoke fails"; + EXPECT_CALL(mockHelper, getJson("Actions.intent", ::testing::_)) + .WillOnce(::testing::Invoke([](const std::string& /*method*/, const nlohmann::json& params) { + EXPECT_TRUE(params.is_null() || (params.is_object() && params.empty())) + << "Actions.intent getter should not send request params"; + return Firebolt::Result{Firebolt::Error::General}; + })); + + auto result = impl.intent(); + EXPECT_FALSE(result) << "Expected error propagation when helper getJson fails"; } + diff --git a/test/unit/actionsTest.cpp b/test/unit/actionsTest.cpp index 7295886..1b87480 100644 --- a/test/unit/actionsTest.cpp +++ b/test/unit/actionsTest.cpp @@ -28,14 +28,11 @@ class ActionsUTest : public ::testing::Test, protected MockBase TEST_F(ActionsUTest, Start) { - nlohmann::json expectedParams; - expectedParams["intent"] = "launch"; - EXPECT_CALL(mockHelper, invoke("Actions.intent", expectedParams)) - .WillOnce(Invoke([&](const std::string& /*methodName*/, const nlohmann::json& /*parameters*/) - { return Firebolt::Result{Firebolt::Error::None}; })); - - auto result = actionsImpl_.intent("launch"); - EXPECT_TRUE(result); + mock_with_response("Actions.intent", "launch"); + + auto result = actionsImpl_.intent(); + ASSERT_TRUE(result) << "ActionsImpl::intent() returned an error"; + EXPECT_EQ(*result, "launch"); } TEST_F(ActionsUTest, SubscribeOnIntent)