Coder Social home page Coder Social logo

rdkservices's Introduction

RDK Services

RDK services are a set of JSON-RPC based RESTful services for accessing various set-top box components. RDK Services are managed and accessed through the Thunder framework. Thunder supports both HTTP and Websocket requests, making the services easily accessible to Lightning, Web, and native client applications.

View Latest Documentation

Table of Contents

Contributing to RDKServices
Comcast CI/CD
Documentation
Coding Guidelines
Versioning
Code Review Checklist
Questions?

Contributing to RDKServices

License Requirements

  1. Before RDK accepts your code into the project you must sign the RDK Contributor License Agreement (CLA).

  2. Each new file should include the latest RDKM license header.

  3. License for this project is included in the root directory and there shouldn't be any additional license file in any of the subfolders.

How to contribute?

  1. Fork the repository, commit your changes, build and test it in at least one approved test platform/device.

  2. To test it in a RDKV device, update SRC_URI and SRCREV in the rdkservices_git.bb recipe to point to your fork.

  3. Submit your changes as a pull request to the latest sprint branch.

  4. If more than one developer has to work on a particular feature, request for a dev branch to be created.

Pull request Checklist

  1. When a pull request is submitted, blackduck, copyright and cla checks will automatically be triggered. Ensure these checks have passed (turned into green).

  2. At least one reviewer needs to review and approve the pull request.

  3. For tracking and release management purposes, each pull request and all the commits in the pull request shall include RDK ticket number(s) or Github issue number(s) and “reason for the change”.

  4. Any pull request from Comcast developers should include a link to successful gerrit verification (in the comment section).

  5. To verify your changeset in gerrit, submit a test gerrit change to rdkservices_git.bb with the SRC_URI and SRCREV pointing to your fork.

  6. If the changes to RDKServices require any Thunder framework changes, the contributor has to plan for a limited regression testing (with the Thunder and RDKServices changes) before submitting the pull request.

Comcast CI/CD

RDKServices branches - sprint vs main (Specific to RDKV Builds)

  1. Comcast gerrit sprint branch will point to rdkservices sprint branch.

    1. For example 2103_sprint branch gerrit recipe will point to sprint/2103 branch in github.
  2. Comcast gerrit stable2 branch will point to rdkservices main branch in github.

Sprint/stable2 git hash updates (Specific to RDKV Builds)

  1. Git hash in rdkservices_git.bb (sprint_branch) will be updated periodically (at least once a week).

    1. Dev owner to follow up as needed and update the JIRA ticket to “ready for sprint testing”.
  2. Once a change is verified in the sprint branch and approved by RM, the developer can contact the maintainers to cherry-pick the change set to the main branch or submit the cherry picked change set to the main branch.

    1. Git hash in rdkservices_git.bb (stable2) will be updated periodically.
    2. Dev owner to follow up as needed and update the ticket to “ready for release testing”.
  3. What if a changeset in the sprint branch fails sprint testing?

    1. The developer has to submit a pull request to undo the commit before the end of the sprint cycle or
    2. The changeset will be abandoned in the sprint branch and won’t make into the main/stable2

Upstream Vs Patch

  1. Patches will increase the chances of build failures when the git hash is moved to a newer version.

  2. We encourage everyone to upstream all the changes to GitHub instead of using patches

  3. On a need basis, a developer can request the maintainers for an approval to use a patch in RDKV build (as a stop-gap measure). An unapproved patch will be rejected.

Documentation

RDK services are described using JSON Schema. JSON Schema provides a standard approach for describing APIs and ensures consistency across all APIs. There are two schemas that are used to describe a service:

Each RDK service has an instance of these schemas in the root of the service folder. For example, MyServicePlugin.json and MyService.json. These files are used to generate API documentation as Markdown. Each service has a Markdown file that is written to the docs/api folder. The following demonstrates the folder structure:

/rdkservices
    /MyService
        /MyService.json
        /MyServicePlugin.json
    /docs/api
        /MyServicePlugin.md

Markdown files are generated from the JSON definitions using the json_generator tool (Tools/json_generator/generator_json.py).

The generator tool requires:

  • Python 3.5 or higher
  • The jsonref library

Verify your Python version:

python --version

Install jsonref if it is not already installed:

pip install jsonref

Generating Markdown for a Single Service

To generate markdown for a single service:

  1. Change directories to Tools/json_generator.

  2. Run generator_json.py and provide the location of the service JSON plugin file using the -d argument and the output directory using the -o argument. You must also include the --no-interfaces-section argument; otherwise, an interface section is added to the markdown that links back to the ThunderInterfaces project. Make certain that you are pointing to the plugin definition and not the interface definition. Here is an example of using the tool:

    python ./generator_json.py -d ../../MyService/MyServicePlugin.json  -o ../docs/api --no-interfaces-section --verbose $files

    The MyServicePlugin.md file is written to the ../docs/api folder. This is the standard directory where all the service API markdown files are written.

Generating Markdown for All Services

A script is provided to generate the markdown for all services and does a complete build of the documentation. The script only generates the markdown for a service if the JSON definition has been updated. In addition, the script post-processes the generated markdown files to create standard link anchors and to clean the build.

To generate markdown for all services:

  1. From the rdkservices repository, change directories to docs/Tools/md_generator.

  2. Run generate_md.py. For example:

    python ./generate_md.py

    All markdown files are written to the ../docs/api folder. This is the standard directory where all the service API markdown files are written.

Use the existing services as a guide when learning the structure of both the plugin and interface schemas.

Coding Guidelines

  1. Style Guide

    The point of having style guidelines is to have a common vocabulary of coding so people can concentrate on what you’re saying rather than on how you’re saying it. If the code you add to a file looks drastically different from the existing code around it, it throws readers out of their rhythm. Please Avoid this. If you’re editing code, take a few minutes to determine the coding style of the component and apply the same style.

    • All RDK Services must have a callsign with a prefix of org.rdk. RDK Service name must use PascalCase (first letter of each word is capitalized). For instance org.rdk.PersistentStore

    • API & Event Names should use camelCase (first word is lowercase & first letter of subsequent words are capitalized). For instance getDeviceInfo, onAppResumed.

    • JSON parameter names in request and responses should use camelCase and be valid ascii strings. For instance "updateAvailable".

    • API getters should start with get. For instance getDefaultInterface. Setters should start with set. For instance setDefaultInterface.

    • Event names should follow the convention on[Noun Object][Action Verb].

      • Object should be a noun and provide useful context on the event.
      • Action should be a verb. Use present and past tenses to indicate if the event is triggered before or after.
      • For instance onAppResumed, onAppResuming.

    • Capitalize acronyms by default. So use setHDMIEnabled instead of setHdmiEnabled.

    • Ensure JSON-RPC API responses return either result or error object on success or error respectively. Please refer to JSON-RPC Spec for more details on expected members in the response object. Highlighting important members below.

      • result
        • This member is REQUIRED on success.
        • This member MUST NOT exist if there was an error invoking the method.
      • error
        • This member is REQUIRED on error.
        • This member MUST NOT exist if there was no error triggered during invocation.
        • The value for this member MUST be an Object as defined below.
          • code
            • A Number that indicates the error type that occurred.
            • This MUST be an integer.
          • message
            • A String providing a short description of the error.
            • The message SHOULD be limited to a concise single sentence.
      • Either the result member or error member MUST be included, but both members MUST NOT be included.
    • To maintain uniformity in all text-editors, set TAB size to 2 or 4 spaces and replace TAB by SPACES.

  2. RDK services are implemented as Thunder Plugins and must adhere to the PluginHost::IPlugin interface. If a RDK service handles WEB requests it implements PluginHost::IWeb. If it activates/deactivates and handles JSON-RPC it implements PluginHost::IDispatcher (or derives from PluginHost::JSONRPC). If it implements custom interfaces it adds them to ThunderInterfaces for RPC. If it exposes interfaces to other processes via RPC it develops an RPC::Communicator. A service specifies its interfaces like this:

    BEGIN_INTERFACE_MAP(MyPlugin)
    INTERFACE_ENTRY(PluginHost::IPlugin)
    INTERFACE_ENTRY(PluginHost::IDispatcher)
    END_INTERFACE_MAP
  1. MODULE_NAME

    • Thunder provides a trace and warning reporting feature. To accurately identify the source of a warning, Thunder needs to know the human readable name of the package (executable or library). This package name is defined by the MODULE_NAME and declared by the MODULE_NAME_DECLARATION()

    • Any package that includes a Thunder component requires such a definition and declaration. If the definition is missing, a compiler error will be reported (error missing MODULE_NAME) and if the declaration is missing, a linker error will be reported (missing or duplicate symbol)

    • Module.h defines the mandatory MODULE_NAME. It also includes framework headers (<plugins/plugins.h>)

      #ifndef MODULE_NAME
      #define MODULE_NAME Plugin_IOController
      #endif
    • Module.cpp defines the mandatory MODULE_NAME_DECLARATION

      #include "Module.h"
      MODULE_NAME_DECLARATION(BUILD_REFERENCE)
  2. A service is registered in a translation unit via a mandatory SERVICE_REGISTRATION(MyService, API_VERSION_NUMBER_MAJOR, API_VERSION_NUMBER_MINOR, API_VERSION_NUMBER_PATCH)

  3. MyPlugin.config can specify: autostart, startuporder, custom properties (passed to the service during activation via PluginHost::IShell::ConfigLine()). During the project configuration, write_config(MyPlugin) in CMakeLists.txt uses MyPlugin.config to generate and install a corresponding json file. Refer to PersistentStore.config

  4. MyPlugin.json documents JSON-RPC interface. It's added to both plugin folder and ThunderInterfaces. The latter generates classes for parameters or enums that can be included like #include <interfaces/json/JsonData_MyPlugin.h>. In the plugin code, JSON-RPC methods and properties are registered like this:

    Register<void /*input params*/, void /*output params*/>(_T("sync"), &MyPlugin::endpoint_sync, this);
    Property<LocationData>(_T("location"), &MyPlugin::get_location /*getter*/, nullptr /*setter*/, this);
  1. Initialization and Cleanup

    • Keep Plugin Constructors & Destructors lean. Most initialization should be done within Initialize() which gets called when the plugin is activated. More on that below. This will ensure that WPEFramework boots up faster since most of the plugins are not auto-started or activated on bootup. Similarly most of the plugin cleanup should happen within Deinitialize() instead of the destructor.

    • Prefer to do Plugin Initialization within IPlugin Initialize(). If there is any error in initialization return non-empty string with useful error information. This will ensure that plugin doesn't get activated and also return this error information to the caller. Ensure that any Initialization done within Initialize() gets cleaned up within IPlugin Deinitialize() which gets called when the plugin is deactivated.

    • Ensure that any std::threads created are joined within Deinitialize() or the destructor to avoid std::terminate exception. Use the ThreadRAII class for creating threads which will ensure that the thread gets joined before destruction.

  2. Inter-plugin communication - There might be use cases where one RDK Service or plugin needs to call APIs in another RDK Service. Don't use JSON-RPC for such communication since it's an overhead and not preferred for inter-plugin communication. JSON-RPC must be used only by applications. Instead use COM RPC through the IShell Interface API QueryInterfaceByCallsign() exposed for each Plugin. Here is an example.

Versioning

  • Versioning

    • Given a version number MAJOR.MINOR.PATCH, increment the:

      • MAJOR version when you make incompatible API changes that break backwards compatibility. This could be removing existing APIs, changes to API Signature or major changes to API behavior that breaks API contract,
      • MINOR version when you add backward compatible new features like adding new APIs, adding new parameters to existing APIs,
      • PATCH version when you make backwards compatible bug fixes.
    • RDK Service version in MAJOR.MINOR.PATCH format is updated using SERVICE_REGISTRATION macro.

    SERVICE_REGISTRATION(DisplaySettings, API_VERSION_NUMBER_MAJOR, API_VERSION_NUMBER_MINOR, API_VERSION_NUMBER_PATCH);
    
    • There is also a Plugin::Metadata structure maintained for each RDK Service that keeps the versioning information. This is returned in call to Controller.1.status.
    static Plugin::Metadata<Plugin::DisplaySettings> metadata(
            // Version (Major, Minor, Patch)
            API_VERSION_NUMBER_MAJOR, API_VERSION_NUMBER_MINOR, API_VERSION_NUMBER_PATCH,
            // Preconditions
            {},
            // Terminations
            {},
            // Controls
            {}
        );
    }
    
    • Changes in version should be updated when commits are added to the main or release branches. There should be a version change per JIRA Ticket. This is not enforced on sprint branches since there could be multiple changes for the same JIRA ticket during development.
  • Changelog

    • Each RDK Service has a CHANGELOG file that contains all changes done so far. When version is updated, add a entry in the CHANGELOG.md at the top with user friendly information on what was changed with the new version. Please don't mention JIRA tickets in CHANGELOG. Refer to Changelog as an example and Keep a Changelog for more details.

    • Please Add entry in the CHANGELOG for each version change and indicate the type of change with these labels:

      • Added for new features.
      • Changed for changes in existing functionality.
      • Deprecated for soon-to-be removed features.
      • Removed for now removed features.
      • Fixed for any bug fixes.
      • Security in case of vulnerabilities.
    • Changes in CHANGELOG should be updated when commits are added to the main or release branches. There should be one CHANGELOG entry per JIRA Ticket. This is not enforced on sprint branches since there could be multiple changes for the same JIRA ticket during development.

  • Deprecation

    • Breaking changes to the API that requires a major version update should first go through Deprecation by doing a minor version update. We recommend atleast 2 RDK releases with the deprecated API/s and minor version update to give time for clients and apps to make changes to remove the deprecated API. Following needs to be done for deprecation.
      • The API/s getting deprecated should be marked with a "deprecated" label in the json schema. This will ensure that it's updated in the API documentation.
      • Add a changelog entry with minor version update and include Deprecated label to call out the API/s getting deprecated.
      • If this API/s is getting replaced by a newer API then it can come in the same minor version update with changelog entry with Added label.

Code Review Checklist

This checklist is primarily intended for maintainers or reviewers. Please check for the following before approving Pull Requests.

  • Coding Guidelines are followed.
  • API Changes are documented and versioned.
  • JIRA Ticket Number is mentioned in the title or commit message for tracking.
  • Any Dependent HAL changes are to be merged first before merging changes to RDK Service to avoid build issues.
  • For a New RDK Service, ensure autostart flag is set to false. The general recommendation is for RDK Services to not be autostarted or activated unless it's required on bootup. Resident Apps can activate the required RDK Services on demand.
  • Approve Pull Requests to main branch or release branches (release/*) only after you get Release Management approval for those specific branches.

Questions?

If you have any questions or concerns reach out to Anand Kandasamy

For a plugin specific question, maintainers might refer you to the plugin owner(s).

rdkservices's People

Stargazers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

Watchers

 avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar  avatar

rdkservices's Issues

Revisit the WebKitBrowser implementation to avoid code duplication.

This is pending on completion of:
rdkcentral/Thunder#217
rdkcentral/Thunder#216

Move the logging functionality to the core:
https://github.com/rdkcentral/rdkservices/pull/162/files#diff-f733dc05f828d3910e543bf2e77f7961R265-R289
In the Portability.h -> DumpCallstack()
Leave the killing of this process up to the Thunder framework:
https://github.com/rdkcentral/rdkservices/pull/162/files#diff-f733dc05f828d3910e543bf2e77f7961R608-R614
Do this by calling IShell->Deactivate(reason => HANGING/WATCHDOG)

If we than update this, lets also remove the thread (saves resources again ;-) by turning this into a Job (https://github.com/rdkcentral/Thunder/blob/master/Source/core/WorkerPool.h#L35) and leave it up to the Timer (part of the workerPool) to trigger HangDetector.

The reason for these changes is because we need this functionality also in the Cobalt/Amazon and Netflix application and it is good to avoid code duplication :-)

<platform>.cmake obstruct build

platform.cmake are not actively used.
Definitions are from old projects and not used in this project: USE_SOUND_PLAYER, ENABLE_DCS_BUILD_FLAGS, USE_DEVICE_SETTINGS_SERVICE, HAS_API_DEVICEPROVISIONING, ENABLE_SYSTEM_17 etc.

Some plugins are hard coded per platform: add_definitions (-DPLUGIN_WAREHOUSE). This makes us unable to turn them on/off from a bitbake recipe. Also dependencies for such plugins are not built.

USE_IARM* are hard coded per platform and conflict with find_package(IARMBus).

USE_DS are hard coded per platform and conflict with find_package(DS).

Flags are hard coded per platform: add_definitions (-DLOGUPLOAD_BEFORE_DEEPSLEEP) and can conflict with bitbake recipes: EXTRA_OECMAKE_append_skyxione-uk = " -DLOGUPLOAD_BEFORE_DEEPSLEEP=ON"

LocationSync test hangs intermittently

[----------] 4 tests from LocationSyncTest
[ RUN      ] LocationSyncTest.activate_locationchange_location_deactivate
[       OK ] LocationSyncTest.activate_locationchange_location_deactivate (1152 ms)
[ RUN      ] LocationSyncTest.activate_sync_deactivate
Error: The operation was canceled.

JSON stubs

Thunder's JsonGenerator can generate proxy stubs and JSON stubs. This practice is ignored in rdkservices. Only 12 of 52 plugins use JSON stubs.

#include <interfaces/json/JsonData_LocationSync.h>

40 of 52 use a bare JsonObject, and the names are written by hand.

README.md specifies that schemas are used "to generate API documentation as Markdown". However, schemas should also be used to generate JSON stubs, as it's done for the plugins in ThunderInterfaces.

APIs take or support String for non-String data

Examples:

"summary": "Delay offset (in ms) on the selected audio port",
"type": "string",
"example": "0"
"summary": "The over temperature grace interval",
"type": "string",
"example": "600"

Functions getNumberParameter, getBoolParameter, etc:

#define getNumberParameter(paramName, param) { \
if (WPEFramework::Core::JSON::Variant::type::NUMBER == parameters[paramName].Content()) \
param = parameters[paramName].Number(); \
else \
try { param = std::stoi( parameters[paramName].String()); } \
catch (...) { param = 0; } \
}
encourage converting anything to int, bool, etc. This conflicts with schema which specifies a fixed type. Passing wrong data won't give an error. "bla bla" is interpreted as 0. "true" or "1" is true:
param = parameters[paramName].String() == "true" || parameters[paramName].String() == "1"; \

Another side effect is inadvertent try/catch enforce to build with -fexceptions:

set_source_files_properties(SystemServices.cpp thermonitor.cpp platformcaps/platformcapsdata.cpp PROPERTIES COMPILE_FLAGS "-fexceptions")

SystemServices constructor does some initialization as well

Plugins should initialize in IPlugin Initialize(). This is reflected in the repo readme.

SystemServices constructor makes a call to setMode which makes a call to IARM_Bus_Call. However, IARM is initialized in SystemServices::Initialize which is called after the constructor. SystemServices constructor relies on IARM being already initialized by other plugins. This isn't causing any issue but it might if someone configures SystemServices to run out of process.

This was found when writing unit tests for SystemServices.

Remove copy-paste patterns in unit tests

* impl; is declared in 23 classes. Move it to a template class e.g. IMockable.
... .impl = &...Mock; occurs in 100+ places. Move it to a mock constructor with IMockable param.
Almost every test case defines:

    Core::ProxyType<Plugin::...> plugin;
    Core::JSONRPC::Handler& handler;
    Core::JSONRPC::Connection connection;
    string response;

Move this to a template class and derive test cases from it.
FactoriesImplementation and PluginHost::IFactories::Assign occurs in 20 files. Move it to a class and derive test cases from it.
Etc.

Inter-plugin communication

Some RDK services use JSONRPC::LinkType for inter-plugin communication - DisplaySettings, MaintenanceManager, RDKShell, ScreenCapture, SystemServices, DeviceProvisioning, EasterEgg.

JSON-RPC is a web socket with a security token and JSON. It's used for JS-C++, or Shell-C++. Thunder exports JSONRPC::LinkType for C++ apps, however, some RDK services use it, even for inter-thread comm. in the same plugin:

uint32_t status = thunderController->Invoke(RDKSHELL_THUNDER_TIMEOUT, api.c_str(), apiRequest.mRequest, joResult);

Plugin querying via an interface - IShell::QueryInterface (same process), or IShell::Root (same process or inter-process) - are better solutions for inter-plugin communication than a web socket connection with JSON.

unit tests without objective

TEST_F(HdmiInputDsTest, writeEDID)
{
EXPECT_EQ(Core::ERROR_NONE, handler.Invoke(connection, _T("writeEDID"), _T("{\"deviceId\": 0, \"message\": \"message\"}"), response));
EXPECT_EQ(response, string("{\"success\":true}"));
}

test passes and coverage good
but if writeEDID implementation is missing, also passes and coverage good

void HdmiInput::writeEDID(int deviceId, std::string message)
{
}

once writeEDID is implemented, test passes and coverage good
once bug appears in writeEDID, passes and good.

it is important to have clear and sufficient expectations

need -I${includedir}/trower-base64/ to build

"trower-base64" is used without cmake find_ commands. target_link_libraries... trower-base64) works as the library is in /usr/lib. But #include "base64.h" does not work as the header is in /usr/include/trower-base64/base64.h. On yocto rdkservices build with a hack -I${includedir}/trower-base64/. The bug is in ContinueWatching and SystemAudioPlayer modules.

How to play base64 encoded wav audio?

We use webkit to play audio, use html5 video tag "data:audio/x-wav;base64,", the platform can't support it.
So is there any other way to play it?

INTERNET / NETWORK preconditions

INTERNET is set by LocationSync and is used for example by WebKitBrowser/Amazon.
NETWORK is used by LocationSync.
Also, in some places like MaintenanceManager, org.rdk.Network.isConnectedToInternet is used instead of preconditions.
Similar is used in 0001-SERXIONE-600-LocationSync-Network-check-timer.patch by LocationSync.
I could not find what sets NETWORK and how it's different from org.rdk.Network.isConnectedToInternet.

WebKitBrowser: How to detect that url was successfully loaded or failed to load?

I tried to implement the way to detect that selected URL has been successfully loaded or failed to load and have problem with that.
There are following callbacks available in the browser plugin (WebKitImplementation.cpp file):

  • uriChangedCallback
  • loadChangedCallback
  • loadFailedCallback

*When a new domain URL load process is initiated, process seems to be straightforward:

url: https://www.live.bbctvapps.co.uk

  1. uriChangedCallback (url: https://www.live.bbctvapps.co.uk)
  2. loadChangedCallback() is being called with following events: WEBKIT_LOAD_STARTED, WEBKIT_LOAD_COMMITTED, WEBKIT_LOAD_FINISHED

URL load detection: wait for loadChangedCallback() with WEBKIT_LOAD_FINISHED event

*When loading url from the same domain (boot url has been already loaded), the only callback which is being called is uriChangedCallback():

boot url: https://widgets.metrological.com/lightning/liberty/2e3c4fc22f0d35e3eb7fdb47eb7d4658#boot
app url: https://widgets.metrological.com/lightning/liberty/2e3c4fc22f0d35e3eb7fdb47eb7d4658#app:com.metrological.app.Euronews

  1. uriChangedCallback (url: https://widgets.metrological.com/lightning/liberty/2e3c4fc22f0d35e3eb7fdb47eb7d4658#app:com.metrological.app.Euronews)

URL load detection: wait for uriChangedCallback() with metrological url

*When loading wrong url from a new domain:

url: https://not_valid_url.com

  1. uriChangedCallback (url: https://not_valid_url.com)
  2. loadChangedCallback (event: WEBKIT_LOAD_STARTED)
  3. uriChangedCallback (url: https://widgets.metrological.com/lightning/liberty/2e3c4fc22f0d35e3eb7fdb47eb7d4658#boot)
  4. loadFailedCallback (url: https://widgets.metrological.com/lightning/liberty/2e3c4fc22f0d35e3eb7fdb47eb7d4658#boot)

URL failed detection: loadFailedCallback() contains previous url, but also there is uriChangedCallback() with metrological url which breaks failure detection

Could you please describe what is the proper way to handle success or failed url load detection?

Plugins should stop threads in Deinitialize

std::thread should be joined by a plugin before the destruction of a plugin. If that happens during the destruction of a plugin a thread can reference the members that have been destructed already and so segfault. I can see it is a common problem when people use ThreadRAII thinking it does everything for them. ThreadRAII as a member variable is not guaranteed to destroy before other member variables. Suggesting to remove ThreadRAII to avoid problems like that.

Unit tests workflow should be fast when invoked locally

Developers need rapid feedback to safely and easily make incremental code changes. Disable (by default) coverage and Valgrind when invoked locally via ACT, to speed up the development of tests/ TDD. When invoked on GitHub, both should remain enabled.

Reboot, systemd and SIGTERM

Hi,

I have seen an issue related to the "reboot" request in System Services. An application on a STB is making this request after encountering a major error, in this case failing to launch the EPG (UI) app.

The application is started and managed by systemd. It expects to receive a SIGTERM signal to do a graceful shutdown. However, no SIGTERM is received by the application after the reboot request. Hence it is unable to do a graceful shutdown before reboot. This can sometimes results in a core dump file.

Can someone tell me if System Services does do a shutdown vis-à-vis systemd? It doesn't appear to be doing that. Otherwise I would expect to receive the SIGTERM signal from systemd.

It should be easy to understand what unit test verifies

Tests have a common flow. A test sets up the conditions, then executes code, and finally verifies that the behavior occurred as expected.
Tests shouldn’t require readers to slowly dig into each line of the test, trying to figure out what’s going on. A reader should readily understand the essential elements of a test’s setup (arrange), what behavior it executes (act), and how that behavior gets verified (assert).
It's known as Arrange-Act-Assert or Setup-Execute-Verify, or Given-When-Then. In other words, Given a context, When the test executes some code, Then some behavior is verified.

TEST_F(ARetweetCollection, IgnoresDuplicateTweetAdded) {
    Tweet tweet("msg", "@user");
    Tweet duplicate(tweet);
    collection.add(tweet);

    collection.add(duplicate);

    ASSERT_THAT(collection.size(), Eq(1u));
}

sleep()-s in code

aside from things like sleep(10); or sleep(60); the following can be often seen:

  • attempts to fix race conditions with sleep()

//give Netflix process some time to terminate gracefully.
sleep(10);

notify(RDKSHELL_EVENT_ON_WILL_DESTROY, params);
sleep(gWillDestroyEventWaitTime);
killAllApps();

sleep(HDMICECSINK_PLUGIN_ACTIVATION_TIME);

sleep(WARMING_UP_TIME_IN_SECONDS);

  • sleep() in a loop (what if reboot/Deinitialize occurs while looping?)

while(tts_request.empty())
{
sleep(60);

do{
network_available = checkNetwork();
if ( !network_available ){
sleep(30);

do{
if ((getServiceState(m_service, "org.rdk.AuthService", state) != Core::ERROR_NONE) || (state != PluginHost::IShell::state::ACTIVATED)) {
sleep(10);

  • sleep() in a loop with no retry count (what if condition never satisfies?)

while(m_currentArcRoutingState != ARC_STATE_ARC_TERMINATED)
{
usleep(500000);
}

  • unclear things like sleep() for 68 ms?

std::cout << "bounds set\n";
usleep(68000);
std::cout << "all set\n";

unit tests are failing

getting failure every time:

| [  FAILED  ] 2 tests, listed below:
| [  FAILED  ] NetworkTest.pingNamedEndpoint
| [  FAILED  ] NetworkTest.ping

someone added tests that use real network instead of mocks

Unit tests should describe the behavior

Deriving highly descriptive test names is important, as tests are read and reread by others who must maintain the code.
A common mistake is to focus on member functions TEST(ARetweetCollection, Add) which results in a mix of different behaviors in a single test.
Test behavior, not methods. A test case name and test name together, left to right, should reveal a sentence that describes what is verified:

TEST(ARetweetCollection, IgnoresDuplicateTweetAdded)
TEST(ARetweetCollection, UsesOriginalTweetTextWhenEmptyTweetAdded)
TEST(ARetweetCollection, ThrowsExceptionWhenUserNotValidForAddedTweet)

A list of test names should reflect the behaviors that the system supports.
When using fixtures, the fixture name should clearly describe the context:

TEST_F(ARetweetCollectionWithOneTweet, IsNoLongerEmpty)
TEST_F(ARetweetCollectionWithOneTweet, HasSizeOfOne)

AbstractPlugin deprecation

AbstractPlugin isn't abstract. It's used to maintain the faulty versioning scheme pointed out in #2284. The issue was acknowledged and closed but not fixed. Readme still says "Plugins must extend the AbstractPlugin helper class." and "Removing or Adding an API should increment the version to the next whole number (2,3,4...).". The majority, 31 of 51 services use AbstractPlugin (7 of them really use versions other than 1). Among CPC services, it's 25 of 29 (only one uses versions other than 1).

Suggest removing AbstractPlugin.

Handling of the unit tests external dependencies

Unit tests should address the rdk services. Unit tests should not address the extrenal code linked as libraries and included via include directive. I.E. such as DS (displaysettings), IARM, and others. At present, the https://github.com/rdkcentral/rdkservices/tree/sprint/22Q4/Tests/headers folder contains the fake headers used by the rdk services. The purpose of those fake headers is to make build possible and make external code mockable. It is observed that people prefer to copy-paste the original headers as is. As a result a considerable part of https://github.com/rdkcentral/rdkservices/tree/sprint/22Q4/Tests/headers folder contains the external code that serves none of the objectives. To avoid that, let's get rid of https://github.com/rdkcentral/rdkservices/tree/sprint/22Q4/Tests/headers folder. It makes sense to generate external headers as empty files by the workflow. The actual definitions (only those which are used) can be squeezed into files like ds.h, iarm.h, and others under the https://github.com/rdkcentral/rdkservices/tree/sprint/22Q4/Tests/mocks folder.

Unit tests should use NiceMock

Unit tests should use NiceMock to address warnings like

| GMOCK WARNING:
| Uninteresting mock function call - taking default action specified at:
| /home/npoltorapavlo/dev/rdkservices/rdkservices/Tests/tests/test_Network.cpp:72:
|     Function call: IARM_Bus_RegisterEventHandler(0x7fe6bf416d87 pointing to "NET_SRV_MGR", 55, 0x7fe6bf3b7e42)
|           Returns: 0
| NOTE: You can safely ignore the above warning unless this call should not happen.  Do not suppress it by blindly adding an EXPECT_CALL() if you don't mean to enforce the call.  See https://github.com/google/googletest/blob/master/googlemock/docs/cook_book.md#knowing-when-to-expect for details.

Ignoring the Google Mock warning is unacceptable
NiceMock allows Google Mock to track interactions only for methods on which expectations exist.

Existing logs produce 3772 GMOCK WARNING and aren't readable.

onApplicationTerminated never fires

With the current implementation, by the time waitpid function returns in westeros compositor, we clear the listener for the corresponding compositor window. This result in never getting the WstClient_stoppedNormal or WstClient_stoppedAbnormal events to rdkcompositor.

As a backup strategy, we intent to send the event after we invoke the compositor kill call.

Lcov processing time has degraded

Generate coverage workflow step takes too long:
4m 16s
3m 15s
3m 32s

Need to exclude uninteresting sources from the processing, for example tests:

Processing Tests/CMakeFiles/RdkServicesTest.dir/tests/test_CountingLock.cpp.gcda
Processing Tests/CMakeFiles/RdkServicesTest.dir/tests/test_LocationSync.cpp.gcda
...

org.rdk....

Observed the following issue.

If the plugin's callsign differs from the name, for example, due to "org.rdk." prefix:

# cat PersistentStore.json 
{
 "locator":"libWPEFrameworkPersistentStore.so",
 "classname":"PersistentStore",
 "precondition":[
  "Platform"
 ],
 "callsign":"org.rdk.PersistentStore",
 "autostart":true,
 "configuration":{
...

then configuration from the json file isn't loaded.

segfaults with "dbusCallHandler" signature

When Iarm event or call handlers are not unregistered and the plugin is unloaded, it can segfault with "dbusCallHandler" signature. Crashes like this with empty stack trace are not possible to triage.

servicemanager legacy code

"success" param in the api result is the legacy code.

In Thunder JSON-RPC there are error codes. If ERROR_NONE the message sends a "result" object. Otherwise the message sends an error code. Error codes are part of the interface:

"description": "User name is already taken (i.e. the user has already joined the room)",
"$ref": "#/common/errors/illegalstate"

A caller shouldn't have to inspect "result" object to see if the api succeeded or not.

"getMeSomeData" methods are also the legacy code.

In Thunder JSON-RPC there are properties / readonly properties:

Property<LocationData>(_T("location"), &LocationSync::get_location, nullptr, this);

Unit tests quality

A proper design of the tests extends their return and keeps them from becoming a maintenance burden.

Deriving highly descriptive test names is important. A test case name and test name together, left to right, should reveal a sentence that describes what is verified - TEST(ASet, IgnoresDuplicateAdded). When using fixtures, the fixture name should clearly describe the context - TEST_F(ASetWithOneItem, HasSizeOfOne).

Tests shouldn't require readers to slowly dig into each line of the test, trying to figure out what's going on. There should be 3 sections, separated by blank lines - setup (Arrange), execute code (Act), and verify that the behavior occurred as expected (Assert). It's known as Arrange-Act-Assert. Irrelevant details can be moved to the fixture or utility function.

A certain level of code coverage should never be the target. Programmers looking to bump up their code coverage numbers quickly figure out that they can write tests without assertions. These nontests are a waste of effort. A more sensible use of code coverage is to identify areas of code that needs more tests.

Tests that can fail for several reasons waste time to pinpoint the cause. A common mistake is to focus on member functions - TEST(ASet, Add) which results in a mix of different behaviors in a single test. One Assert per Test helps to have tests that fail for a specific reason, and drives to focus on one behavior at a time.

Unit means a small piece of logic independent from other parts. If coupled with other parts, it's integration testing. Integration tests are difficult to maintain and should be converted to fast unit tests by removing dependencies.

utils.cpp

Utils violate single responsibility:
https://github.com/rdkcentral/rdkservices/blob/sprint/2201/helpers/utils.h
https://github.com/rdkcentral/rdkservices/blob/sprint/2201/helpers/utils.cpp

and utils.cpp is built more than once (28 times):
https://github.com/rdkcentral/rdkservices/blob/sprint/2201/FrameRate/CMakeLists.txt#L27
https://github.com/rdkcentral/rdkservices/blob/sprint/2201/DataCapture/CMakeLists.txt#L27
...

To address these issues I've started replacing utils with single purpose headers like:
https://github.com/rdkcentral/rdkservices/blob/sprint/2203/helpers/UtilsIarm.h
https://github.com/rdkcentral/rdkservices/blob/sprint/2203/helpers/UtilsLogging.h

For example, DataCapture shared object size was reduced from 200036 to 109188 bytes.

There are still 37 rdkservices that use utils.h (28 build utils.cpp).

Monitor lacks provision to read memory stat at the moment of query

Right now, Monitor plugin is implemented to read memory information from plugins at periodic intervals. But sometimes returning cached value are not desirable. If any component is interested in memory consumption of a plugin (say WebKitBrowser) at the moment, the only option we have is to increase the frequency of memory read.

Having a provision to force read memory stat from plugins would be of help in such scenarios.

Versioning

I think the versioning provided by Thunder:

https://github.com/rdkcentral/Thunder/blob/master/Source/plugins/JSONRPC.h#L116

is for the [rare] case when multiple variants of the same API should coexist. API version (uint8_t: 1, 2, etc) is not the same as the plugin version used for the documentation:

https://github.com/rdkcentral/rdkservices/blob/sprint/2201/FrameRate/FrameRatePlugin.json#L9

(In a similar fashion, when using SQLite library, version "3.38.0" is specified in the library documentation or changelog, but it's not specified when calling "sqlite3_open". If the library has multiple variants of the same API, it can be "sqlite3_open" or "sqlite3_open_v2" with additional parameters. "sqlite3_open_v2" doesn't mean it was added in SQLite "3.2.0", it means there are 2 variants to call this API)

I would expect the same logic in rdkservices but it's different:

For example, "org.rdk.FrameRate.2.setDisplayFrameRate" doesn't mean it differs from "org.rdk.FrameRate.1.setDisplayFrameRate". Instead, "org.rdk.FrameRate.1.setDisplayFrameRate" doesn't exist. On the other hand, "org.rdk.FrameRate.1.updateFps" and "org.rdk.FrameRate.2.updateFps" are the same. This looks like "2" = plugin version.

But in Thunder "2" is part of the method and not the callsign:

https://github.com/rdkcentral/Thunder/blob/master/Source/core/JSONRPC.h#L154

(This makes sense as no version is needed to activate the plugin)

However, the logic in AbstractPlugin:

https://github.com/rdkcentral/rdkservices/blob/sprint/2201/helpers/AbstractPlugin.h

makes it look like "2" = plugin version. This is confusing, because for the user, like an app, there's no difference. When app calls "org.rdk.FrameRate.1.setDisplayFrameRate" or "org.rdk.FrameRate.2.setDisplayFrameRate" and the plugin doesn't have it, the result in both cases will be an error (-32601 "Unknown method." or -32602 "Requested version is not supported."). There's no need for the app to specify that "setDisplayFrameRate" was added in "version 2", or "version 38" in order to just call it.

(As if SQLite adds a new API in version "3.38.1" and calls it "sqlite3_reopen_v3_38_1". At the same time, adds "sqlite3_open_v3_38_1" which doesn't differ from "sqlite3_open" and both can be called. Not sure if this makes sense, but such is the logic in AbstractPlugin)

Suggesting to remove AbstractPlugin and the logic it introduces.

The code of AVInput

// Thunder plugins communication

For me, it's not clear what AVInput does.
It has 2 methods, which use device::HdmiInput. This is clear. However, the rest of the code:

  • starts a timer
  • creates a security token
  • checks if org.rdk.HdmiInput is activated
  • activates org.rdk.HdmiInput
  • listens to org.rdk.HdmiInput events over JSON-RPC

This is not clear as, at a glance, AVInput can operate without org.rdk.HdmiInput. If the authors wanted to get events from org.rdk.HdmiInput, there's an easier way - receive events from IARM directly.

Also, if org.rdk.HdmiInput doesn't exist, AVInput keeps trying to activate it indefinitely and the methods return failure "org.rdk.HdmiInput plugin is not ready". But my understanding is device::HdmiInput functions can be called without need in org.rdk.HdmiInput. There's also nothing in the documentation on why org.rdk.HdmiInput is needed.

I think "Thunder plugins communication" logic of AVInput should be removed. That is, lines 205-403.

How to build?

How do you build the code in this repo? I haven't been able to find anything in the RDK wiki or the README although I may have missed something.

Duplicated web-browser interfaces

This issue is to assess if potential rework is necessary around multiple web-browser interfaces with similar functionality.

IBrowserResources interface with UserScripts / UserStyleSheets methods was included into ThunderInterfaces: rdkcentral/ThunderInterfaces@cccdfaa

A year later a similar interface was also added to IBrowser.h: IBrowserScripting with RunJavaScript / AddUserScript / RemoveAllUserScripts methods:
rdkcentral/ThunderInterfaces@37e1fcf

The two interfaces overlaps in terms of functionality of executing user scripts but they are not exactly the same:

  • IBrowserResources::UserScripts executes scripts pointed by URI (file://)
  • IBrowserScripting::AddUserScript executes script code given as parameter

Please assess if the described functionalities scattered across multiple interfaces can be put in one place.
If I recall correctly any changes in Thunder interfaces can be introduced only when major version of these interfaces gets increased.

Unit testing impediments and TDD

At present we have a number of developers who write tests for everything and usually facing the same problems explained further. Problem 1 (what to test). to write a test need to understand how the code works. plugins can be 5000+ lines of code, average 1000+. every line needs to be analyzed which takes time. need smaller testable modules, or "units". Problem 2 (who should do that). Developers who write tests are not familiar with this code and won't need it in future. Plugin owners can do a better job writing tests for their code. TDD looks like a better approach, as the tests are not added all at once, but along with the respective fix/feature. Problem 3 (how to build). the problem is external dependencies. getting a random plugin to build for tests can be a non-trivial work. This makes TDD complicated even to start. Maybe should change the approach a bit and focus on getting the plugins build. As a starting point for TDD can add one basic test which does nothing else but creating/destroying a plugin instance

Unit tests build time has degraded

Building locally, rebuild after 1 small change in a test takes 3min. This eliminates the advantages of unit tests and should be fixed asap.

use of shell subprocess

In most places there is no need to use system/popen. For example, it's used to:

modify a string:

std::string script = "echo '" + path + "' | sed -r \"s/([^$]*)([$\\{]*)([^$\\{\\}\\/]*)(.*)/\\3/\"";
variable = Utils::cRunScript(script.c_str());

search a string in a file:
script = ". /etc/device.properties; echo \"$" + variable + "\"";
value = Utils::cRunScript(script.c_str());

const char * script1 = R"(grep DEVICE_TYPE /etc/device.properties | cut -d "=" -f2 | tr -d '\n')";
m_isHybridDevice = Utils::cRunScript(script1).substr();

FILE *p = popen("cat /proc/mounts | grep mmcblk0p1 | awk '{print $2}' ", "r");

search files:
std::string result = Utils::cRunScript(script.c_str());

command = "find "+path+" -iname "+filter+" > /tmp/tempBuffer.dat";
retStat = system(command.c_str());

read a file:
string httpCodeStr = Utils::cRunScript("cat /tmp/xconf_httpcode_thunder.txt");

response = Utils::cRunScript("cat /tmp/xconf_response_thunder.txt");

FILE* fp = popen(CAT_DWNLDPROGRESSFILE_AND_GET_INFO, "r");

create a dir:
std::string command = "mkdir -p " + dir + " \0";
Utils::cRunScript(command.c_str());

remove a file:
snprintf(cmd, 127, "rm -f %s", STANDBY_REASON_FILE);
pipe = popen(cmd, "r");

helpers folder

/helpers is like a framework, but it's not a library/libraries. Files from it are just compiled in multiple places. For example, utils.cpp is compiled in 26 places (see issue #2319), SystemServicesHelper.cpp in 2 places, etc.

Also, most of the /helpers' functionality looks like reinventing the wheel. For example, Thunder gives us the logging TRACE, but /helpers "help" to avoid it and use LOGINFO. Utils::fileExists "help" to avoid Core::File(..).Exists(). 2 timer classes "help" to avoid Core::TimerType, etc.

Suggest using the functionality given by the Thunder framework and getting rid of /helpers.

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.