Feature/usblog l2 60497#85
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds functional tests for the USB log upload feature (usblogupload binary) and modifies existing test cases for the uploadSTBLogs functionality. The changes introduce comprehensive testing for various USB log upload scenarios including error handling, validation, and success cases.
Changes:
- Added new test file
test_usb_logupload.pywith 7 test cases covering USB log upload functionality - Modified
test_uploadstblogs_normal_upload.pyto change how the logupload binary is invoked and removed some assertions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 21 comments.
| File | Description |
|---|---|
| test/functional-tests/tests/test_usb_logupload.py | New test suite for USB log upload with tests for missing paths, archive creation, MAC address logging, cleanup, success cases, invalid usage, and unmounted USB scenarios |
| test/functional-tests/tests/test_uploadstblogs_normal_upload.py | Modified subprocess invocation from list-based to shell string format with output redirection; removed DEVICE_TYPE assertion |
| def test_usblogupload_missing_log_path(self, tmp_path): | ||
| # Simulate missing log path by passing a non-existent mount point | ||
| usb_mount = str(tmp_path / "not_a_mount") | ||
| result = subprocess.run([USBLOGUPLOAD_BIN, 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 == 2 or result.returncode == 3, "Should fail with USB not mounted or write error" | ||
| logs = grep_usblogupload_logs("Failed") | ||
| # Accept log file or process output containing 'fail', 'error', or 'not mounted' | ||
| output = (result.stdout + result.stderr).lower() | ||
| assert ( | ||
| logs or | ||
| "fail" in output or | ||
| "error" in output or | ||
| "not mounted" in output | ||
| ), ( | ||
| f"Should log a failure message. Got stdout: {result.stdout}, stderr: {result.stderr}" | ||
| ) | ||
|
|
||
| def test_usblogupload_archive_creation(self, tmp_path): | ||
| # 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("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" | ||
| 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" | ||
|
|
||
| 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.
Missing pytest.mark.order decorators on test methods. All other test files in the functional-tests directory use @pytest.mark.order decorators to ensure tests run in a specific sequence. This ensures deterministic test execution order and helps with debugging test failures. Add @pytest.mark.order decorators to each test method with sequential numbering.
| with open(LOG_FILE, "a", encoding="utf-8") as f: | ||
| f.write(result.stdout) | ||
| f.write(result.stderr) | ||
| assert result.returncode == 2 or result.returncode == 3, "Should fail with USB not mounted or write error" |
There was a problem hiding this comment.
Inconsistent assertion logic that may lead to flaky tests. The test asserts that returncode should be 2 or 3, but exit code 2 is for "USB not mounted" and 3 is for "Writing error to USB". These are different failure scenarios. The test should be more specific about which error is expected when passing a non-existent mount point. Based on the test name and scenario, exit code 2 (USB not mounted) is the correct expectation.
| assert result.returncode == 2 or result.returncode == 3, "Should fail with USB not mounted or write error" | |
| assert result.returncode == 2, "Should fail with USB not mounted (exit code 2)" |
| usb_mount = "/tmp" | ||
| # Run the binary and capture output | ||
| result = subprocess.run([USBLOGUPLOAD_BIN, usb_mount], capture_output=True, text=True) |
There was a problem hiding this comment.
Test uses hardcoded /tmp path instead of tmp_path fixture. The test function accepts tmp_path as a parameter but doesn't use it, instead hardcoding "/tmp" as the mount point. This is inconsistent with the pattern used in other test methods and could lead to issues if /tmp is not available or has permission problems in certain test environments. Either use the tmp_path fixture consistently or remove the unused parameter.
| usb_mount = "/tmp" | |
| # Run the binary and capture output | |
| result = subprocess.run([USBLOGUPLOAD_BIN, usb_mount], capture_output=True, text=True) | |
| usb_mount = tmp_path / "usb" | |
| usb_mount.mkdir() | |
| # Run the binary and capture output | |
| result = subprocess.run([USBLOGUPLOAD_BIN, str(usb_mount)], capture_output=True, text=True) |
| ]) | ||
|
|
||
|
|
||
| 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.
Shell command injection vulnerability. Constructing shell commands as strings with shell=True is unsafe. The command string concatenates arguments that could potentially contain shell metacharacters. Although the URL appears hardcoded, this pattern is risky and inconsistent with the safer approach used in the removed code (using a list of arguments). Use the list-based approach without shell=True for better security.
| # Look for archive or compression log | ||
| 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 |
There was a problem hiding this comment.
Assertion without actual validation. The test searches for archive creation logs but does not assert anything about the results. The comment says "Archive log may or may not appear depending on implementation", which suggests this test is not verifying expected behavior. Either add an assertion to validate that archive logs exist when expected, or clarify what conditions should produce these logs.
| # Archive log may or may not appear depending on implementation | |
| if result.returncode == 0: | |
| # On successful completion, an archive creation message should be logged | |
| output = (result.stdout + result.stderr).lower() | |
| assert ( | |
| logs or | |
| "successfully created archive" in output | |
| ), ( | |
| f"Expected archive creation to be logged on success. " | |
| f"Got stdout: {result.stdout}, stderr: {result.stderr}" | |
| ) | |
| # For write error (return code 3), archive creation and logging are not required |
| 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.
Missing indentation before function definition. The function test_usblogupload_success should be indented at the same level as the other test methods in the class. This is a Python syntax issue that will cause the test class structure to be incorrect.
| 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" |
| "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.
Shell command injection vulnerability. Constructing shell commands as strings with shell=True is unsafe. The command string concatenates arguments that could potentially contain shell metacharacters. Although the URL appears hardcoded, this pattern is risky and inconsistent with the safer approach used in the removed code (using a list of arguments). Use the list-based approach without shell=True for better security.
| 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.
Shell command injection vulnerability. Using shell=True with f-strings that include file paths is a security risk. If LOG_FILE were to contain shell metacharacters or be controlled by an attacker, this could lead to command injection. Use subprocess methods without shell=True, or use safer alternatives like truncating the file with Python's open() function.
| subprocess.run(f"echo '' > {LOG_FILE}", shell=True) | |
| yield | |
| # Teardown: clear log file | |
| subprocess.run(f"echo '' > {LOG_FILE}", shell=True) | |
| try: | |
| with open(LOG_FILE, "w", encoding="utf-8"): | |
| pass | |
| except OSError as e: | |
| print(f"Could not clear log file {LOG_FILE} during setup: {e}") | |
| yield | |
| # Teardown: clear log file | |
| try: | |
| with open(LOG_FILE, "w", encoding="utf-8"): | |
| pass | |
| except OSError as e: | |
| print(f"Could not clear log file {LOG_FILE} during teardown: {e}") |
| f.write(result.stderr) | ||
| # Look for archive or compression log | ||
| logs = grep_usblogupload_logs("Successfully created archive") | ||
| assert result.returncode in (0, 3), "Should exit with success or write error code" |
There was a problem hiding this comment.
Inconsistent assertion logic that may lead to flaky tests. The test checks for a success exit code (0) or write error (3), but when a valid mount point is provided, the result should be deterministic. If the operation succeeds, it should return 0; if there's a write error, it should return 3. Accepting both makes the test less effective at catching regressions. Consider splitting this into separate test cases for success and error scenarios, or make the assertion more specific based on the actual test scenario.
| assert result.returncode in (0, 3), "Should exit with success or write error code" | |
| assert result.returncode == 0, "Should exit with success code" |
| 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.
Potentially incorrect regex pattern. The pattern ":.*File:" appears to be searching for a colon followed by any characters and then "File:", which may not correctly match the expected log format for MAC address entries. Based on the comment, this should match logs containing MAC addresses and file names. Consider reviewing the actual log format and adjusting the regex pattern to be more specific and accurate for matching MAC address entries.
| 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 |
There was a problem hiding this comment.
The test creates logs variable but never asserts anything about it. The comment on line 106 says the log may or may not appear, making this test non-deterministic. This test only checks the return code and doesn't verify the actual logging behavior it claims to test.
| # This log may or may not appear depending on implementation | |
| assert logs, "Should log failure to initialize logging system on invalid usage" |
| 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 expects a specific return code of 0 for success, but it's inconsistent with other tests in the same file that accept multiple return codes (0 or 3). The binary might return different codes in different scenarios. Consider accepting multiple valid return codes like other tests do.
| assert result.returncode == 0, "Should exit with success code 0" | |
| assert result.returncode in (0, 3), "Should exit with success or write error code" |
| result = subprocess.run(cmd, shell=True, capture_output=True, text=True, timeout=300) | ||
| return result | ||
|
|
||
| subprocess.run("/usr/local/bin/logupload uploadlogsnow >> /opt/logs/logupload.log",shell=True) |
There was a problem hiding this comment.
The function run_uploadlogsnow defines a variable cmd but then doesn't use it. Instead, it directly calls subprocess.run with a hardcoded command. Either remove the unused cmd variable assignment or use it in the subprocess.run call. Additionally, this function doesn't return any result, which differs from the original implementation that returned the result object.
| subprocess.run("/usr/local/bin/logupload uploadlogsnow >> /opt/logs/logupload.log",shell=True) | |
| cmd = "/usr/local/bin/logupload uploadlogsnow >> /opt/logs/logupload.log" | |
| return sp.run(cmd, shell=True) |
|
|
||
|
|
||
| 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.
There is trailing whitespace at the end of this line. Remove it to maintain code cleanliness.
| return result | ||
|
|
||
| subprocess.run("/usr/local/bin/logupload uploadlogsnow >> /opt/logs/logupload.log",shell=True) | ||
|
|
There was a problem hiding this comment.
The subprocess module is imported as 'sp' on line 27, but this code uses the full name 'subprocess'. Either use 'sp.run()' to match the import alias, or change the import to 'import subprocess' without an alias.
| echo "1. Running usbLogupload Tests..." | ||
| pytest -v --json-report --json-report-summary \ | ||
| --json-report-file $RESULT_DIR/uploadLogsNow.json test/functional-tests/tests/test4.py | ||
| --json-report-file $RESULT_DIR/usb_logupload.json test/functional-tests/tests/test_usb_logupload.py |
There was a problem hiding this comment.
There is an invisible character (likely a right-to-left mark or zero-width character) at the end of this line. This can cause the shell script to fail when trying to locate the test file. Remove the invisible character at the end of the line.
| --json-report-file $RESULT_DIR/usb_logupload.json test/functional-tests/tests/test_usb_logupload.py | |
| --json-report-file $RESULT_DIR/usb_logupload.json test/functional-tests/tests/test_usb_logupload.py |
| result = subprocess.run(cmd, shell=True, capture_output=True, text=True, timeout=300) | ||
| return result | ||
|
|
||
| subprocess.run("/usr/local/bin/logupload uploadlogsnow >> /opt/logs/logupload.log.0",shell=True) |
There was a problem hiding this comment.
The function signature has changed from returning a result object to returning None, which breaks the existing code. The original function returned the result of subprocess.run(), but now it returns nothing. This change affects any code that depends on checking the return value, return code, or captured output from this function.
| # This checks for cleanup log line (if implemented) | ||
| assert result.returncode in (0, 3), "Should exit with success or write error code" | ||
| def test_usblogupload_success(self, tmp_path): | ||
| usb_mount = "/tmp" |
There was a problem hiding this comment.
The tmp_path parameter is defined but never used in this test method. The test uses the hardcoded path "/tmp" instead. Either remove the unused parameter or use tmp_path for better test isolation and avoid potential conflicts with the actual system's /tmp directory.
| usb_mount = "/tmp" | |
| usb_mount = str(tmp_path) |
| # Check initialization logs | ||
| init_logs = grep_uploadstb_logs("Context initialization successful") | ||
| assert len(init_logs) > 0, "Context should be initialized successfully" | ||
|
|
There was a problem hiding this comment.
Removed assertion that verifies device properties are loaded. The original code checked for DEVICE_TYPE in the logs to ensure device properties were loaded successfully. This is an important validation step. Consider whether this assertion should be retained or if there's a specific reason it was removed.
| # Verify device properties are loaded (DEVICE_TYPE should be logged) | |
| device_type_logs = grep_uploadstb_logs("DEVICE_TYPE") | |
| assert len(device_type_logs) > 0, "Device properties should be loaded and DEVICE_TYPE should be logged" |
| with open(LOG_FILE, "a", encoding="utf-8") as f: | ||
| f.write(result.stdout) | ||
| f.write(result.stderr) |
There was a problem hiding this comment.
The file handle opened with open(LOG_FILE, "a", encoding="utf-8") is not properly closed in case of exceptions during write operations. Use a context manager (with statement) to ensure the file is properly closed even if an exception occurs: with open(LOG_FILE, "a", encoding="utf-8") as f: followed by the write operations inside the with block. This pattern is safer and prevents potential resource leaks.
|
|
||
| # Setup debug logging | ||
| echo "LOG.RDK.DEFAULT" >> /etc/debug.ini | ||
| echo "RDK_PROFILE=TV" >> /etc/device.properties |
There was a problem hiding this comment.
The RDK_PROFILE environment variable is being written to /etc/device.properties before the script ensures that the file exists (line 44-46). If /etc/device.properties doesn't exist when line 32 executes, the file will be created by the echo command, but subsequent checks and appends may fail or behave inconsistently. Move this line to after line 54 (after all device.properties existence and content checks) to ensure proper file initialization order.
| import os | ||
| import re | ||
| import pytest | ||
|
|
There was a problem hiding this comment.
The test file is missing a module-level docstring that is consistently present in all other test files in the codebase. For example, test_uploadLogsNow.py has "Test cases for uploadLogsNOw functionality..." and test_uploadstblogs_error_handling.py has "Test cases for uploadSTBLogs error handling and edge cases...". Add a module docstring describing the purpose and scope of these USB log upload tests to maintain consistency with the rest of the test suite.
| 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") |
There was a problem hiding this comment.
The variable 'logs' is assigned but never used in this test. Either remove the assignment if the log check is not needed, or add an assertion to verify the logs contain expected content. Having unused variables reduces code clarity and may indicate incomplete test logic.
| logs = grep_usblogupload_logs("Failed to initialize logging system") |
| with open(LOG_FILE, "a", encoding="utf-8") as f: | ||
| f.write(result.stdout) | ||
| f.write(result.stderr) |
There was a problem hiding this comment.
The file handle opened with open(LOG_FILE, "a", encoding="utf-8") is not properly closed in case of exceptions during write operations. Use a context manager (with statement) to ensure the file is properly closed even if an exception occurs: with open(LOG_FILE, "a", encoding="utf-8") as f: followed by the write operations inside the with block. This pattern is safer and prevents potential resource leaks.
|
|
||
| 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 removed assertion for DEVICE_TYPE was checking that device properties are properly loaded. This is an important validation step that ensures the configuration system is working correctly. Without this check, the test might pass even if device properties are not being read properly. Consider keeping this assertion or adding an alternative way to verify that the configuration is loaded.
| # Verify that device properties (including DEVICE_TYPE) were loaded from configuration | |
| device_type_logs = grep_uploadstb_logs_regex(r"DEVICE_TYPE") | |
| assert len(device_type_logs) > 0, "Device properties must be loaded from configuration (DEVICE_TYPE should be present in logs)" |
| "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 security issue as line 57 - using shell=True with subprocess.run. Use the list form instead for better security.
| assert result.returncode == 2 or result.returncode == 3, "Should fail with USB not mounted or write error" | ||
| logs = grep_usblogupload_logs("Failed") | ||
| # Accept log file or process output containing 'fail', 'error', or 'not mounted' | ||
| output = (result.stdout + result.stderr).lower() | ||
| assert ( | ||
| logs or | ||
| "fail" in output or | ||
| "error" in output or | ||
| "not mounted" in output | ||
| ), ( | ||
| f"Should log a failure message. Got stdout: {result.stdout}, stderr: {result.stderr}" |
There was a problem hiding this comment.
The assertion logic here allows for multiple different return codes (2 or 3) and also multiple ways to indicate failure (logs or output). This makes the test less precise and harder to debug. According to the README.md, exit code 2 means "USB not mounted" and exit code 3 means "Writing error to USB". Consider splitting this into two separate test cases: one that expects exit code 2 with "not mounted" message, and another that simulates write failures expecting exit code 3.
| assert result.returncode == 2 or result.returncode == 3, "Should fail with USB not mounted or write error" | |
| logs = grep_usblogupload_logs("Failed") | |
| # Accept log file or process output containing 'fail', 'error', or 'not mounted' | |
| output = (result.stdout + result.stderr).lower() | |
| assert ( | |
| logs or | |
| "fail" in output or | |
| "error" in output or | |
| "not mounted" in output | |
| ), ( | |
| f"Should log a failure message. Got stdout: {result.stdout}, stderr: {result.stderr}" | |
| # According to README: exit code 2 => "USB not mounted" | |
| assert result.returncode == 2, "Expected exit code 2 (USB not mounted) for non-existent mount point" | |
| # Require an explicit "not mounted" indication in logs or process output | |
| logs = grep_usblogupload_logs("not mounted") | |
| output = (result.stdout + result.stderr).lower() | |
| assert ( | |
| logs or | |
| "not mounted" in output | |
| ), ( | |
| f"Should indicate 'not mounted'. Got stdout: {result.stdout}, stderr: {result.stderr}" |
| 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.
Same issue as previous tests - the assertion accepts both exit code 0 and 3, making the test expectations unclear. See comments on earlier test methods.
| # Final assertion: pass if any evidence of success, regardless of return code | ||
| assert success_found or status_indicators or archive_evidence, \ | ||
| f"Upload should succeed (logs/status/archive/returncode). Got return code: {result.returncode}" |
There was a problem hiding this comment.
The modified assertion now accepts success based on any of three conditions (success_found OR status_indicators OR archive_evidence), which significantly weakens the test. The original assertion required both success logs/status AND didn't check the return code. The new version completely ignores the return code and only requires one piece of evidence. This makes the test too permissive - it could pass even if critical parts of the upload process failed. Consider requiring multiple forms of evidence for success, not just any single indicator.
|
|
||
| # Allow non-zero return code if logs or status files indicate success | ||
| # (Some environments may return 1 for non-critical issues) |
There was a problem hiding this comment.
The comment states "Allow non-zero return code if logs or status files indicate success", but this weakens the test significantly. The test now passes based on indirect evidence (log messages, status files) rather than the actual return code of the binary. This makes the test less reliable and could mask actual failures. If the binary is returning non-zero codes for successful uploads, that's a bug in the binary that should be fixed, not worked around in the tests. Consider either: 1) Fixing the binary to return 0 on success, or 2) Documenting why non-zero returns are acceptable and making the return code checking more precise (e.g., "accept codes 0 and 1, but not others").
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
test/functional-tests/tests/test_uploadLogsNow.py:22
- This file has been converted from Python tests into C++/GoogleTest code, but it still has a
.pyextension and lives underfunctional-tests/tests(and is invoked by pytest in the L2 runner). That combination is not runnable. Additionally, the content appears to duplicateuploadstblogs/unittest/uploadlogsnow_gtest.cpp; if the goal is unit testing, it should be added/updated there (or removed here) and wired into the autotools unit test build.
####################################################################################
# If not stated otherwise in this file or this component's Licenses file the
# following copyright and licenses apply:
#
# Copyright 2026 RDK Management
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
####################################################################################
"""
Test cases for uploadLogsNOw functionality
Tests the immediate upload logs scenario with custom endpoint URL configuration
| pytest -v --json-report --json-report-summary \ | ||
| --json-report-file $RESULT_DIR/uploadLogsNow.json test/functional-tests/tests/test_uploadLogsNow.py |
There was a problem hiding this comment.
pytest is invoked on test_uploadLogsNow.py, but that file now contains C++/GoogleTest code (it’s no longer valid Python). This will cause the L2 runner to fail immediately when pytest tries to import/parse it. Either restore a Python test module here, or move the GoogleTest code to a .cpp under the unit test build and update the runner to execute the compiled gtest binary instead of pytest.
| pytest -v --json-report --json-report-summary \ | |
| --json-report-file $RESULT_DIR/uploadLogsNow.json test/functional-tests/tests/test_uploadLogsNow.py | |
| # The UploadLogsNow tests have been migrated to a GoogleTest-based binary. | |
| # Run the gtest binary if available; otherwise, skip this suite gracefully. | |
| GTEST_UPLOADLOGSNOW_BINARY="/usr/bin/uploadLogsNow_gtest" | |
| if [ -x "$GTEST_UPLOADLOGSNOW_BINARY" ]; then | |
| "$GTEST_UPLOADLOGSNOW_BINARY" | |
| UPLOADLOGSNOW_STATUS=$? | |
| if [ $UPLOADLOGSNOW_STATUS -ne 0 ]; then | |
| echo "UploadLogsNow GoogleTest suite failed with status $UPLOADLOGSNOW_STATUS" | |
| fi | |
| else | |
| echo "UploadLogsNow GoogleTest binary not found at $GTEST_UPLOADLOGSNOW_BINARY; skipping this suite." | |
| fi |
|
|
||
| # 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 on each run, which can create duplicate keys in /etc/device.properties and make property parsing order-dependent. Consider making this idempotent (create the file first if needed, then grep -q '^RDK_PROFILE=' before appending).
| def grep_usblogupload_logs(search: str): | ||
| search_result = [] | ||
| search_pattern = re.compile(re.escape(search), re.IGNORECASE) | ||
| try: | ||
| with open(LOG_FILE, 'r', encoding='utf-8', errors='ignore') as file: | ||
| for line in file: | ||
| if search_pattern.search(line): | ||
| search_result.append(line) |
There was a problem hiding this comment.
grep_usblogupload_logs() always applies re.escape() to the input, so callers cannot use regex patterns. However, later tests pass strings like ":.*File:" that look like regex. Either remove re.escape() (and treat input as regex), or add a separate grep_usblogupload_logs_regex() helper and update callers accordingly.
No description provided.