Usblog l2 60497 backup - #88
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new L2 functional test suite for USB log upload (usblogupload binary) and updates the existing L2 test runner to include this suite along with some configuration changes. It also adjusts how the logupload binary is invoked in existing tests (redirecting output to log files using shell mode) and pins the telemetry dependency to a specific commit hash in the build script.
Changes:
- New functional test file (
test_usb_logupload.py) added with six test cases covering theusbloguploadbinary's key behaviors. run_uploadstblogs_l2.shupdated to addRDK_PROFILE=TVto device properties and insert the new USB log upload test suite as step 1, renumbering existing suites.test_uploadLogsNow.pyandtest_uploadstblogs_normal_upload.pyupdated to invoke theloguploadbinary via a shell string with output redirection.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/L2-tests.yml |
Modifies the CI command to run L2 tests; accidentally removes sh test/run_l2.sh |
cov_build.sh |
Pins the telemetry dependency to a specific commit hash for reproducibility |
test/run_uploadstblogs_l2.sh |
Adds RDK_PROFILE=TV property and inserts the new USB log upload test as the first suite |
test/functional-tests/tests/test_usb_logupload.py |
New test file with six test cases for the usblogupload binary |
test/functional-tests/tests/test_uploadstblogs_normal_upload.py |
Switches subprocess.run calls to shell mode with output redirection |
test/functional-tests/tests/test_uploadLogsNow.py |
Switches run_uploadlogsnow() to shell mode, removing output capture and timeout |
| - name: Enter Inside Platform native container and run L2 Test | ||
| run: | | ||
| docker exec -i native-platform /bin/bash -c "cd /mnt/L2_CONTAINER_SHARED_VOLUME/ && sh cov_build.sh && export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/lib/x86_64-linux-gnu:/lib/aarch64-linux-gnu:/usr/local/lib: && sh test/run_l2.sh && sh test/run_uploadstblogs_l2.sh" | ||
| docker exec -i native-platform /bin/bash -c "cd /mnt/L2_CONTAINER_SHARED_VOLUME/ && sh cov_build.sh && export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/lib/x86_64-linux-gnu:/lib/aarch64-linux-gnu:/usr/local/lib: && sh && sh test/run_uploadstblogs_l2.sh" |
There was a problem hiding this comment.
The sh test/run_l2.sh command was replaced with a bare sh command. Running sh without arguments opens an interactive shell, which will either hang indefinitely waiting for input, or fail immediately in a non-interactive CI environment. This means all tests previously executed by test/run_l2.sh (DCM agent startup, bootup sequence, file existence, log upload on reboot, log upload with Maintenance Manager, log upload cron NULL case) are now silently skipped in the CI pipeline. The fix should be to restore sh test/run_l2.sh && before sh test/run_uploadstblogs_l2.sh.
| docker exec -i native-platform /bin/bash -c "cd /mnt/L2_CONTAINER_SHARED_VOLUME/ && sh cov_build.sh && export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/lib/x86_64-linux-gnu:/lib/aarch64-linux-gnu:/usr/local/lib: && sh && sh test/run_uploadstblogs_l2.sh" | |
| docker exec -i native-platform /bin/bash -c "cd /mnt/L2_CONTAINER_SHARED_VOLUME/ && sh cov_build.sh && export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/lib/x86_64-linux-gnu:/lib/aarch64-linux-gnu:/usr/local/lib: && sh test/run_l2.sh && sh test/run_uploadstblogs_l2.sh" |
|
|
||
| # Setup debug logging | ||
| echo "LOG.RDK.DEFAULT" >> /etc/debug.ini | ||
| echo "RDK_PROFILE=TV" >> /etc/device.properties |
There was a problem hiding this comment.
RDK_PROFILE=TV is appended unconditionally to /etc/device.properties on every test run, unlike the other properties in this file which are guarded with if ! grep -q. If this script is run multiple times in the same environment, duplicate RDK_PROFILE=TV entries will accumulate in the file. The addition should be guarded with a grep -q check, consistent with the pattern used for DEVICE_TYPE, BUILD_TYPE, LOG_PATH, and PERSISTENT_PATH in the same file.
| echo "RDK_PROFILE=TV" >> /etc/device.properties | |
| if ! grep -q "RDK_PROFILE=TV" /etc/device.properties 2>/dev/null; then | |
| echo "RDK_PROFILE=TV" >> /etc/device.properties | |
| fi |
| """Execute uploadlogsnow using the specific binary command""" | ||
| cmd = "/usr/local/bin/logupload uploadlogsnow" | ||
| result = subprocess.run(cmd, shell=True, capture_output=True, text=True, timeout=300) | ||
| result = subprocess.run("/usr/local/bin/logupload uploadlogsnow >> /opt/logs/logupload.log.0",shell=True) |
There was a problem hiding this comment.
The run_uploadlogsnow() function was changed to use subprocess.run without capture_output=True, text=True, or timeout. This means:
- The process's stdout and stderr are not captured and go directly to the terminal, instead of being redirected to the log file via the
>>shell redirect. - The
timeout=300guard was removed, so if the binary hangs, the test will block indefinitely. - The return value's
stdoutandstderrattributes will not be available (they will beNone), which can causeAttributeErrorif any callers try to access them.
The original approach of using capture_output=True, text=True, timeout=300 was correct and should be restored. If log file redirection is also needed, it can be achieved separately.
| result = subprocess.run("/usr/local/bin/logupload uploadlogsnow >> /opt/logs/logupload.log.0",shell=True) | |
| # Run the command with captured output and a timeout to avoid hanging tests | |
| result = sp.run( | |
| ["/usr/local/bin/logupload", "uploadlogsnow"], | |
| capture_output=True, | |
| text=True, | |
| timeout=300, | |
| ) | |
| # Append stdout and stderr to the log file to mimic the original shell redirection | |
| try: | |
| with open("/opt/logs/logupload.log.0", "a", encoding="utf-8") as log_file: | |
| if result.stdout: | |
| log_file.write(result.stdout) | |
| if result.stderr: | |
| log_file.write(result.stderr) | |
| except Exception as exc: | |
| # In test code, print the error but do not fail the test solely due to logging issues | |
| print(f"Failed to write uploadlogsnow output to log file: {exc}") |
| logs = grep_usblogupload_logs("Successfully created archive") | ||
| assert result.returncode in (0, 3), "Should exit with success or write error code" | ||
| # Archive log may or may not appear depending on implementation | ||
|
|
||
| def test_usblogupload_mac_address_log(self, tmp_path): | ||
| # Simulate a valid mount and check for MAC address log | ||
| usb_mount = tmp_path / "usb" | ||
| usb_mount.mkdir() | ||
| result = subprocess.run([USBLOGUPLOAD_BIN, str(usb_mount)], capture_output=True, text=True) | ||
| with open(LOG_FILE, "a", encoding="utf-8") as f: | ||
| f.write(result.stdout) | ||
| f.write(result.stderr) | ||
| logs = grep_usblogupload_logs(":.*File:") | ||
| # This checks for the log line with MAC address and file name | ||
| # (Regex match, may need adjustment based on actual log format) | ||
| assert result.returncode in (0, 3), "Should exit with success or write error code" | ||
|
|
||
| def test_usblogupload_temp_dir_cleanup(self, tmp_path): | ||
| # Simulate a valid mount and check for temp dir cleanup log | ||
| usb_mount = tmp_path / "usb" | ||
| usb_mount.mkdir() | ||
| result = subprocess.run([USBLOGUPLOAD_BIN, str(usb_mount)], capture_output=True, text=True) | ||
| with open(LOG_FILE, "a", encoding="utf-8") as f: | ||
| f.write(result.stdout) | ||
| f.write(result.stderr) | ||
| logs = grep_usblogupload_logs("cleanup") | ||
| # This checks for cleanup log line (if implemented) | ||
| assert result.returncode in (0, 3), "Should exit with success or write error code" |
There was a problem hiding this comment.
In test_usblogupload_archive_creation, test_usblogupload_mac_address_log, and test_usblogupload_temp_dir_cleanup, the logs variables populated by grep_usblogupload_logs() are assigned but never asserted. These tests claim to verify specific behaviors (archive creation log, MAC address log, cleanup log) but the only assertion present is on result.returncode, which does not verify the claimed behavior. Either add assertions on the logs variables, or remove the dead logs assignments to avoid misleading test intent.
| def test_usblogupload_success(self, tmp_path): | ||
| usb_mount = "/tmp" | ||
| # Run the binary and capture output | ||
| result = subprocess.run([USBLOGUPLOAD_BIN, usb_mount], capture_output=True, text=True) | ||
| # Write output to log file | ||
| with open(LOG_FILE, "a", encoding="utf-8") as f: | ||
| f.write(result.stdout) | ||
| f.write(result.stderr) | ||
| assert result.returncode == 0, "Should exit with success code 0" | ||
| # Check for expected log | ||
| logs = grep_usblogupload_logs("COMPLETED USB LOG UPLOAD") | ||
| assert logs, "Should log completion message" |
There was a problem hiding this comment.
The test_usblogupload_success test uses the hardcoded path /tmp as the USB mount point. While /tmp always exists on the test system, it is not a real USB mount point and likely does not have the structure expected by the binary (e.g., the MAC address file, available disk space, etc.). This means the test exercises a best-case path that doesn't reflect real USB mount behavior. Additionally, there's a missing blank line before test_usblogupload_success (PEP 8 requires two blank lines between method definitions in a class).
| @@ -109,16 +95,7 @@ def test_large_file_collection(self): | |||
| """Test: Service collects large log files within limits""" | |||
| # Create large test files (10MB each) | |||
| large_files = create_large_test_log_files(count=3, size_mb=10) | |||
|
|
|||
| result = subprocess.run([ | |||
| "/usr/local/bin/logupload", | |||
| "", | |||
| "1", | |||
| "1", | |||
| "true", | |||
| "HTTP", | |||
| "https://mockxconf:50058/" | |||
| ]) | |||
| result = subprocess.run("/usr/local/bin/logupload '' 1 1 true HTTP https://mockxconf:50058/ >> /opt/logs/logupload.log.0",shell=True) | |||
There was a problem hiding this comment.
Both subprocess.run calls were changed to use shell=True with no capture_output=True or text=True. The >> shell redirect in the command string will redirect the process's output to the log file, but the returned result object will have stdout=None and stderr=None. This means any test logic that relies on inspecting result.stdout or result.stderr will silently get None, potentially leading to misleading test results or AttributeError. The helper function run_uploadstblogs() in uploadstblogs_helper.py already provides the proper invocation pattern (capture_output=True, text=True, timeout=300) and could be reused here instead.
| def test_usblogupload_invalid_usage(self): | ||
| result = subprocess.run([USBLOGUPLOAD_BIN], capture_output=True) | ||
| assert result.returncode == 4, "Should exit with invalid usage code 4" | ||
| logs = grep_usblogupload_logs("Failed to initialize logging system") | ||
| # This log may or may not appear depending on implementation | ||
|
|
||
| def test_usblogupload_usb_not_mounted(self): | ||
| result = subprocess.run([USBLOGUPLOAD_BIN, "/tmp/notmounted"], capture_output=True) | ||
| assert result.returncode == 2, "Should exit with USB not mounted code 2" | ||
| logs = grep_usblogupload_logs("Failed to validate USB mount point") |
There was a problem hiding this comment.
Similarly, test_usblogupload_invalid_usage and test_usblogupload_usb_not_mounted each assign a logs variable using grep_usblogupload_logs() to check for specific log output ("Failed to initialize logging system" and "Failed to validate USB mount point" respectively), but then never assert on those variables. The test descriptions imply that these log messages are important for verifying correct behavior, but without an assertion they are not actually checked.
| """Execute uploadlogsnow using the specific binary command""" | ||
| cmd = "/usr/local/bin/logupload uploadlogsnow" | ||
| result = subprocess.run(cmd, shell=True, capture_output=True, text=True, timeout=300) | ||
| result = subprocess.run("/usr/local/bin/logupload uploadlogsnow >> /opt/logs/logupload.log.0",shell=True) |
There was a problem hiding this comment.
The subprocess module is imported as sp (alias), but sp is never used anywhere in this file. All calls use the bare name subprocess which is implicitly available through the from uploadstblogs_helper import * wildcard import. The unused import subprocess as sp on line 27 is dead code and should be removed to avoid confusion.
| with open(LOG_FILE, "a", encoding="utf-8") as f: | ||
| f.write(result.stdout) | ||
| f.write(result.stderr) | ||
| logs = grep_usblogupload_logs(":.*File:") |
There was a problem hiding this comment.
The grep_usblogupload_logs helper uses re.escape(search) to compile the search string as a literal pattern. However, in test_usblogupload_mac_address_log (line 73), the argument ":.*File:" is intended as a regular expression pattern (using .* as a wildcard), but re.escape will escape the dot and asterisk, causing the function to search for the exact literal string ":.*File:" instead of matching log lines with MAC address and file name. The function signature or usage needs to be consistent: either remove re.escape from the helper to allow regex patterns, or change the test to pass a literal string instead of a regex pattern.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| def test_usblogupload_success(self, tmp_path): | ||
| usb_mount = "/tmp" | ||
| # Run the binary and capture output | ||
| result = subprocess.run([USBLOGUPLOAD_BIN, usb_mount], capture_output=True, text=True) | ||
| # Write output to log file | ||
| with open(LOG_FILE, "a", encoding="utf-8") as f: | ||
| f.write(result.stdout) | ||
| f.write(result.stderr) | ||
| assert result.returncode == 0, "Should exit with success code 0" |
There was a problem hiding this comment.
The test_usblogupload_success test hardcodes /tmp as the USB mount point instead of using tmp_path like all the other tests in the same class. Using /tmp as a real mount path is semantically incorrect (it is not a USB mount), and it may interact with actual system state across test runs. The parameter tmp_path is already declared in the function signature but is never used — the test should use tmp_path consistently with the rest of the test suite.
| f.write(result.stderr) | ||
| logs = grep_usblogupload_logs("cleanup") | ||
| # This checks for cleanup log line (if implemented) | ||
| assert result.returncode in (0, 3), "Should exit with success or write error code" |
There was a problem hiding this comment.
test_usblogupload_temp_dir_cleanup and test_usblogupload_success are missing a blank line between them. This violates PEP 8, which requires two blank lines between method definitions in a class. The same issue exists in test_usb_logupload.py at the same location (lines 88–89).
| assert result.returncode in (0, 3), "Should exit with success or write error code" | |
| assert result.returncode in (0, 3), "Should exit with success or write error code" |
| # Simulate a valid mount and check for archive creation log | ||
| usb_mount = tmp_path / "usb" | ||
| usb_mount.mkdir() | ||
| result = subprocess.run([USBLOGUPLOAD_BIN, str(usb_mount)], capture_output=True, text=True) | ||
| with open(LOG_FILE, "a", encoding="utf-8") as f: | ||
| f.write(result.stdout) | ||
| f.write(result.stderr) | ||
| # Look for archive or compression log | ||
| logs = grep_usblogupload_logs("archive") | ||
| assert result.returncode in (0, 3), "Should exit with success or write error code" | ||
| # Archive log may or may not appear depending on implementation | ||
|
|
||
| def test_usblogupload_mac_address_log(self, tmp_path): | ||
| # Simulate a valid mount and check for MAC address log | ||
| usb_mount = tmp_path / "usb" | ||
| usb_mount.mkdir() | ||
| result = subprocess.run([USBLOGUPLOAD_BIN, str(usb_mount)], capture_output=True, text=True) | ||
| with open(LOG_FILE, "a", encoding="utf-8") as f: | ||
| f.write(result.stdout) | ||
| f.write(result.stderr) | ||
| logs = grep_usblogupload_logs(":.*File:") | ||
| # This checks for the log line with MAC address and file name | ||
| # (Regex match, may need adjustment based on actual log format) | ||
| assert result.returncode in (0, 3), "Should exit with success or write error code" | ||
|
|
||
| def test_usblogupload_temp_dir_cleanup(self, tmp_path): | ||
| # Simulate a valid mount and check for temp dir cleanup log | ||
| usb_mount = tmp_path / "usb" | ||
| usb_mount.mkdir() | ||
| result = subprocess.run([USBLOGUPLOAD_BIN, str(usb_mount)], capture_output=True, text=True) | ||
| with open(LOG_FILE, "a", encoding="utf-8") as f: | ||
| f.write(result.stdout) | ||
| f.write(result.stderr) | ||
| logs = grep_usblogupload_logs("cleanup") | ||
| # This checks for cleanup log line (if implemented) |
There was a problem hiding this comment.
The variable logs is assigned the result of grep_usblogupload_logs("archive") but is never used in any assertion. The test does not validate whether archive creation actually occurred. The same issue exists in test_usb_logupload.py (line 61), test_usblogupload_mac_address_log (line 83/73), test_usblogupload_temp_dir_cleanup (line 96/86), test_usblogupload_invalid_usage (line 115/105), and test_usblogupload_usb_not_mounted (line 121/111). Either the log results should be asserted, or the log-fetching calls should be removed.
| # Simulate a valid mount and check for archive creation log | |
| usb_mount = tmp_path / "usb" | |
| usb_mount.mkdir() | |
| result = subprocess.run([USBLOGUPLOAD_BIN, str(usb_mount)], capture_output=True, text=True) | |
| with open(LOG_FILE, "a", encoding="utf-8") as f: | |
| f.write(result.stdout) | |
| f.write(result.stderr) | |
| # Look for archive or compression log | |
| logs = grep_usblogupload_logs("archive") | |
| assert result.returncode in (0, 3), "Should exit with success or write error code" | |
| # Archive log may or may not appear depending on implementation | |
| def test_usblogupload_mac_address_log(self, tmp_path): | |
| # Simulate a valid mount and check for MAC address log | |
| usb_mount = tmp_path / "usb" | |
| usb_mount.mkdir() | |
| result = subprocess.run([USBLOGUPLOAD_BIN, str(usb_mount)], capture_output=True, text=True) | |
| with open(LOG_FILE, "a", encoding="utf-8") as f: | |
| f.write(result.stdout) | |
| f.write(result.stderr) | |
| logs = grep_usblogupload_logs(":.*File:") | |
| # This checks for the log line with MAC address and file name | |
| # (Regex match, may need adjustment based on actual log format) | |
| assert result.returncode in (0, 3), "Should exit with success or write error code" | |
| def test_usblogupload_temp_dir_cleanup(self, tmp_path): | |
| # Simulate a valid mount and check for temp dir cleanup log | |
| usb_mount = tmp_path / "usb" | |
| usb_mount.mkdir() | |
| result = subprocess.run([USBLOGUPLOAD_BIN, str(usb_mount)], capture_output=True, text=True) | |
| with open(LOG_FILE, "a", encoding="utf-8") as f: | |
| f.write(result.stdout) | |
| f.write(result.stderr) | |
| logs = grep_usblogupload_logs("cleanup") | |
| # This checks for cleanup log line (if implemented) | |
| # Simulate a valid mount and verify exit status for archive creation | |
| usb_mount = tmp_path / "usb" | |
| usb_mount.mkdir() | |
| result = subprocess.run([USBLOGUPLOAD_BIN, str(usb_mount)], capture_output=True, text=True) | |
| with open(LOG_FILE, "a", encoding="utf-8") as f: | |
| f.write(result.stdout) | |
| f.write(result.stderr) | |
| assert result.returncode in (0, 3), "Should exit with success or write error code" | |
| def test_usblogupload_mac_address_log(self, tmp_path): | |
| # Simulate a valid mount and verify exit status when MAC address logging is expected | |
| usb_mount = tmp_path / "usb" | |
| usb_mount.mkdir() | |
| result = subprocess.run([USBLOGUPLOAD_BIN, str(usb_mount)], capture_output=True, text=True) | |
| with open(LOG_FILE, "a", encoding="utf-8") as f: | |
| f.write(result.stdout) | |
| f.write(result.stderr) | |
| assert result.returncode in (0, 3), "Should exit with success or write error code" | |
| def test_usblogupload_temp_dir_cleanup(self, tmp_path): | |
| # Simulate a valid mount and verify exit status for temp dir cleanup path | |
| usb_mount = tmp_path / "usb" | |
| usb_mount.mkdir() | |
| result = subprocess.run([USBLOGUPLOAD_BIN, str(usb_mount)], capture_output=True, text=True) | |
| with open(LOG_FILE, "a", encoding="utf-8") as f: | |
| f.write(result.stdout) | |
| f.write(result.stderr) |
| ]) | ||
|
|
||
|
|
||
| result = subprocess.run("/usr/local/bin/logupload '' 1 1 true HTTP https://mockxconf:50058/ >> /opt/logs/logupload.log.0",shell=True) |
There was a problem hiding this comment.
Using shell=True with output redirected to a file (>>) means result.stdout and result.stderr will be empty, yet result.returncode is asserted on line 60. More importantly, the log-grep assertions on lines 63 and 66 depend on the log file containing the output, but previously capture_output=True was used so those logs may not have been written to the file either. Also, passing an empty string '' as a shell argument may not be interpreted correctly as an empty argument by all shells.
| subprocess.run(f"echo '' > {LOG_FILE}", shell=True) | ||
| yield | ||
| # Teardown: clear log file | ||
| subprocess.run(f"echo '' > {LOG_FILE}", shell=True) |
There was a problem hiding this comment.
Using echo '' > {LOG_FILE} writes a newline (not an empty file) and also uses shell=True with an f-string interpolating a file path, which is a shell injection risk if LOG_FILE ever contains special characters. A safer and more idiomatic approach is to open the file in write mode with Python directly (e.g., open(LOG_FILE, 'w').close() or using truncate()).
| subprocess.run(f"echo '' > {LOG_FILE}", shell=True) | |
| yield | |
| # Teardown: clear log file | |
| subprocess.run(f"echo '' > {LOG_FILE}", shell=True) | |
| open(LOG_FILE, "w", encoding="utf-8").close() | |
| yield | |
| # Teardown: clear log file | |
| open(LOG_FILE, "w", encoding="utf-8").close() |
| import subprocess | ||
| import os | ||
| import re | ||
| import pytest | ||
|
|
There was a problem hiding this comment.
The new test_usb_logupload.py file is missing the standard Apache 2.0 copyright header that is present in all other test files in this directory (e.g., test_uploadLogsNow.py, test_uploadstblogs_normal_upload.py). The header should be added for consistency with the codebase conventions.
| Scenario: USB not mounted or missing log path | ||
| Given the USB log upload binary is available | ||
| When I run usblogupload with a non-existent mount point | ||
| Then the process should fail with code 2 or 3 | ||
| And a failure message should be logged | ||
|
|
||
| Scenario: Archive creation on valid mount | ||
| Given a valid USB mount point | ||
| When I run usblogupload | ||
| Then the process should exit with code 0 or 3 | ||
| And an archive creation log may appear | ||
|
|
||
| Scenario: MAC address and file log | ||
| Given a valid USB mount point | ||
| When I run usblogupload | ||
| Then the process should exit with code 0 or 3 | ||
| And a log line with MAC address and file name may appear | ||
|
|
||
| Scenario: Temp directory cleanup | ||
| Given a valid USB mount point | ||
| When I run usblogupload | ||
| Then the process should exit with code 0 or 3 | ||
| And a cleanup log may appear | ||
|
|
||
| Scenario: Successful USB log upload | ||
| Given the USB log upload binary is available | ||
| When I run usblogupload with a valid mount point | ||
| Then the process should exit with code 0 | ||
| And a completion message should be logged | ||
|
|
||
| Scenario: Invalid usage | ||
| Given the USB log upload binary is available | ||
| When I run usblogupload with no arguments | ||
| Then the process should exit with code 4 | ||
| And a log about failed logging system initialization may appear | ||
|
|
||
| Scenario: USB not mounted | ||
| Given the USB log upload binary is available | ||
| When I run usblogupload with an unmounted path | ||
| Then the process should exit with code 2 | ||
| And a log about failed USB mount point validation may appear |
There was a problem hiding this comment.
In usblogupload.feature, the scenario "USB not mounted or missing log path" (line 4-8) and "USB not mounted" (lines 40-44) cover similar scenarios – both test with non-existent paths. Their corresponding Python tests test_usblogupload_missing_log_path and test_usblogupload_usb_not_mounted assert different exit codes: codes 2 or 3 vs. strictly code 2. If these scenarios are truly distinct (missing log path vs. USB mount point validation failure), the feature file should clearly differentiate the inputs and expected behavior to avoid ambiguity.
| Feature: USB Log Upload | ||
| This feature covers the USB log upload functionality, including error handling, archive creation, MAC address logging, temp directory cleanup, and success/failure scenarios. |
There was a problem hiding this comment.
The new usblogupload.feature file is missing the standard Apache 2.0 copyright header that is present in all other feature files in this directory (e.g., dcm-agent_bootup_sequence.feature, uploadstblogs_normal_upload.feature). The header should be added for consistency with the codebase conventions.
| def test_upload_started(): | ||
| assert "Start log upload Script" in grep_dcmdlogs("Start log upload Script") | ||
| assert "FLAG=0" in grep_dcmdlogs("Triggered uploadSTBLogs.sh with arguments") | ||
| assert "Called uploadDCMLogs" in grep_dcmdlogs("Called uploadDCMLogs") | ||
| assert "Start log upload via library API" in grep_dcmdlogs("Start log upload via library API") |
There was a problem hiding this comment.
The deletion of test_upload_script_started (order=2) in test_log_upload_onreboot_true_case.py leaves a gap in the test ordering: the sequence goes from order=1 to order=3, skipping order=2. While pytest-ordering handles this gracefully (tests will still run), the ordering numbers are inconsistent and may cause confusion when adding new tests in the future. The remaining tests should be renumbered to maintain a consistent sequence (1, 2, 3, 4, 5, 6) rather than (1, 3, 4, 4, 6).
| @pytest.mark.run(order=3) | ||
| def test_fw_cron_scheduled(): | ||
| sleep(420) |
There was a problem hiding this comment.
The deletion of test_upload_script_started_onboot_false (order=2) leaves a gap in the ordering sequence (1, 3, 4, 5, 6). Similarly to the true-case test, the remaining tests should be renumbered for consistency. More importantly, the sleep(420) from the deleted test_upload_script_started_onboot_false was moved into test_fw_cron_scheduled (order=3). This changes the overall test timing: previously the 420-second sleep was in an independent test step (order=2) before order=3. Now it is at the beginning of order=3. This effectively preserves the timing but it is worth ensuring the intent was to continue waiting 420 seconds before checking the FW cron schedule.
| @pytest.mark.run(order=3) | |
| def test_fw_cron_scheduled(): | |
| sleep(420) | |
| @pytest.mark.run(order=2) | |
| def test_wait_before_fw_cron_check(): | |
| # Wait for FW cron scheduling window before validation | |
| sleep(420) | |
| @pytest.mark.run(order=3) | |
| def test_fw_cron_scheduled(): |
| sleep(420) | ||
| assert "Scheduling DCM_FW_UPDATE Job handle" in grep_dcmdlogs("Scheduling DCM_FW_UPDATE Job handle") | ||
|
|
||
|
|
There was a problem hiding this comment.
In test_log_upload_onreboot_false_case.py, the test test_upload_script_started_onboot_false (order=2) was removed, but the sleep(420) that was inside it has been moved into test_fw_cron_scheduled (order=3). This means the sleep now runs unconditionally for that test case instead of only in the reboot-false path. The sleep was previously in the order=2 function to wait before checking for the false-onboot case. The intent and correctness of this logic change should be verified — if the sleep is truly needed here and in the right test is fine, but the duplication of logic (the moved sleep) could indicate an oversight.
| sleep(420) | |
| assert "Scheduling DCM_FW_UPDATE Job handle" in grep_dcmdlogs("Scheduling DCM_FW_UPDATE Job handle") | |
| max_wait = 420 | |
| interval = 10 | |
| for _ in range(int(max_wait / interval)): | |
| if "Scheduling DCM_FW_UPDATE Job handle" in grep_dcmdlogs("Scheduling DCM_FW_UPDATE Job handle"): | |
| return | |
| sleep(interval) | |
| # Final check after waiting up to max_wait seconds | |
| assert "Scheduling DCM_FW_UPDATE Job handle" in grep_dcmdlogs("Scheduling DCM_FW_UPDATE Job handle") |
| "HTTP", | ||
| "https://mockxconf:50058/" | ||
| ]) | ||
| result = subprocess.run("/usr/local/bin/logupload '' 1 1 true HTTP https://mockxconf:50058/ >> /opt/logs/logupload.log.0",shell=True) |
There was a problem hiding this comment.
Same issue as line 57: subprocess.run uses shell=True with a shell redirect but no capture_output or timeout. The binary could hang indefinitely.
| """Test: DCM scheduled upload is triggered correctly""" | ||
| restore_device_properties() | ||
| # Create test log files in /opt/logs | ||
| for i in range(2): | ||
| subprocess.run(f"dd if=/dev/urandom of=/opt/logs/test_log_{i}.log bs=1024 count=100 2>/dev/null", shell=True) | ||
| # DCM scheduled upload (FLAG=0, DCM_FLAG=0, TriggerType=0) | ||
| result = subprocess.run("/usr/local/bin/logupload '' 0 0 0 HTTP http://localhost:8080 0 0 '' >> /opt/logs/logupload.log.0",shell=True) | ||
| # Check for DCM processing | ||
| dcm_logs = grep_uploadstb_logs_regex(r"DCM|scheduled|FLAG.*0") | ||
| assert len(dcm_logs) > 0, "DCM scheduled upload should be processed" |
There was a problem hiding this comment.
The test_dcm_scheduled_trigger method body is indented one extra level (8 spaces instead of 4) relative to other methods in the same class (TestDCMScheduledStrategy). In Python, this is valid but inconsistent with the rest of the class and may cause confusion. More critically, the method body is now at class body indentation level (8 spaces) while still being a method — this is correct Python syntax but the unusual extra indentation is a style inconsistency that deviates from every other test method in this file.
| """Test: DCM scheduled upload is triggered correctly""" | |
| restore_device_properties() | |
| # Create test log files in /opt/logs | |
| for i in range(2): | |
| subprocess.run(f"dd if=/dev/urandom of=/opt/logs/test_log_{i}.log bs=1024 count=100 2>/dev/null", shell=True) | |
| # DCM scheduled upload (FLAG=0, DCM_FLAG=0, TriggerType=0) | |
| result = subprocess.run("/usr/local/bin/logupload '' 0 0 0 HTTP http://localhost:8080 0 0 '' >> /opt/logs/logupload.log.0",shell=True) | |
| # Check for DCM processing | |
| dcm_logs = grep_uploadstb_logs_regex(r"DCM|scheduled|FLAG.*0") | |
| assert len(dcm_logs) > 0, "DCM scheduled upload should be processed" | |
| """Test: DCM scheduled upload is triggered correctly""" | |
| restore_device_properties() | |
| # Create test log files in /opt/logs | |
| for i in range(2): | |
| subprocess.run(f"dd if=/dev/urandom of=/opt/logs/test_log_{i}.log bs=1024 count=100 2>/dev/null", shell=True) | |
| # DCM scheduled upload (FLAG=0, DCM_FLAG=0, TriggerType=0) | |
| result = subprocess.run("/usr/local/bin/logupload '' 0 0 0 HTTP http://localhost:8080 0 0 '' >> /opt/logs/logupload.log.0",shell=True) | |
| # Check for DCM processing | |
| dcm_logs = grep_uploadstb_logs_regex(r"DCM|scheduled|FLAG.*0") | |
| assert len(dcm_logs) > 0, "DCM scheduled upload should be processed" |
| subprocess.run(f"dd if=/dev/urandom of=/opt/logs/test_log_{i}.log bs=1024 count=100 2>/dev/null", shell=True) | ||
| # DCM scheduled upload (FLAG=0, DCM_FLAG=0, TriggerType=0) | ||
| result = subprocess.run("/usr/local/bin/logupload '' 0 0 0 HTTP http://localhost:8080 0 0 '' >> /opt/logs/logupload.log.0",shell=True) |
There was a problem hiding this comment.
The test_dcm_scheduled_trigger function uses subprocess.run(...) directly (lines 170, 172), but the file imports subprocess only as sp (import subprocess as sp, line 27). The subprocess name used on these lines is available only through the wildcard from uploadstblogs_helper import * import, which is an implicit and fragile dependency. If uploadstblogs_helper.py ever stops exporting subprocess, these calls will break with a NameError. It would be cleaner to either use sp.run(...) consistently (the module's explicit alias) or add a direct import subprocess to the file.
| subprocess.run(f"dd if=/dev/urandom of=/opt/logs/test_log_{i}.log bs=1024 count=100 2>/dev/null", shell=True) | |
| # DCM scheduled upload (FLAG=0, DCM_FLAG=0, TriggerType=0) | |
| result = subprocess.run("/usr/local/bin/logupload '' 0 0 0 HTTP http://localhost:8080 0 0 '' >> /opt/logs/logupload.log.0",shell=True) | |
| sp.run(f"dd if=/dev/urandom of=/opt/logs/test_log_{i}.log bs=1024 count=100 2>/dev/null", shell=True) | |
| # DCM scheduled upload (FLAG=0, DCM_FLAG=0, TriggerType=0) | |
| result = sp.run("/usr/local/bin/logupload '' 0 0 0 HTTP http://localhost:8080 0 0 '' >> /opt/logs/logupload.log.0", shell=True) |
| import pytest | ||
|
|
||
| USBLOGUPLOAD_BIN = "/usr/local/bin/usblogupload" | ||
| LOG_FILE = "/opt/logs/logupload.log" # Adjust if needed |
There was a problem hiding this comment.
The test_usb_logupload.py file has a development-time placeholder comment # Adjust if needed next to the LOG_FILE constant. This suggests the constant value may not be finalized and should be confirmed and the comment removed before merging.
| LOG_FILE = "/opt/logs/logupload.log" # Adjust if needed | |
| LOG_FILE = "/opt/logs/logupload.log" |
| init_logs = grep_uploadstb_logs("Context initialization successful") | ||
| assert len(init_logs) > 0, "Context should be initialized successfully" | ||
|
|
||
| # Verify device properties loaded | ||
| logs = grep_uploadstb_logs("DEVICE_TYPE") | ||
| assert len(logs) > 0, "Device type should be loaded from properties" | ||
|
|
||
| collection_logs = grep_uploadstb_logs_regex(r"collect|archive|gather") | ||
| assert len(collection_logs) > 0, "Log collection should be attempted" |
There was a problem hiding this comment.
The assertion in the removed assertion block checks for DEVICE_TYPE log lines using grep_uploadstb_logs("DEVICE_TYPE"). These assertions have been removed in this PR, but the initialization test still asserts on grep_uploadstb_logs("Context initialization successful") and collect|archive|gather. The removal of the DEVICE_TYPE assertion reduces test coverage for verifying that device properties are loaded correctly, which is an important initialization step.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
.github/workflows/L2-tests.yml:42
- In a GitHub Actions
run:block using bash, the shell runs withset -eby default, meaning any command that exits with a non-zero status immediately aborts the script. Sincegit diff --name-only --exit-codeexits with code 1 when there are diffs, the shell will terminate at that line before theif [ $? -ne 0 ]check is ever evaluated. This means the custom warning message will never be printed andexit 1on line 42 is unreachable. The error will appear as a generic shell failure rather than the informative message intended.
To fix this, capture the exit code explicitly without triggering early exit, for example by using git diff --name-only --exit-code || DIFF_EXIT=$? and then checking $DIFF_EXIT.
- name: Start l2-container service
run: |
docker run -d --name native-platform --link mockxconf -e ENABLE_MTLS=true -v ${{ github.workspace }}:/mnt/L2_CONTAINER_SHARED_VOLUME ghcr.io/rdkcentral/docker-device-mgt-service-test/native-platform:latest
- name: Enter Inside Platform native container and run L2 Test
run: |
docker exec -i native-platform /bin/bash -c "cd /mnt/L2_CONTAINER_SHARED_VOLUME/ && sh cov_build.sh && export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/usr/lib/x86_64-linux-gnu:/lib/aarch64-linux-gnu:/usr/local/lib: && sh test/run_l2.sh && sh test/run_uploadstblogs_l2.sh"
.github/workflows/L2-tests.yml:62
- The
run_uploadstblogs_l2.shscript has been removed from the docker exec command in the workflow. The command now only runssh test/run_l2.sh, which means none of thetest_usb_logupload.pytests added in this PR, and none of the other uploadstblogs functional tests (error handling, normal upload, retry logic, security, resource management, upload strategies) will be executed in CI. The test runner scriptrun_uploadstblogs_l2.shneeds to be added back to this command to ensure the new and existing uploadstblogs tests are executed.
ls -l
| - name: Start mock-xconf service | ||
| run: | | ||
| docker run -d --name mockxconf -p 50050:50050 -p 50051:50051 -p 50052:50052 -v ${{ github.workspace }}:/mnt/L2_CONTAINER_SHARED_VOLUME ghcr.io/rdkcentral/docker-device-mgt-service-test/mockxconf:latest | ||
| docker run -d --name mockxconf -p 50050:50050 -p 50051:50051 -p 50052:50052 -e ENABLE_MTLS=true -v ${{ github.workspace }}:/mnt/L2_CONTAINER_SHARED_VOLUME ghcr.io/rdkcentral/docker-device-mgt-service-test/mockxconf:latest |
There was a problem hiding this comment.
The .astylerc options file referenced in the --options=.astylerc argument does not exist in the repository. This will cause the astyle command to fail with an error like "Cannot find options file: .astylerc", which in turn will fail the entire "Check for formatting errors" step and break the CI pipeline on every PR. The .astylerc file needs to be created in the repository root, or the reference to it should be removed (using astyle with default options or another options file that exists).
|
|
||
| @pytest.mark.run(order=6) | ||
| def test_upload_started(): | ||
| assert "Start log upload Script" in grep_dcmdlogs("Start log upload Script") | ||
| assert "FLAG=0" in grep_dcmdlogs("Triggered uploadSTBLogs.sh with arguments") | ||
| assert "Called uploadDCMLogs" in grep_dcmdlogs("Called uploadDCMLogs") | ||
| assert "Start log upload via library API" in grep_dcmdlogs("Start log upload via library API") | ||
|
|
There was a problem hiding this comment.
After the removal of the order=2 test, the ordering in test_log_upload_onreboot_true_case.py now has a gap (1, then 3) and also has two tests both marked with @pytest.mark.run(order=4): test_fwupdate_script_started and test_upload_cron_scheduled. Having duplicate order values means the execution order between these two tests is non-deterministic when using pytest-ordering. If either test produces state that the other relies on, this could cause intermittent failures. The order numbers should be updated to be unique and sequential.
| @pytest.mark.run(order=3) | ||
| def test_fw_cron_scheduled(): | ||
| sleep(420) | ||
| assert "Scheduling DCM_FW_UPDATE Job handle" in grep_dcmdlogs("Scheduling DCM_FW_UPDATE Job handle") |
There was a problem hiding this comment.
After the removal of the order=2 test in this file, there is now a gap from order=1 directly to order=3, skipping order=2. While pytest-ordering handles gaps gracefully, it is good practice to renumber the remaining tests sequentially to keep the intent clear and avoid confusion for future maintainers.
| import pytest | ||
|
|
||
| USBLOGUPLOAD_BIN = "/usr/local/bin/usblogupload" | ||
| LOG_FILE = "/opt/logs/logupload.log" # Adjust if needed |
There was a problem hiding this comment.
The LOG_FILE constant is set to /opt/logs/logupload.log (without the .0 suffix), while the rest of the test suite consistently uses /opt/logs/logupload.log.0 (the UPLOADSTB_LOG constant in uploadstblogs_helper.py). The binary output is redirected to logupload.log.0 in other test files (lines 57 and 98 of test_uploadstblogs_normal_upload.py). Reading from and writing to a different log file may cause tests in this file to never find the expected log entries. The comment # Adjust if needed confirms this uncertainty. This should be resolved to use the correct log file path.
| LOG_FILE = "/opt/logs/logupload.log" # Adjust if needed | |
| LOG_FILE = "/opt/logs/logupload.log.0" |
No description provided.