Feature/test l2

[Abhinavpv28]

No description provided.

[Copilot]

Pull request overview

Adds unit- and L2-level coverage for the usbLogUpload component and wires it into existing CI/test runners alongside uploadstblogs.

Changes:

• Introduces new GoogleTest suites for usbLogUpload modules (utils/archive) and updates existing ones (main/validation/file-manager).
• Adds autotools build scaffolding for usbLogUpload/unittest and updates the repo unit-test runner to build/run the new test executables.
• Extends L2 test scripts/workflow and adds a new functional test + feature file for usblogupload.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
usbLogUpload/unittest/usb_log_validation_gtest.cpp	Updates validation unit tests; adds a custom main() for the gtest executable.
usbLogUpload/unittest/usb_log_utils_gtest.cpp	New utils unit test suite with local stubs and a custom main().
usbLogUpload/unittest/usb_log_main_gtest.cpp	Adds mock include and custom main() with global mock cleanup.
usbLogUpload/unittest/usb_log_file_manager_gtest.cpp	Adds mock include, introduces a remove_directory stub, and adjusts assertions in filesystem tests.
usbLogUpload/unittest/usb_log_archive_gtest.cpp	New archive unit test suite with dependency stubs and a custom main().
usbLogUpload/unittest/configure.ac	New autotools configure script for usbLogUpload unit tests.
usbLogUpload/unittest/Makefile.am	Adds new gtest binaries and link inputs for usbLogUpload unit tests.
usbLogUpload/src/usb_log_main.c	Wraps production main() with #ifndef GTEST_ENABLE for unit-test linking.
usbLogUpload/include/usb_log_validation.h	Removes the validate_system_prerequisites() declaration.
unit_test.sh	Updates unit-test script to build/run usbLogUpload unit tests and adjusts env/properties setup.
test/run_uploadstblogs_l2.sh	Improves device properties setup and adds MAC file preparation for L2 runs; adds a usbLogupload pytest run.
test/functional-tests/tests/test_usb_logupload.py	New pytest functional test coverage for usblogupload.
test/functional-tests/tests/test_uploadstblogs_normal_upload.py	Changes how logupload is invoked (shell + redirection).
test/functional-tests/tests/test_uploadLogsNow.py	Changes how uploadlogsnow is invoked (shell + redirection).
test/functional-tests/tests/test_log_upload_onreboot_true_case.py	Adjusts assertions to match “library API” logging behavior.
test/functional-tests/tests/test_log_upload_onreboot_false_case.py	Adjusts assertions to match “library API” logging behavior.
test/functional-tests/features/usblogupload.feature	New feature spec describing USB log upload scenarios.
.github/workflows/L2-tests.yml	Enables mTLS in mock-xconf and native-platform containers via env var.

[Copilot]

Same issue here: using shell=True without a timeout and redirecting output to a file reduces test observability and can cause CI hangs. Prefer subprocess.run([...], capture_output=True, text=True, timeout=...) (and write logs explicitly if needed).

[Copilot]

AC_CHECK_LIB([gtest],[main]) / AC_CHECK_LIB([gmock],[main]) is checking for a main symbol inside the gtest/gmock libraries, which is typically only provided by gtest_main (and not by -lgtest/-lgmock). This can make ./configure fail even when gtest/gmock are installed. Update the checks to look for the correct libraries/symbols (e.g., check gtest_main if you want the library-provided main), or remove these AC_CHECK_LIB checks and rely on header/library discovery via your link flags.

Suggested change

	AC_CHECK_LIB([gtest], [main])
	AC_CHECK_LIB([gmock], [main])
	AC_CHECK_LIB([gtest_main], [main])
	AC_CHECK_LIB([gmock_main], [main])

[Copilot]

remove_directory is declared with C linkage in this translation unit (because usb_log_file_manager.h is included inside an extern "C" { ... } block), but the definition here has default C++ linkage. That will either fail to compile (conflicting linkage) or fail to link (mangled symbol). Define it with extern "C" to match the declaration, or remove this stub and use the real implementation/mock instead.

[Copilot]

CreateUsbLogDirectorySuccessTest no longer verifies that the directory was actually created (it only checks the return code). With the file-ops mocking in place, this test can pass even if create_usb_log_directory() becomes a no-op. Add an assertion that the directory exists after the call, or assert that the underlying directory-creation operation/mock was invoked with the expected path.

[Copilot]

CleanupTemporaryFilesSuccessTest no longer checks that the temporary directory was actually removed. As written, the test can pass even if cleanup_temporary_files() doesn't perform any cleanup (especially with the current remove_directory stub). Add a post-condition assertion (directory is gone) or verify the cleanup operation via the mock to ensure the behavior is really exercised.

[Copilot]

Appending RDK_PROFILE=TV to /etc/device.properties on every run (>>) can create multiple RDK_PROFILE entries, which may lead to ambiguous parsing depending on how properties are read. Prefer idempotent update logic (replace existing key if present, otherwise append) similar to what’s done in test/run_uploadstblogs_l2.sh.

Suggested change

	echo "RDK_PROFILE=TV" >> /etc/device.properties
	if grep -q '^RDK_PROFILE=' /etc/device.properties 2>/dev/null; then
	sed -i 's/^RDK_PROFILE=.*/RDK_PROFILE=TV/' /etc/device.properties
	else
	echo "RDK_PROFILE=TV" >> /etc/device.properties
	fi

[Copilot]

These export LD_LIBRARY_PATH=... lines are evaluated by make, so $TOP_DIR will be treated as a make variable (e.g., $T + OP_DIR...) and will not expand to the shell environment variable you set in unit_test.sh. This can result in an incorrect LD_LIBRARY_PATH during build/test runs. Prefer setting LD_LIBRARY_PATH in the test runner script/CI environment (or use AM_TESTS_ENVIRONMENT with $$TOP_DIR if you truly need a make-time export).

Suggested change

	export LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH
	export LD_LIBRARY_PATH="$TOP_DIR/uploadstblogs/src/.libs:$LD_LIBRARY_PATH"
	export LD_LIBRARY_PATH=/usr/local/lib:$$LD_LIBRARY_PATH
	export LD_LIBRARY_PATH="$$TOP_DIR/uploadstblogs/src/.libs:$$LD_LIBRARY_PATH"

[Copilot]

This second unconditional append of RDK_PROFILE=TV can further duplicate the key in /etc/device.properties, which can cause non-deterministic behavior if the reader takes the first/last occurrence. Consider making the update idempotent (replace if present) and do it only once per script run.

[Copilot]

subprocess.run(...) was changed to a shell command with output redirection and no timeout/capture_output. This makes the test less debuggable (stdout/stderr are lost to the test runner) and can cause CI hangs if the command blocks. Prefer passing args as a list (no shell), keep a timeout, and capture output so failures are actionable.

Suggested change

	result = subprocess.run("/usr/local/bin/logupload uploadlogsnow >> /opt/logs/logupload.log.0",shell=True)
	cmd = ["/usr/local/bin/logupload", "uploadlogsnow"]
	result = sp.run(cmd, capture_output=True, text=True, timeout=60)

[Copilot]

Switching to shell=True with output redirection removes any timeout/capture of stdout/stderr and can hide failures or hang CI if logupload blocks. Prefer invoking the binary with an argument list (no shell), adding a timeout, and capturing output so assertions and failure logs remain reliable.

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.

[Copilot]

This file uses strncpy but doesn’t include <cstring>/<string.h>. Depending on transitive includes this can fail to compile. Add the appropriate header explicitly.

[Copilot]

run_uploadlogsnow() no longer sets a timeout or captures stdout/stderr. If logupload hangs in CI, this test suite can block indefinitely, and failures become harder to diagnose. Consider restoring a bounded timeout and capturing output (or at least enabling it on failure) instead of relying on shell redirection.

[Copilot]

These calls switched to shell=True without any timeout or captured output. That makes the tests vulnerable to hanging indefinitely in CI and reduces debuggability when failures occur. Prefer subprocess.run([...], timeout=..., capture_output=True, text=True) (or reuse run_uploadstblogs() from uploadstblogs_helper.py) and write logs conditionally on failure.

[Copilot]

cat /opt/logs/dcmd.log.0 will dump the entire log into CI output, which can be very large and may contain sensitive data. Consider tailing a small number of lines, gating it behind a failure condition, or removing it entirely.

Suggested change

	cat /opt/logs/dcmd.log.0
	tail -n 200 /opt/logs/dcmd.log.0

[Copilot]

remove_directory is declared with C linkage via file_operations.h (included through the surrounding extern "C"), but it’s defined here with C++ linkage. This will typically fail to compile due to conflicting language linkage. Define it inside an extern "C" block (matching the header) or rename it to avoid clashing with the production symbol.

[Copilot]

RDK_PROFILE=TV is appended a second time later in the script, which further increases the chance of duplicated keys in /etc/device.properties. If the profile must be enforced, do it once using an idempotent replace-or-add approach.

[Copilot]

This fixture writes device.properties next to the test file, but the code under test reads properties via getDevicePropertyData (typically backed by /etc/device.properties). As-is, the fixture likely has no effect on usblogupload behavior and can give false confidence. Consider updating /etc/device.properties in an idempotent way (or mocking getDevicePropertyData) instead of creating a local file.

Suggested change

	# Path to device.properties for test
	device_properties_path = os.path.join(os.path.dirname(__file__), "device.properties")
	backup_path = device_properties_path + ".bak"
	# Backup original if exists
	if os.path.exists(device_properties_path):
	os.rename(device_properties_path, backup_path)
	# Ensure RDK_PROFILE=TV is present
	with open(device_properties_path, "w", encoding="utf-8") as f:
	f.write("RDK_PROFILE=TV\n")
	yield
	# Restore original after test
	if os.path.exists(backup_path):
	os.remove(device_properties_path)
	os.rename(backup_path, device_properties_path)
	# Path to device.properties used by getDevicePropertyData (typically /etc/device.properties)
	device_properties_path = "/etc/device.properties"
	file_existed_before = os.path.exists(device_properties_path)
	original_content = ""
	modified = False
	if file_existed_before:
	# Read existing content so we can restore it after the test, if we modify it
	try:
	with open(device_properties_path, "r", encoding="utf-8", errors="ignore") as f:
	original_content = f.read()
	except OSError:
	# If we cannot read the file, do not attempt to modify or restore it
	original_content = ""
	file_existed_before = False
	# Ensure RDK_PROFILE=TV is present during the test
	needs_write = True
	if file_existed_before and original_content:
	if "RDK_PROFILE=TV" in original_content:
	needs_write = False
	if needs_write:
	try:
	mode = "a" if file_existed_before else "w"
	with open(device_properties_path, mode, encoding="utf-8") as f:
	f.write("RDK_PROFILE=TV\n")
	modified = True
	except OSError:
	# If we cannot write, proceed with the test; it may fail appropriately
	modified = False
	yield
	# Restore original state after test if we modified the file
	if modified:
	try:
	if file_existed_before:
	with open(device_properties_path, "w", encoding="utf-8") as f:
	f.write(original_content)
	else:
	# File was created by this fixture; remove it
	if os.path.exists(device_properties_path):
	os.remove(device_properties_path)
	except OSError:
	# Best-effort restore; ignore errors during cleanup
	pass

[Copilot]

Pull request overview

Copilot reviewed 19 out of 19 changed files in this pull request and generated 12 comments.

Comments suppressed due to low confidence (1)

test/run_uploadstblogs_l2.sh:67

• This script edits source code in /usr/common_utilities to disable SSL verification and comment out auth logic. That can mask real security behavior and may affect other test suites running in the same environment. Consider limiting these changes to a controlled mock/test configuration (or revert the edits after the tests complete).

cd /usr/common_utilities
sed -i '/file_upload\.sslverify/s/= 1;/= 0;/' uploadutils/mtls_upload.c
sed -i 's/\(ret_code = setCommonCurlOpt(curl, s3url, NULL, \)true\()\)/\1false\2/g' uploadutils/uploadUtil.c
sed -i '/if (auth) {/,/}/s/^/\/\/ /' uploadutils/uploadUtil.c

[Copilot]

This file uses strncpy and rmdir but does not include the headers that declare them (<cstring>/<string.h> for strncpy, <unistd.h> for rmdir). This is likely to fail compilation with stricter toolchains.

Suggested change

	#include <sys/stat.h>
	#include <sys/stat.h>
	#include <cstring>
	#include <unistd.h>

[Copilot]

get_current_timestamp stub writes to buf[len-1] and passes len-1 to strncpy without guarding len == 0. If the production code ever calls this with a zero-length buffer, this becomes an out-of-bounds write in the test binary. Consider returning an error when len == 0 (or ensuring the stub matches production validation).

Suggested change

	strncpy(buf, "01/01/26-12:00:00", len-1);
	buf[len-1] = '\0';
	if (buf == NULL || len == 0) {
	return -1;
	}
	strncpy(buf, "01/01/26-12:00:00", len - 1);
	buf[len - 1] = '\0';

[Copilot]

grep_usblogupload_logs escapes the search string, so patterns like ":.*File:" are treated literally (not as regex). In test_usblogupload_mac_address_log this likely never matches. If a regex match is intended, either add a separate helper that does regex matching or avoid re.escape for that call site.

[Copilot]

All subprocess.run(...) calls here lack a timeout. If usblogupload blocks (e.g., waiting on I/O), these tests can hang the entire suite. Consider adding a reasonable timeout per invocation and reporting stdout/stderr on timeout to aid debugging.

[Copilot]

pwd at this point looks like leftover debug output and will add noise to CI logs. Consider removing it (or use it consistently with a label if it’s needed for debugging).

Suggested change

pwd

[Copilot]

This test file uses subprocess.run(...) but does not import subprocess. Unless subprocess is injected via one of the star-imported helpers (unlikely and non-obvious), this will raise NameError and fail the test.

[Copilot]

mock_file_operations.cpp is added to the test sources, but COMMON_CPPFLAGS does not include ../../uploadstblogs/unittest/mocks. Since mock_file_operations.cpp includes "mock_file_operations.h" (no relative path), this will fail to compile unless the mocks directory is added to the include path (or the include in that file is changed).

[Copilot]

LOG_FILE is under /opt/logs, but the tests append to it without ensuring the directory exists. If /opt/logs is missing in the environment, open(LOG_FILE, "a") will raise and fail the test before any assertions. Consider creating the directory/file in a fixture (or using tmp_path for per-test logs).

[Copilot]

echo "AA:BB:CC:dd:EE:FF" >> /tmp/.estb_mac appends a new MAC each run, which can change behavior if the consumer reads the first/last line. Prefer overwriting (>), or ensure the file is cleaned before writing to keep tests deterministic.

Suggested change

	echo "AA:BB:CC:dd:EE:FF" >> /tmp/.estb_mac
	echo "AA:BB:CC:dd:EE:FF" > /tmp/.estb_mac

[Copilot]

Unconditionally cat-ing /opt/logs/dcmd.log.0 can flood CI logs and may fail if the file doesn’t exist yet. Consider guarding it (e.g., only print on failure, or check existence and tail a limited number of lines).

Suggested change

	cat /opt/logs/dcmd.log.0
	if [ -f /opt/logs/dcmd.log.0 ]; then
	echo "Last 200 lines of /opt/logs/dcmd.log.0:"
	tail -n 200 /opt/logs/dcmd.log.0
	fi

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 11 comments.

Comments suppressed due to low confidence (1)

unit_test.sh:53

• unit_test.sh clones external GitHub repositories (iarmmgrs, rdk_logger) and invokes cov_build.sh, which itself clones and builds additional external code, all from mutable branch heads. This makes every test run execute remote code with the privileges and secrets of the CI/device environment if any of those repositories or the network path are compromised. Pin each git clone to an immutable commit or vetted release tag (or vendor the dependencies) so the test runner no longer fetches and executes unverified code at runtime.

cd ../..
sh cov_build.sh
cd -
git clone https://github.com/rdkcentral/iarmmgrs.git
cp iarmmgrs/sysmgr/include/sysMgr.h /usr/local/include
cp iarmmgrs/maintenance/include/maintenanceMGR.h /usr/local/include
git clone https://github.com/rdkcentral/rdk_logger.git
cp rdk_logger/include/rdk_logger.h /usr/local/include

[Copilot]

AC_CHECK_LIB([gtest], [main]) / AC_CHECK_LIB([gmock], [main]) is brittle: many installations provide main() in libgtest_main instead of libgtest, and these unit tests now provide their own main() anyway. Consider checking for a gtest symbol other than main (or linking -lgtest_main and removing per-test main()), to avoid configure failures on standard gtest packages.

Suggested change

	AC_CHECK_LIB([gtest], [main])
	AC_CHECK_LIB([gmock], [main])
	dnl Use a C++ link test for Google Test and Google Mock instead of checking for main().
	AC_LANG_PUSH([C++])
	dnl Check for Google Test library
	AC_CACHE_CHECK([for Google Test library], [ac_cv_lib_gtest], [
	save_LIBS="$LIBS"
	LIBS="$LIBS -lgtest"
	AC_LINK_IFELSE(
	[AC_LANG_PROGRAM(
	[[#include <gtest/gtest.h>]],
	[[int argc = 0;
	char **argv = 0;
	::testing::InitGoogleTest(&argc, argv);
	return 0;]])],
	[ac_cv_lib_gtest=yes],
	[ac_cv_lib_gtest=no])
	LIBS="$save_LIBS"
	])
	if test "x$ac_cv_lib_gtest" = xyes; then
	LIBS="$LIBS -lgtest"
	AC_DEFINE([HAVE_LIBGTEST], [1],
	[Define to 1 if you have the Google Test library.])
	fi
	dnl Check for Google Mock library (typically depends on gtest)
	AC_CACHE_CHECK([for Google Mock library], [ac_cv_lib_gmock], [
	save_LIBS="$LIBS"
	LIBS="$LIBS -lgmock -lgtest"
	AC_LINK_IFELSE(
	[AC_LANG_PROGRAM(
	[[#include <gmock/gmock.h>]],
	[[int argc = 0;
	char **argv = 0;
	::testing::InitGoogleMock(&argc, argv);
	return 0;]])],
	[ac_cv_lib_gmock=yes],
	[ac_cv_lib_gmock=no])
	LIBS="$save_LIBS"
	])
	if test "x$ac_cv_lib_gmock" = xyes; then
	LIBS="$LIBS -lgmock -lgtest"
	AC_DEFINE([HAVE_LIBGMOCK], [1],
	[Define to 1 if you have the Google Mock library.])
	fi
	AC_LANG_POP([C++])

[Copilot]

The fixture writes RDK_PROFILE=TV to a device.properties file in the test directory, but usblogupload reads device properties via getDevicePropertyData() (typically backed by /etc/device.properties). As written, this setup likely has no effect on the binary under test. Consider updating /etc/device.properties (with proper backup/restore) or stubbing getDevicePropertyData() for this test environment.

[Copilot]

ExecuteWithValidInputTest in this file expects usb_log_upload_execute("/tmp/test_usb") to return USB_LOG_SUCCESS, but the test never creates that mount-point directory. Because validate_usb_mount_point() uses access() directly, the call will fail on a clean system. Create the directory in the test (and clean it up) or adjust the expectation/mocking so the test matches real behavior.

[Copilot]

The getDevicePropertyData() / getIncludePropertyData() stubs ignore the provided buffer length and use strcpy(). Even in tests, it’s better to respect the size_t parameter (e.g., bounded copy + explicit NUL termination) to avoid accidental overflows if the production code calls these with smaller buffers in the future.

Suggested change

	int getDevicePropertyData(const char*, char* buf, size_t) { strcpy(buf, "false"); return UTILS_SUCCESS; }
	int getIncludePropertyData(const char*, char* buf, size_t) { strcpy(buf, "/opt/logs"); return UTILS_SUCCESS; }
	int getDevicePropertyData(const char*, char* buf, size_t bufSize)
	{
	if (buf == nullptr || bufSize == 0U) {
	return UTILS_SUCCESS;
	}
	std::snprintf(buf, bufSize, "false");
	return UTILS_SUCCESS;
	}
	int getIncludePropertyData(const char*, char* buf, size_t bufSize)
	{
	if (buf == nullptr || bufSize == 0U) {
	return UTILS_SUCCESS;
	}
	std::snprintf(buf, bufSize, "/opt/logs");
	return UTILS_SUCCESS;
	}

[Copilot]

cov_build.sh now clones common_utilities without pinning a branch/tag/commit. This makes CI non-deterministic and can break builds when upstream changes. Prefer pinning to a known-good branch or commit SHA (and documenting it) to keep L2/Coverage runs reproducible.

[Copilot]

The copy_file_and_delete() stub always returns success without creating the destination file or deleting the source. Since create_usb_log_archive() (via create_archive()) will likely generate a real .tgz inside temp_dir, TearDown()'s rmdir(temp_dir) can fail because the directory is not empty. Make the stub simulate the expected copy+delete (or update teardown to remove the directory contents) so the test is self-cleaning and accurately validates behavior.

[Copilot]

grep -q '^RDK_PROFILE=' /etc/device.properties is run before ensuring /etc/device.properties exists. If the file is missing, grep will emit an error to stderr (even though the script continues). Consider touching/creating /etc/device.properties before this block to keep the script clean and predictable.

[Copilot]

remove_directory_recursive() calls ftw() but checks for FTW_DP/FTW_SL typeflags. Those constants are used with nftw() (post-order traversal) and won’t be produced by ftw(), which means subdirectories won’t get removed and the final rmdir(path) can fail. Consider switching to nftw(..., FTW_DEPTH | FTW_PHYS) or handling FTW_D properly so the test cleanup actually deletes nested directories.

[Copilot]

CreateUsbLogDirectorySuccessTest only asserts the return code and no longer verifies the observable behavior (that the directory was actually created). Adding an assertion that the directory exists after create_usb_log_directory() will make the test meaningful and help prevent regressions.

[Copilot]

This script git clones several external repositories (iarmmgrs, rdk_logger, telemetry, common_utilities) from GitHub using mutable branch heads, then builds/installs them, which is a classic supply-chain risk. If any of these repos or the network path is compromised, an attacker can execute arbitrary code in your build/test environment with access to secrets and build artifacts. Pin these dependencies to immutable commit hashes or vetted release tags (or vendor the sources locally) so CI does not fetch and run unverified remote code on each run.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 6 comments.

[Copilot]

fopen() isn’t checked for failure before writing; if the test runs in an environment where the working directory is not writable, this will segfault. Add assertions that f/f2 are non-null (and handle cleanup) so failures are reported cleanly.

Suggested change

	fputs("testdata", f);
	fclose(f);
	EXPECT_EQ(copy_file_and_delete(src, dst), 0);
	FILE* f2 = fopen(dst, "r");
	ASSERT_NE(f2, nullptr);
	ASSERT_NE(f, nullptr) << "Failed to open source file for writing";
	if (f == nullptr) {
	return;
	}
	fputs("testdata", f);
	fclose(f);
	EXPECT_EQ(copy_file_and_delete(src, dst), 0);
	FILE* f2 = fopen(dst, "r");
	ASSERT_NE(f2, nullptr) << "Failed to open destination file for reading";
	if (f2 == nullptr) {
	unlink(dst);
	return;
	}

[Copilot]

The two export LD_LIBRARY_PATH=... lines are fragile in Automake: the second assignment overwrites the first, uses an undefined TOP_DIR make variable (expands to empty), and includes quotes that will become part of the value. This can result in an incorrect LD_LIBRARY_PATH during builds/runs. Prefer setting LD_LIBRARY_PATH in the test runner scripts (as you already do in unit_test.sh) or use an rpath (-Wl,-rpath,...) / AM_LDFLAGS for test binaries instead of exporting it here.

Suggested change

	export LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH
	export LD_LIBRARY_PATH="$TOP_DIR/uploadstblogs/src/.libs:$LD_LIBRARY_PATH"

[Copilot]

The workflow no longer runs test/run_l2.sh, so the DCM/L2 suites executed there (boot sequence, cron cases, etc.) won’t run in CI anymore. If the intent is to narrow coverage to uploadstblogs/usblogupload only, consider either adding those checks into run_uploadstblogs_l2.sh or running both scripts to avoid silently dropping coverage.

Suggested change

	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_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"

[Copilot]

This remove_directory definition is unused (no references in this test file) and, because it lacks extern "C", it will not override the C remove_directory() symbol that usb_log_file_manager.c calls. If the intent is to mock remove_directory for cleanup_temporary_files(), add it to the existing mock_file_operations shim (with C linkage) and drive it via g_mockFileOperations, or remove this function to avoid confusion.

Suggested change

	bool remove_directory(const char* dirpath) {
	if (!dirpath) return false;
	struct stat st;
	// Return false if directory does not exist
	if (stat(dirpath, &st) != 0 || !S_ISDIR(st.st_mode)) return false;
	// Otherwise, simulate success
	return true;
	}

[Copilot]

This script now skips the test_log_upload_onreboot_false_case.py suite entirely. If this is intended, it should be documented and/or replaced with an equivalent assertion elsewhere; otherwise it reduces L2 coverage and can hide regressions for the "onreboot=false" path.

Suggested change

	#pytest -v --json-report --json-report-summary --json-report-file $RESULT_DIR/log_upload_reboot_false_test.json test/functional-tests/tests/test_log_upload_onreboot_false_case.py
	pytest -v --json-report --json-report-summary --json-report-file $RESULT_DIR/log_upload_reboot_false_test.json test/functional-tests/tests/test_log_upload_onreboot_false_case.py

[Copilot]

Same issue here: using shell=True with a single command string (and '') changes argv compared to the previous list-form invocation, and removes the ability to easily capture stdout/stderr for assertions. Prefer passing an argv list (including an explicit empty-string argument if needed) and adding a timeout/capture for reliability.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.

[Copilot]

Commenting out the log_upload_onreboot_false_case pytest invocation reduces L2 coverage, and cat /opt/logs/dcmd.log.0 can produce very large CI logs (and is noisy even on success). If this is temporary debugging, consider reverting it or gating the cat behind a failure condition / debug flag.

Suggested change

	#pytest -v --json-report --json-report-summary --json-report-file $RESULT_DIR/log_upload_reboot_false_test.json test/functional-tests/tests/test_log_upload_onreboot_false_case.py
	cat /opt/logs/dcmd.log.0
	pytest -v --json-report --json-report-summary --json-report-file $RESULT_DIR/log_upload_reboot_false_test.json test/functional-tests/tests/test_log_upload_onreboot_false_case.py
	if [ "${DCM_SHOW_LOGS:-0}" -ne 0 ] && [ -f /opt/logs/dcmd.log.0 ]; then
	cat /opt/logs/dcmd.log.0
	fi

[Copilot]

Including mock_file_operations.h implies this test binary is now linking against the mocked create_directory() implementation (from mock_file_operations.cpp), whose default behavior returns success without actually creating directories. With that default, usb_log_upload_execute() can fail later (e.g., create_temporary_directory() checks access() on the created path). Consider either initializing g_mockFileOperations to perform realistic directory creation / expectations, or updating the test expectations to match the mocked behavior.

[Copilot]

grep_usblogupload_logs() uses re.escape(search), so callers cannot pass regex patterns. Tests later pass strings like ":.*File:" expecting regex matching, which will never work with this helper. Add a regex-based grep helper (no escaping) or change the tests to search for a literal substring.

Suggested change

	search_pattern = re.compile(re.escape(search), re.IGNORECASE)
	search_pattern = re.compile(search, re.IGNORECASE)

[Copilot]

copy_file_and_delete() is stubbed to return success but it doesn’t delete the source archive created by create_archive(). That leaves files inside ./test_usb_log_dir, so TearDown()’s rmdir() will fail (directory not empty) and can make the suite flaky. Either have the stub simulate deletion of the source path, or explicitly remove the generated archive(s) in TearDown() (e.g., unlink + rmdir).

Suggested change

	// Simulate successful copy
	(void)dst; /* Destination is not used in this stub */
	if (src != NULL) {
	/* Simulate deletion of the source archive so temp_dir can be removed */
	(void)remove(src);
	}
	/* Always report success to match stub semantics */