RDKEMW-13945: Fix L2 Issue on RFC Component#182
Conversation
There was a problem hiding this comment.
Pull request overview
Enables the RFC WebPA functional test in the L2 test runner script to address the reported L2 gap for the RFC component.
Changes:
- Uncomments/enables execution of
test_rfc_webpa.pyin the L2 test run.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
run_l2.sh
Outdated
| pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_override_rfc_prop.json test/functional-tests/tests/test_rfc_override_rfc_prop.py | ||
|
|
||
| #pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_rfc_webpa.json test/functional-tests/tests/test_rfc_webpa.py | ||
| pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_rfc_webpa.json test/functional-tests/tests/test_rfc_webpa.py |
There was a problem hiding this comment.
The report filename rfc_rfc_webpa.json is inconsistent with the other report files in this script (which follow rfc_<test_name>.json). Consider renaming it to rfc_webpa.json (or another single-rfc_ prefix) to keep report naming consistent and avoid confusion/accidental mismatches in any report aggregation.
| pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_rfc_webpa.json test/functional-tests/tests/test_rfc_webpa.py | |
| pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_webpa.json test/functional-tests/tests/test_rfc_webpa.py |
49394a7 to
379c655
Compare
379c655 to
5a03df1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_WebPA_Set_ACC_Id(): | ||
| print("Starting parodus mock process") | ||
| payload = '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.AccountInfo.AccountID","dataType":0,"value":"412370664406228514"}]}' | ||
| payload = '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_FirmwareToDownload","dataType":0,"value":" TESTIMAGE_DEV.bin"}]}' |
There was a problem hiding this comment.
test_WebPA_Set_ACC_Id now sends a SET for Device.DeviceInfo.X_RDKCENTRAL-COM_FirmwareToDownload, while the paired GET test still queries ...RFC.Feature.AccountInfo.AccountID. As written, these tests no longer validate a consistent WebPA set/get flow and the test name is misleading; update the SET to target AccountID (as before) or rename/update the GET assertions to match the parameter being set.
| payload = '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_FirmwareToDownload","dataType":0,"value":" TESTIMAGE_DEV.bin"}]}' | |
| payload = '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.AccountInfo.AccountID","dataType":0,"value":"412370664406228514"}]}' |
| def test_WebPA_Set_ACC_Id(): | ||
| print("Starting parodus mock process") | ||
| payload = '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.AccountInfo.AccountID","dataType":0,"value":"412370664406228514"}]}' | ||
| payload = '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_FirmwareToDownload","dataType":0,"value":" TESTIMAGE_DEV.bin"}]}' |
There was a problem hiding this comment.
The SET payload value contains leading whitespace ("value":" TESTIMAGE_DEV.bin"), which is likely to cause brittle behavior (exact-match comparisons, file lookup, or downstream parsing). If whitespace is not intentionally part of the value being tested, trim/remove it (or make the test explicitly assert that trimming behavior is handled).
| payload = '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_FirmwareToDownload","dataType":0,"value":" TESTIMAGE_DEV.bin"}]}' | |
| payload = '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_FirmwareToDownload","dataType":0,"value":"TESTIMAGE_DEV.bin"}]}' |
run_l2.sh
Outdated
| pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_override_rfc_prop.json test/functional-tests/tests/test_rfc_override_rfc_prop.py | ||
|
|
||
| #pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_rfc_webpa.json test/functional-tests/tests/test_rfc_webpa.py | ||
| ./usr/local/bin/parodus -v |
There was a problem hiding this comment.
./usr/local/bin/parodus -v uses a relative path (from the repo working directory in CI) and will fail unless the script is run from /. This should be an absolute path (/usr/local/bin/parodus) to match the preceding ls -l /usr/local/bin/parodus and the test code.
| ./usr/local/bin/parodus -v | |
| /usr/local/bin/parodus -v |
5a03df1 to
fcfa636
Compare
fcfa636 to
f525338
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_WebPA_Set_ACC_Id(): | ||
| print("Starting parodus mock process") | ||
| payload = '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.AccountInfo.AccountID","dataType":0,"value":"412370664406228514"}]}' | ||
| command = ["/usr/local/bin/parodus", payload] | ||
| payload = '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_FirmwareToDownload","dataType":0,"value":"TESTIMAGE_DEV.bin"}]}' | ||
| command = ["/tmp/parodus", payload] |
There was a problem hiding this comment.
test_WebPA_Set_ACC_Id no longer sets the AccountID parameter (it sets X_RDKCENTRAL-COM_FirmwareToDownload instead), but test_WebPA_Get_ACC_Id still asserts an AccountID value (412370664406228514) is returned/logged. As written, the GET test is no longer consistent/self-contained and is likely to fail depending on prior device state. Update the SET payload + expected value to match the GET assertion, or update the GET test/assertions (and test names) to validate the parameter you actually set here.
| payload = '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_FirmwareToDownload","dataType":0,"value":"TESTIMAGE_DEV.bin"}]}' | ||
| command = ["/tmp/parodus", payload] | ||
|
|
There was a problem hiding this comment.
The test now hard-codes the parodus binary path as /tmp/parodus, which makes it depend on external setup (e.g., run_l2.sh copying the binary first) and can break when this test is executed directly in environments where only /usr/local/bin/parodus exists. Consider resolving the binary via an environment variable with a default, or falling back to /usr/local/bin/parodus when /tmp/parodus is absent.
bd8176f to
fd92d6a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cp ./rfcMgr/gtest/mocks/tr181store.ini /opt/secure/RFC/tr181store.ini | ||
|
|
||
| rbuscli set Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Control.ConfigSetTime uint32 1763118860 | ||
| ls -l /usr/local/bin/parodus |
There was a problem hiding this comment.
ls -l /usr/local/bin/parodus is a debug-only step and doesn't contribute to setting up or running the tests. It adds noise to CI logs and will print an error if the file is missing (even though the script continues). Consider removing it or turning it into an explicit existence check that fails fast with a clear message.
| ls -l /usr/local/bin/parodus | |
| if [ ! -x /usr/local/bin/parodus ]; then | |
| echo "Error: /usr/local/bin/parodus not found or not executable." >&2 | |
| exit 1 | |
| fi |
| def test_WebPA_Get_ACC_Id(): | ||
| print("Starting parodus mock process") | ||
| payload ='{"command":"GET","names":["Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.AccountInfo.AccountID"]}' | ||
| command = ["/usr/local/bin/parodus", payload] | ||
| command = ["/tmp/parodus", payload] | ||
|
|
There was a problem hiding this comment.
Hard-coding the WebPA client path to /tmp/parodus makes this test dependent on external setup (e.g., run_l2.sh copying the binary). When running pytest directly, /tmp/parodus may not exist and the test will fail before exercising RFC/WebPA behavior. Consider using the canonical install location (or an env/configurable path) and/or have the test/fixture prepare the binary.
87283a8 to
7eff2c0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -36,7 +36,7 @@ def test_WebPA_Set_ACC_Id(): | |||
| def test_WebPA_Get_ACC_Id(): | |||
| print("Starting parodus mock process") | |||
| payload ='{"command":"GET","names":["Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.AccountInfo.AccountID"]}' | |||
| command = ["/usr/local/bin/parodus", payload] | |||
| command = ["/tmp/parodus", payload] | |||
|
|
|||
There was a problem hiding this comment.
The parodus binary path is hard-coded as /tmp/parodus in multiple places. Since this is an execution dependency, consider centralizing it (e.g., a constant in rfc_test_helper.py or a fixture/env var) so future path changes don’t require editing multiple tests.
| - name: Enable core dump | ||
| run: ulimit -c unlimited | ||
|
|
There was a problem hiding this comment.
ulimit -c unlimited is executed on the GitHub runner, but the RFC build/tests run inside the native-platform container. This ulimit won’t affect processes inside the container, so it won’t enable core dumps where you need them. Set the ulimit on docker run (e.g., --ulimit core=-1) or run ulimit via docker exec in the same container/session that launches parodus.
.github/workflows/L2-tests.yml
Outdated
|
|
||
| - name: Run test | ||
| run: | | ||
| /usr/local/bin/parodus '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_FirmwareToDownload","dataType":0,"value":"TESTIMAGE_DEV.bin"}]}' |
There was a problem hiding this comment.
The Run test step invokes /usr/local/bin/parodus directly on the GitHub runner, but earlier steps indicate parodus exists inside the native-platform container (and run_l2.sh copies it to /tmp/parodus inside that container). This step will not exercise the containerized environment and is likely to fail if the runner doesn’t have that binary. Run the command via docker exec native-platform ... (and use the same binary path you prepared).
| /usr/local/bin/parodus '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_FirmwareToDownload","dataType":0,"value":"TESTIMAGE_DEV.bin"}]}' | |
| docker exec native-platform /tmp/parodus '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_FirmwareToDownload","dataType":0,"value":"TESTIMAGE_DEV.bin"}]}' |
| - name: Print stacktrace | ||
| if: failure() | ||
| run: | | ||
| core=$(ls core* 2>/dev/null | head -n1) | ||
| if [ -n "$core" ]; then | ||
| gdb -batch -ex "bt full" /tmp/parodus $core | ||
| fi |
There was a problem hiding this comment.
The stacktrace step looks for core* files on the GitHub runner and runs gdb ... /tmp/parodus $core, but /tmp/parodus is created inside the native-platform container (per run_l2.sh) and any core dump would also be inside that container’s filesystem. Additionally, this workflow doesn’t install gdb, so the step may fail even if a core exists. To make this work reliably, collect the core/binary from the container (e.g., docker cp) and either run gdb in the container or install gdb on the runner before invoking it.
| pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_xconf_reboot.json test/functional-tests/tests/test_rfc_xconf_reboot.py | ||
|
|
||
| pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_override_rfc_prop.json test/functional-tests/tests/test_rfc_override_rfc_prop.py | ||
| cp /usr/local/bin/parodus /tmp/parodus |
There was a problem hiding this comment.
Copying the parodus binary directly to /tmp/parodus introduces a classic symlink attack risk if this script runs with elevated privileges. An unprivileged user can create /tmp/parodus as a symlink to a sensitive root-owned file, causing cp to overwrite that target when the script runs, leading to privilege escalation or corruption of critical system files. Use a secure temporary path (e.g., via mktemp) and ensure the destination is not a pre-existing symlink in a world-writable directory.
| cp /usr/local/bin/parodus /tmp/parodus | |
| PARODUS_TMP="$(mktemp /tmp/parodus.XXXXXX)" | |
| cp /usr/local/bin/parodus "$PARODUS_TMP" | |
| echo "parodus binary copied to temporary path: $PARODUS_TMP" |
7eff2c0 to
6dd49ca
Compare
6dd49ca to
a4b7d8e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
test/functional-tests/tests/test_rfc_webpa.py:51
test_WebPA_Get_ACC_Idasserts the AccountID value412370664406228514is present in parodus logs, but this file no longer sets that parameter (the SET test now sets FirmwareToDownload). As-is, this test will be order-dependent or fail when run in isolation. Either restore the AccountID SET in this suite or adjust the GET/assertions to match what the SET test configures.
def test_WebPA_Get_ACC_Id():
print("Starting parodus mock process")
payload ='{"command":"GET","names":["Device.DeviceInfo.X_RDKCENTRAL-COM_RFC.Feature.AccountInfo.AccountID"]}'
command = ["/tmp/parodus", payload]
result = subprocess.run(command, capture_output=True, text=True)
assert result.returncode == 0, f"Command failed with error: {result.stderr}"
STATUS_CODE_MSG = '"statusCode":200'
assert STATUS_CODE_MSG in grep_paroduslogs(STATUS_CODE_MSG)
SUCCESS_STATUS_MSG = '"message":"Success"'
assert SUCCESS_STATUS_MSG in grep_paroduslogs(SUCCESS_STATUS_MSG)
ACC_ID_MSG = '"value":"412370664406228514"'
assert ACC_ID_MSG in grep_paroduslogs(ACC_ID_MSG)
run_l2.sh:34
run_l2.shno longer executes any of the L2 pytest suites (all invocations were removed), so the workflow will produce no JSON reports under$RESULT_DIRand may cause the result upload step to fail or silently skip coverage. Please restore the L2 test execution (or gate the debugging-only behavior behind a flag) so CI still runs the intended test set.
RESULT_DIR="/tmp/rfc_test_report"
mkdir -p "$RESULT_DIR"
cp ./rfc.properties /opt/rfc.properties
cp /opt/certs/client.pem /etc/ssl/certs/client.pem
cp ./rfcMgr/gtest/mocks/tr181store.ini /opt/secure/RFC/tr181store.ini
ls -l /usr/local/bin/parodus
cp /usr/local/bin/parodus /tmp/parodus
#pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_rfc_webpa.json test/functional-tests/tests/test_rfc_webpa.py
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/L2-tests.yml
Outdated
| run: | | ||
| pwd | ||
| ls -l /usr/local/bin | ||
| file /usr/local/bin/parodus | ||
| ldd /usr/local/bin/parodus || true |
There was a problem hiding this comment.
The Debug parodus binary step has invalid YAML indentation: run: is indented under - name: instead of aligned with it. This will prevent the workflow from parsing. Fix the indentation so run is at the same level as name for that step.
| run: | | |
| pwd | |
| ls -l /usr/local/bin | |
| file /usr/local/bin/parodus | |
| ldd /usr/local/bin/parodus || true | |
| run: | | |
| pwd | |
| ls -l /usr/local/bin | |
| file /usr/local/bin/parodus | |
| ldd /usr/local/bin/parodus || true |
a4b7d8e to
ffa092b
Compare
ffa092b to
dda9df5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
run_l2.sh:34
run_l2.shno longer runs any of the L2 pytest suites (the entire block invoking multiplepytest --json-report ...commands was removed). As a result, the workflow step that callssh run_l2.shwill stop producing L2 test results, which looks like an unintended functional regression. Reintroduce the test execution block and keep any parodus-copy/debug steps in addition to (not instead of) running the L2 tests.
ls -l /usr/local/bin/parodus
cp /usr/local/bin/parodus /tmp/parodus
#pytest --json-report --json-report-summary --json-report-file $RESULT_DIR/rfc_rfc_webpa.json test/functional-tests/tests/test_rfc_webpa.py
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Enable core dump | ||
| run: ulimit -c unlimited | ||
|
|
||
| - name: Debug parodus binary | ||
| run: | | ||
| pwd | ||
| ls -l /usr/local/bin | ||
| file /usr/local/bin/parodus | ||
| ldd /usr/local/bin/parodus || true | ||
|
|
||
| - name: Run test | ||
| run: | | ||
| cd /usr/local/bin | ||
| ./parodus '{"command":"SET","parameters":[{"name":"Device.DeviceInfo.X_RDKCENTRAL-COM_FirmwareToDownload","dataType":0,"value":"TESTIMAGE_DEV.bin"}]}' | ||
|
|
There was a problem hiding this comment.
The new "Enable core dump", "Debug parodus binary", and "Run test" steps run on the GitHub Actions runner host, not inside the native-platform container where RFC/parodus is built/executed. This will likely fail (runner may not have /usr/local/bin/parodus) and ulimit -c unlimited in its own step won’t affect other steps or the container. Run these diagnostics via docker exec native-platform ... (and set ulimit in the same container-executed command where parodus runs).
No description provided.