L2 Tests for Crashupload

[gomathishankar37]

No description provided.

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/crashupload/41/rdk/components/generic/crashupload

• Commit: 6c2a9d8

Report detail: gist'

[Copilot]

Pull request overview

This PR adds comprehensive L2 functional tests for the crashupload functionality, including end-to-end testing of minidump upload, coredump upload, and startup cleanup processes. The changes include new test suites, a mock server implementation for simulating S3 endpoints, and modifications to the build and CI/CD pipeline to support the test infrastructure.

Changes:

• Added three new test suites covering minidump upload, coredump upload, and startup cleanup scenarios
• Implemented a Node.js mock server (crashUpload.js) to simulate S3 presigned URL endpoints for testing
• Updated build scripts to apply SSL verification patches for L2 test environment compatibility
• Modified GitHub Actions workflow to orchestrate multiple Docker containers for test execution

Reviewed changes

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

Show a summary per file

File	Description
test/functional-tests/tests/test_startup_cleanup.py	Implements tests for on-startup cleanup batch process, verifying cleanup logic for old dumps and archives
test/functional-tests/tests/test_minidump_happy_path.py	Tests the complete minidump upload workflow including archive creation and S3 upload simulation
test/functional-tests/tests/test_coredump_happy_path.py	Tests the coredump upload workflow with specific coredump file naming requirements and processing logic
test/crashUpload.js	Mock HTTPS server implementing S3 presigned URL endpoints for metadata and file upload simulation
cov_build.sh	Adds automated patching of common_utilities to disable SSL verification for mock server compatibility
run_l2.sh	Adds execution of the three new test suites with JSON report generation
.github/workflows/L2-Test.yml	Orchestrates Docker containers for mockxconf and native-platform, enabling integrated L2 testing

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The 'sleep 5' at the start of the command chain is a brittle way to wait for the container to be ready. If the container takes longer than 5 seconds to start (e.g., under high load), subsequent commands will fail. Consider using a retry loop or a health check command like 'docker exec mockxconf echo "ready"' with retries instead of a fixed sleep duration.

Suggested change

	sleep 5 && \
	attempts=0
	max_attempts=30
	until docker exec mockxconf echo "ready" >/dev/null 2>&1; do
	attempts=$((attempts + 1))
	if [ "$attempts" -ge "$max_attempts" ]; then
	echo "Error: mockxconf container did not become ready in time."
	exit 1
	fi
	echo "Waiting for mockxconf container to be ready... (attempt $attempts/$max_attempts)"
	sleep 2
	done && \

[Copilot]

This test file is missing the copyright and license header that is present in the other test files (test_minidump_happy_path.py and test_coredump_happy_path.py). For consistency and legal compliance, add the same Apache 2.0 license header at the top of this file.

[Copilot]

The variable 'threshold_found' is assigned inside the loop but its value is used to track when failures start occurring. However, the logic appears inverted - when result_val becomes non-zero (failure), threshold_found is set. But the comment says "First success" for result_val == 0. This logic should be reviewed to ensure it correctly identifies the buffer size threshold where the function starts failing rather than succeeding.

[Copilot]

The command on line 62 references paths 'test/cov_build.sh' and 'test/run_l2.sh', but based on the diff, the actual files appear to be at the root level: 'cov_build.sh' and 'run_l2.sh' (without the 'test/' prefix). This path mismatch will cause the chmod and sh commands to fail because the files won't be found at the specified locations. Update the paths to match the actual file locations or move the files to match the expected paths.

Suggested change

	docker exec -i native-platform /bin/bash -c "cd /mnt/L2_CONTAINER_SHARED_VOLUME/ && chmod +x test/cov_build.sh test/run_l2.sh && sh test/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"
	docker exec -i native-platform /bin/bash -c "cd /mnt/L2_CONTAINER_SHARED_VOLUME/ && chmod +x cov_build.sh run_l2.sh && 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 run_l2.sh"

[Copilot]

Variable result is not used.

Suggested change

	result = self.run_crashupload(dump_type="0", upload_flag="secure")
	self.run_crashupload(dump_type="0", upload_flag="secure")

[Copilot]

Variable result is not used.

Suggested change

	result = self.run_crashupload(dump_type="0", upload_flag="secure")
	self.run_crashupload(dump_type="0", upload_flag="secure")

[Copilot]

Import of 'shutil' is not used.

Suggested change

import shutil

[Copilot]

Import of 'shutil' is not used.

Suggested change

import shutil

[Copilot]

Import of 'sys' is not used.

Suggested change

import sys

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/crashupload/41/rdk/components/generic/crashupload

• Commit: 6becd83

Report detail: gist'

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/crashupload/41/rdk/components/generic/crashupload

• Commit: 69a3ea3

Report detail: gist'

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Race condition in startup verification. The grep command on line 51 checks if the node process is running, but since docker exec -d was used on line 49 to start it in detached mode, there's no guarantee the process has fully started even after the 5-second sleep. If the process takes longer to start or encounters an error during initialization, the grep might succeed (finding the starting process) while the server isn't actually ready to accept connections. Consider adding a health check that actually tests the server endpoint (e.g., curl the admin endpoint) rather than just checking if the process exists.

Suggested change

	sleep 5 && \
	docker exec mockxconf ps aux | grep -E "node.*crashUpload" && \
	echo "✓ crashUpload.js is running" || (echo "✗ crashUpload.js failed to start" && docker exec mockxconf cat /tmp/crashUpload.log && exit 1)
	docker exec mockxconf sh -c '\
	echo "Waiting for crashUpload.js health endpoint..." && \
	for i in $(seq 1 12); do \
	if curl -fsS http://localhost:50056/health >/dev/null 2>&1; then \
	echo "✓ crashUpload.js is responding to health checks"; \
	exit 0; \
	fi; \
	echo "Health check attempt $i failed; retrying in 5 seconds..."; \
	sleep 5; \
	done; \
	echo "✗ crashUpload.js failed health checks"; \
	echo "==== crashUpload.js log (/tmp/crashUpload.log) ===="; \
	cat /tmp/crashUpload.log || true; \
	exit 1 \
	'

[Copilot]

Missing error handling for request body processing. The code doesn't set up an error handler for the 'error' event on the request stream. If the client disconnects or there's a network error during body reading, the error will go unhandled and could crash the Node.js process. Add req.on('error', (err) => { ... }) to handle stream errors gracefully.

Suggested change

	req.on('data', chunk => body.push(chunk));
	req.on('data', chunk => body.push(chunk));
	req.on('error', (err) => {
	console.error('[Crash Metadata] Request error while reading body:', err.message);
	if (!res.headersSent) {
	res.writeHead(400, { 'Content-Type': 'text/plain' });
	res.end('Error reading request body');
	}
	});

[Copilot]

Naive form data parsing for multipart/form-data. The code on line 150 uses body.split('&') which works for application/x-www-form-urlencoded but will not correctly parse multipart/form-data (which uses boundary separators, not &). This could lead to incorrect parsing when the Content-Type is multipart/form-data. Consider using a proper form parsing library like 'busboy' or 'formidable', or at minimum, add explicit handling that only applies this parsing for 'application/x-www-form-urlencoded'.

Suggested change

	} else {
	// Parse form data (application/x-www-form-urlencoded or multipart/form-data)
	body.split('&').forEach(param => {
	const [key, value] = param.split('=');
	if (key) {
	metadata[key] = decodeURIComponent(value || '');
	}
	});
	} else if (contentType.includes('application/x-www-form-urlencoded')) {
	// Parse URL-encoded form data
	body.split('&').forEach(param => {
	const [key, value] = param.split('=');
	if (key) {
	metadata[key] = decodeURIComponent(value || '');
	}
	});
	} else if (contentType.includes('multipart/form-data')) {
	// Multipart form-data is not parsed naively to avoid incorrect metadata.
	console.warn('[Crash Metadata] Received multipart/form-data; multipart parsing is not implemented in this test server.');
	} else if (body && body.trim().length > 0) {
	// Unsupported content type; avoid naive parsing that could misinterpret data.
	console.warn('[Crash Metadata] Unsupported Content-Type for metadata parsing:', contentType);

[Copilot]

Missing copyright header. The file test_startup_cleanup.py is missing the standard Apache 2.0 copyright header that is present in other test files like test_lock_and_exit.py, test_minidump_happy_path.py, and test_coredump_happy_path.py. For consistency with the codebase, add the copyright header at the beginning of the file.

[Copilot]

Incomplete subdirectory cleanup. The cleanup only removes files within directories using Path(path).glob("*"), but this doesn't handle subdirectories. If test runs create subdirectories (which can happen with archive operations or failed tests), they won't be cleaned up. Consider using shutil.rmtree() for directory cleanup or recursively cleaning subdirectories. Example from test_lock_and_exit.py shows using shutil.rmtree() for complete directory removal.

Suggested change

	for item in Path(path).glob("*"):
	try:
	if item.is_file():
	item.unlink()
	print(f"Cleaned up: {item}")
	except Exception as e:
	print(f"Warning: Could not remove {item}: {e}")
	try:
	shutil.rmtree(path)
	print(f"Cleaned up directory and contents: {path}")
	except Exception as e:
	print(f"Warning: Could not remove directory {path}: {e}")

[Copilot]

Build script may fail silently if patches don't apply. The sed commands use the -i flag for in-place editing but don't check if the pattern was found before replacement. If the file structure changes in the common_utilities repository, the sed commands will succeed (exit code 0) even if they didn't find and replace anything. The verification steps check for the result, but if a file doesn't exist or the pattern is slightly different, the build could proceed with unpatched files. Consider adding existence checks for files before applying patches, or using grep to verify the pattern exists before attempting replacement.

[Copilot]

Code duplication across test files. The methods setup_configuration_files(), cleanup_test_environment(), setup_directories(), verify_mock_server(), run_crashupload(), and check_logs() are duplicated with nearly identical implementations across test_minidump_happy_path.py, test_coredump_happy_path.py, and test_startup_cleanup.py. This violates the DRY principle and makes maintenance harder. Consider extracting these common methods into a shared base class or utility module that all test files can inherit from or import. For example, create a test/functional-tests/tests/test_utils.py file with a CrashUploadTestBase class.

Suggested change

	def check_logs(self, log_file="/opt/logs/core_log.txt"):
	"""Read and return log file contents"""
	if os.path.exists(log_file):
	with open(log_file, 'r') as f:
	content = f.read()
	print(f"Log contents:\n{content}")
	return content
	return ""
	def check_rate_limit_file(self):
	"""Check rate limit timestamp file"""
	timestamp_file = "/tmp/.minidump_upload_timestamps"
	if os.path.exists(timestamp_file):
	with open(timestamp_file, 'r') as f:
	content = f.read()
	print(f"Rate limit timestamps:\n{content}")
	return content
	return ""
	def _read_file_if_exists(self, path, label=None):
	"""
	Helper to read a file if it exists, print its contents with an optional label,
	and return the content as a string. Returns an empty string if the file
	does not exist.
	"""
	if not os.path.exists(path):
	return ""
	with open(path, "r") as f:
	content = f.read()
	if label:
	print(f"{label}:\n{content}")
	else:
	print(content)
	return content
	def check_logs(self, log_file="/opt/logs/core_log.txt"):
	"""Read and return log file contents"""
	return self._read_file_if_exists(log_file, label="Log contents")
	def check_rate_limit_file(self):
	"""Check rate limit timestamp file"""
	timestamp_file = "/tmp/.minidump_upload_timestamps"
	return self._read_file_if_exists(timestamp_file, label="Rate limit timestamps")

[Copilot]

Potential path traversal vulnerability with inadequate sanitization. While the code attempts to sanitize the filename by removing ".." and replacing special characters, this sanitization happens after path.basename() is called. If the URL path contains encoded path traversal sequences (like %2e%2e%2f), they could bypass the regex replacement. Additionally, the regex /[^a-zA-Z0-9._-]/g could allow sequences like "..." which could still be problematic. Consider using a whitelist approach instead: validate that the filename only contains expected patterns, and reject invalid filenames rather than trying to sanitize them. Since this is a test mock server, the risk is lower, but it's still a bad practice to establish.

[Copilot]

Variable mock_data is not used.

[Copilot]

Variable rate_limit_content is not used.

Suggested change

	rate_limit_content = self.check_rate_limit_file()
	self.check_rate_limit_file()

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/crashupload/41/rdk/components/generic/crashupload

• Commit: fed6ce8

Report detail: gist'

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/crashupload/41/rdk/components/generic/crashupload

• Commit: df40017

Report detail: gist'

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The assertion logic is inverted or the condition checking is problematic. When total_files is 0, the test prints success and returns early. However, when total_files is NOT 4, the assertion fails with the message "Should keep only 4 dumps/archives, found {total_files}". This means if total_files is 1, 2, or 3, the test will fail, even though these are valid intermediate states during upload. The test should either accept any value less than or equal to 4, or should have more nuanced logic to distinguish between "files uploaded" (0 files) and "cleanup happened but some remain" (1-4 files).

Suggested change

	# Either way, we should have 4 files (dumps or archives) from the most recent
	total_files = len(remaining_dumps) + len(remaining_archives)
	# If files were uploaded and deleted, we expect 0 dumps but archives created
	# If upload failed, we expect 4 dumps kept
	if total_files == 0:
	# Files were uploaded and cleaned up - this is also valid
	print("✓ All dumps were uploaded and cleaned up (expected behavior)")
	else:
	assert total_files == 4, f"Should keep only 4 dumps/archives, found {total_files}"
	print(f"✓ Correct {total_files} dumps/archives kept (most recent)")
	# Either way, we should have at most 4 files (dumps or archives) from the most recent
	total_files = len(remaining_dumps) + len(remaining_archives)
	# If files were uploaded and deleted, we expect 0 dumps but archives created
	# If upload failed, we expect up to 4 dumps kept
	if total_files == 0:
	# Files were uploaded and cleaned up - this is also valid
	print("✓ All dumps were uploaded and cleaned up (expected behavior)")
	else:
	assert total_files <= 4, f"Should keep at most 4 dumps/archives, found {total_files}"
	print(f"✓ Acceptable number of dumps/archives kept ({total_files} ≤ 4, most recent)")

[Copilot]

Test assertions are inconsistent with the test description. The test creates 7 coredumps and claims to verify that "only 4 most recent dumps remain", but at line 428, the expected indices are "range(3, 7)" which would be indices 3, 4, 5, 6 (the 4th through 7th files created, 0-indexed). However, these are not necessarily the 4 "most recent" by modification time since files are created with sleep(0.2) between them. If cleanup is based on modification time (mtime), the test should verify based on mtime, not creation order indices.

[Copilot]

The assertion uses a strict equality check that may cause flaky test failures. If any files are in an intermediate state during processing (e.g., being archived, being uploaded), the count could be different from exactly 7. The test should be more resilient to timing issues, perhaps checking that the count is greater than or equal to some minimum value, or adding synchronization to ensure the binary has finished processing.

Suggested change

	assert len(remaining_dumps) == 7, f"All 7 coredumps should remain (NO_DUMPS_FOUND), found {len(remaining_dumps)}"
	assert len(remaining_dumps) >= 7, f"At least 7 coredumps should remain (NO_DUMPS_FOUND), found {len(remaining_dumps)}"

[Copilot]

Hardcoded path without environment variable fallback. This is inconsistent with the pattern used in other test files (test_lock_and_exit.py line 63) where 'os.environ.get('CRASHUPLOAD_BINARY', '/usr/local/bin/crashupload')' is used to allow overriding the binary path.

[Copilot]

Hardcoded path without environment variable fallback. This is inconsistent with the pattern used in other test files (test_lock_and_exit.py line 63) where 'os.environ.get('CRASHUPLOAD_BINARY', '/usr/local/bin/crashupload')' is used to allow overriding the binary path.

[Copilot]

Test has a duplicate order marker. Both 'test_minidump_upload_success' (line 525) and 'test_minidump_metadata_fields' (line 619) use '@pytest.mark.order(2)'. This will cause pytest-ordering to run these tests in an undefined order or potentially fail. Each test should have a unique order value.

Suggested change

	@pytest.mark.order(2)
	@pytest.mark.order(4)

[Copilot]

Potential path traversal vulnerability in filename sanitization. While the code attempts to sanitize the filename at lines 234-235 by removing '..' and special characters, this approach has weaknesses. The regex '/../g' only removes literal '..' sequences, but doesn't prevent other path traversal patterns like encoded characters (%2e%2e), or multiple path separators. Additionally, the sanitization happens after 'path.basename(urlPath)' is called, which already provides some protection, but the order and completeness of checks should be improved. Consider using a whitelist approach or more comprehensive sanitization.

Suggested change

	const urlPath = req.url.split('?')[0];
	let filename = path.basename(urlPath) || `crash_${uploadId}.tgz`;
	// Sanitize filename: remove path traversal sequences and special characters
	filename = filename.replace(/\.\./g, '').replace(/[^a-zA-Z0-9._-]/g, '_');
	// Ensure filename is not empty after sanitization
	if (!filename || filename.length === 0) {
	// Use URL API to safely parse and decode the pathname
	const parsedUrl = new url.URL(req.url, 'https://localhost');
	const urlPath = parsedUrl.pathname;
	// Derive a base filename from the URL path or fall back to a default
	let filename = path.basename(urlPath) || `crash_${uploadId}.tgz`;
	// Sanitize filename: restrict to a safe character set
	filename = filename.replace(/[^a-zA-Z0-9._-]/g, '_');
	// Reject unsafe or traversal-like filenames and fall back to a safe default
	if (!filename || filename === '.' || filename === '..' || filename.includes('..')) {

[Copilot]

Missing error handling for path.join operation. If either 'uploadDir' or 'filename' contain unexpected values after sanitization, path.join could fail or produce unexpected results. While unlikely given the sanitization above, defensive programming would suggest wrapping this in try-catch or validating that the resulting filepath is within the expected directory.

Suggested change

	const filepath = path.join(uploadDir, filename);
	const filepath = path.join(uploadDir, filename);
	const normalizedUploadDir = path.resolve(uploadDir) + path.sep;
	const normalizedFilepath = path.resolve(filepath);
	// Ensure the resolved filepath is within the expected upload directory
	if (!normalizedFilepath.startsWith(normalizedUploadDir)) {
	throw new Error('Resolved filepath is outside the allowed upload directory');
	}

[Copilot]

Variable result is not used.

[Copilot]

Variable rate_limit_content is not used.

Suggested change

	# Rate limit file should be updated after upload
	# Rate limit file should be updated after upload
	assert rate_limit_content is not None, "Rate limit file should be updated after upload"

[rdkcmf-jenkins]

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

• Protex Server Path: /home/blackduck/github/crashupload/41/rdk/components/generic/crashupload

• Commit: 9d6318c

Report detail: gist'

[rdkcmf-jenkins]

b'## WARNING: A Blackduck scan failure has been waived

A prior failure has been upvoted

• Upvote reason: ok

• Commit: 9d6318c
  '

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The request handlers don't include error event listeners on the request object. If an error occurs while reading the request body (e.g., client disconnects), it will cause an uncaught exception. Consider adding req.on('error', ...) handlers in both handleCrashMetadataPost and handleS3Put functions to handle potential request errors gracefully and prevent server crashes.

[Copilot]

There's an inconsistency in test ordering across test files. test_minidump_happy_path.py and test_startup_cleanup.py both start with order(0), while test_coredump_happy_path.py starts with order(1). This could cause unexpected test execution order when running multiple test files together. Consider either: 1) making all test files start from order(0) for consistency, or 2) using unique order numbers across all test files if strict cross-file ordering is required. The current approach may lead to confusion about which tests run in which order.

[Copilot]

The S3 key construction on line 180 uses metadata.filename without proper URL encoding or path sanitization. If the filename contains special characters (like spaces, slashes, or other URL-unsafe characters), the presigned URL could be malformed. Consider using encodeURIComponent() on the filename portion of the path to ensure the URL is properly formed. While this is a test mock server, proper URL encoding is important for realistic testing.

[Copilot]

The requests.get() call on line 108-112 has verify=False which disables SSL certificate verification. While this is acceptable for a test environment with self-signed certificates, it triggers an InsecureRequestWarning from urllib3. Consider adding import urllib3 and urllib3.disable_warnings(urllib3.exceptions.InsecureRequestWarning) at the module level to suppress these warnings in test output, or use warnings.filterwarnings('ignore', message='Unverified HTTPS request') for cleaner test logs.

[Copilot]

The form data parsing on lines 150-155 is overly simplistic and doesn't properly handle multipart/form-data (despite the comment claiming it does). The code only handles application/x-www-form-urlencoded format by splitting on '&' and '='. For multipart/form-data (which is commonly used for file uploads), a proper parser is needed. While this may work for the current test scenarios, the comment is misleading. Either remove the multipart/form-data reference from the comment, or implement proper multipart parsing using a library like busboy or formidable.

[Copilot]

Variable rate_limit_content is not used.

Suggested change

	# Rate limit file should be updated after upload
	# Rate limit file should be updated after upload
	assert rate_limit_content is not None, "Rate limit tracking file was not updated"

[Copilot]

Variable initial_dump_path is not used.

Suggested change

	initial_dump_path = Path(dump_file)
	initial_dump_path = Path(dump_file)
	print(f"Initial coredump path: {initial_dump_path}")

[Copilot]

Variable mock_data is not used.

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The file ends with excessive blank lines (lines 721-722 are both blank). Python convention (PEP 8) is to have exactly one blank line at the end of the file. Remove the extra blank line.

Suggested change

[Copilot]

Using verify=False with requests will generate InsecureRequestWarning messages. For test code, it's better to suppress these warnings explicitly using urllib3.disable_warnings() or warnings.filterwarnings('ignore') at the module level to keep test output clean. This pattern is commonly used in test suites that interact with mock servers using self-signed certificates.

[Copilot]

This diagnostic test attempts to compile and link C code dynamically, which has several issues:

1. Assumes gcc and development tools are available in the test environment
2. Assumes the common_utilities library can be linked (may not be exported properly)
3. May fail in CI/CD environments without compilers

Consider either:

• Making this test optional (skip if compilation fails without treating it as an error)
• Using a simpler approach that doesn't require compilation
• Moving this to a separate diagnostic script outside the main test suite

The test already has fallback logic to handle compilation failure, but it might be cleaner to skip the compilation attempt entirely if the goal is just to inspect the binary.

[Copilot]

The sys module is imported but never used in this file. Remove the unused import to keep the code clean.

Suggested change

import sys

[Copilot]

The subprocess.run call for executing crashupload does not specify a timeout parameter. This could cause tests to hang indefinitely if the binary encounters an issue.

The other test files (test_minidump_happy_path.py and test_coredump_happy_path.py) use timeout=120 for crashupload execution. This should be added here for consistency and robustness:

result = subprocess.run(
    cmd,
    stdout=subprocess.PIPE,
    stderr=subprocess.PIPE,
    text=True,
    timeout=120
)

Suggested change

	text=True
	text=True,
	timeout=120

[Copilot]

Variable result is not used.

Suggested change

	result = self.run_crashupload(dump_type="1", upload_flag="secure")
	self.run_crashupload(dump_type="1", upload_flag="secure")

[Copilot]

Variable initial_dump_path is not used.

Suggested change

	initial_dump_path = Path(dump_file)

[Copilot]

Variable result is not used.

Suggested change

	result = self.run_crashupload(dump_type="1", upload_flag="secure")
	self.run_crashupload(dump_type="1", upload_flag="secure")

[Copilot]

Variable dump_file is not used.

Suggested change

	dump_file = self.create_test_coredump("mutex_test_core.prog.4000.1706381100.gz", size_kb=35)
	self.create_test_coredump("mutex_test_core.prog.4000.1706381100.gz", size_kb=35)

[Copilot]

Variable result is not used.

Suggested change

	result = self.run_crashupload(dump_type="1", upload_flag="secure")
	self.run_crashupload(dump_type="1", upload_flag="secure")

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The rm -rf common_utilities command on line 138 is potentially dangerous if the script is run from an unexpected directory or if the directory path is somehow manipulated. Consider adding safety checks:

1. Verify we're in the expected ROOT directory before removing
2. Use rm -rf "${ROOT}/common_utilities" with quotes around the variable
3. Add a check that ROOT is not empty or root directory

Example:

if [ -n "$ROOT" ] && [ "$ROOT" != "/" ] && [ -d "$ROOT/common_utilities" ]; then
    rm -rf "$ROOT/common_utilities"
fi

Suggested change

	if [ -d "common_utilities" ]; then
	echo "Removing existing common_utilities directory..."
	rm -rf common_utilities
	if [ -n "$ROOT" ] && [ "$ROOT" != "/" ] && [ -d "$ROOT/common_utilities" ]; then
	echo "Removing existing common_utilities directory..."
	rm -rf "$ROOT/common_utilities"

[Copilot]

The git clone command on line 141 has no error handling. If the clone fails (network issues, branch doesn't exist, authentication problems), the script will continue to line 142 (cd common_utilities) which will fail, potentially changing to an unexpected directory or failing silently. Add error checking:

if ! git clone https://github.com/rdkcentral/common_utilities.git -b feature/upload_L2; then
    echo "ERROR: Failed to clone common_utilities"
    exit 1
fi

[Copilot]

The test creates a C program as a string but uses incorrect escape sequences. The printf format strings contain \\n which will be written as literal backslash-n in the file, not as escape sequences. When the C compiler processes this, it should be \n (single backslash). The current code has double backslashes which means the test program will print literal '\n' characters instead of newlines. Change \\n to \n in the test_program string.

Suggested change

	printf("Testing buffer size thresholds:\\n");
	for (int size = 1020; size <= 1030; size++) {
	memset(buffer, 0, sizeof(buffer));
	result = getDevicePropertyData("S3_AMAZON_SIGNING_URL", buffer, size);
	printf("buff_size=%d: result=%d\\n", size, result);
	}
	// Also test with larger sizes
	for (int size = 2040; size <= 2050; size++) {
	memset(buffer, 0, sizeof(buffer));
	result = getDevicePropertyData("S3_AMAZON_SIGNING_URL", buffer, size);
	printf("buff_size=%d: result=%d\\n", size, result);
	printf("Testing buffer size thresholds:\n");
	for (int size = 1020; size <= 1030; size++) {
	memset(buffer, 0, sizeof(buffer));
	result = getDevicePropertyData("S3_AMAZON_SIGNING_URL", buffer, size);
	printf("buff_size=%d: result=%d\n", size, result);
	}
	// Also test with larger sizes
	for (int size = 2040; size <= 2050; size++) {
	memset(buffer, 0, sizeof(buffer));
	result = getDevicePropertyData("S3_AMAZON_SIGNING_URL", buffer, size);
	printf("buff_size=%d: result=%d\n", size, result);

[Copilot]

The test accepts exit code 255 as a valid outcome due to a known bug, but this makes the test less effective at catching regressions. If the binary is updated to fix the buffer size bug, this test will still pass with exit code 255, masking potential new issues. Consider:

1. Adding an environment variable or flag to indicate whether the bug is present in the build
2. Failing the test with exit code 255 once the bug is fixed
3. Using pytest.skip() with a clear message when the known bug is detected, rather than treating it as a pass

This would make the test suite more maintainable and help ensure the bug fix is properly validated.

[Copilot]

The cleanup logic in teardown doesn't verify that files were successfully deleted. If cleanup fails (due to permissions, open file handles, etc.), subsequent tests might fail or behave unexpectedly due to leftover files. Consider:

1. Adding assertions in cleanup to ensure critical files/directories are removed
2. Using a more robust cleanup approach like shutil.rmtree() with ignore_errors=False and proper exception handling
3. At minimum, logging warnings when cleanup fails so test failures can be diagnosed

The current implementation silently continues if cleanup fails, which could lead to hard-to-debug test interactions.

[Copilot]

Variable rate_limit_content is not used.

Suggested change

	rate_limit_content = self.check_rate_limit_file()
	self.check_rate_limit_file()

[Copilot]

Variable dump_file is not used.

Suggested change

	dump_file = self.create_test_minidump("test_metadata.dmp", size_kb=5)
	self.create_test_minidump("test_metadata.dmp", size_kb=5)

[Copilot]

Variable result is not used.

Suggested change

	result = self.run_crashupload(dump_type="0", upload_flag="secure")
	self.run_crashupload(dump_type="0", upload_flag="secure")

[Copilot]

Variable initial_dump_path is not used.

Suggested change

initial_dump_path = Path(dump_file)

[Copilot]

Variable result is not used.

Suggested change

	result = self.run_crashupload(dump_type="0", upload_flag="secure")
	self.run_crashupload(dump_type="0", upload_flag="secure")

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Missing copyright header. All other test files in the test/functional-tests/tests directory include the standard Apache License 2.0 copyright header block. This file should include the same header for consistency and proper licensing.

[Copilot]

Significant code duplication across test files. The setup_configuration_files, setup_directories, cleanup_test_environment, verify_mock_server, and other helper methods are duplicated across test_minidump_happy_path.py, test_coredump_happy_path.py, and test_startup_cleanup.py. Consider extracting these into a shared base class or conftest.py fixture to improve maintainability and reduce code duplication. This would make it easier to update configuration logic in one place when needed.

[Copilot]

Test creates temporary files that may not be cleaned up. The test_verify_buffer_sizes diagnostic test creates temporary files (/tmp/test_buffer_size.c and /tmp/test_buffer_size executable) but doesn't ensure they are cleaned up after the test, even when compilation fails or the test exits early. Consider adding cleanup logic in a try-finally block or relying on the fixture's teardown method to remove these temporary test artifacts.

Suggested change

	# Run the test program
	run_result = subprocess.run(
	["/tmp/test_buffer_size"],
	capture_output=True,
	text=True,
	timeout=10
	)
	print(f"Test output:\n{run_result.stdout}")
	if run_result.stderr:
	print(f"stderr:\n{run_result.stderr}")
	# Analyze results to find the threshold
	lines = run_result.stdout.split('\n')
	threshold_found = None
	for line in lines:
	if "result=" in line:
	parts = line.split()
	for part in parts:
	if part.startswith("buff_size="):
	size = int(part.split('=')[1].rstrip(':'))
	if part.startswith("result="):
	result_val = int(part.split('=')[1])
	if result_val == 0 and threshold_found is None:
	# First success
	pass
	elif result_val != 0:
	# Found the threshold where it starts failing
	threshold_found = size
	if threshold_found:
	print(f"\n✓ Buffer size threshold detected: {threshold_found}")
	print(f" MAX_DEVICE_PROP_BUFF_SIZE is likely: {threshold_found}")
	else:
	print("\n⚠ Could not determine exact threshold from test")
	try:
	# Run the test program
	run_result = subprocess.run(
	["/tmp/test_buffer_size"],
	capture_output=True,
	text=True,
	timeout=10
	)
	print(f"Test output:\n{run_result.stdout}")
	if run_result.stderr:
	print(f"stderr:\n{run_result.stderr}")
	# Analyze results to find the threshold
	lines = run_result.stdout.split('\n')
	threshold_found = None
	for line in lines:
	if "result=" in line:
	parts = line.split()
	for part in parts:
	if part.startswith("buff_size="):
	size = int(part.split('=')[1].rstrip(':'))
	if part.startswith("result="):
	result_val = int(part.split('=')[1])
	if result_val == 0 and threshold_found is None:
	# First success
	pass
	elif result_val != 0:
	# Found the threshold where it starts failing
	threshold_found = size
	if threshold_found:
	print(f"\n✓ Buffer size threshold detected: {threshold_found}")
	print(f" MAX_DEVICE_PROP_BUFF_SIZE is likely: {threshold_found}")
	else:
	print("\n⚠ Could not determine exact threshold from test")
	finally:
	# Ensure temporary test artifacts are cleaned up
	for tmp_path in ("/tmp/test_buffer_size.c", "/tmp/test_buffer_size"):
	if os.path.exists(tmp_path):
	try:
	os.remove(tmp_path)
	except OSError as e:
	print(f"⚠ Failed to remove temporary file {tmp_path}: {e}")

[Copilot]

Hardcoded 3-second sleep may cause test flakiness or unnecessary delays. The comment indicates this is for file system stability checks, but 3 seconds is arbitrary and may be: 1) too short on slow systems, causing intermittent failures, or 2) too long on fast systems, unnecessarily slowing down the test suite. Consider: 1) Polling the file size with a timeout to detect when it stabilizes, or 2) Making this configurable via environment variable, or 3) Reducing to the minimum necessary (comment mentions 2 consecutive 1-second intervals, so 2.1 seconds should suffice). This is duplicated from the minidump test and should be addressed consistently.

Suggested change

	# The scanner checks for size stability over 2 consecutive 1-second intervals
	time.sleep(3)
	# The scanner checks for size stability over 2 consecutive 1-second intervals.
	# Default to 2.1 seconds (just over the required 2 seconds), but allow override
	# via environment for slow/fast systems and CI tuning.
	settle_seconds_default = 2.1
	settle_seconds_env = os.getenv("CRASHUPLOAD_FS_SETTLE_SECONDS")
	if settle_seconds_env is not None:
	try:
	settle_seconds_default = float(settle_seconds_env)
	except ValueError:
	# Ignore invalid values and keep the safe default
	pass
	time.sleep(settle_seconds_default)

[Copilot]

Unused import. The 'requests' module is imported but never used in the code. The verify_mock_server method uses subprocess to call curl instead. Remove this unused import to keep the dependencies clean.

[Copilot]

Variable rate_limit_content is not used.

Suggested change

	# Rate limit file should be updated after upload
	# Rate limit file should be updated after upload
	assert rate_limit_content, "Rate limit file should be updated and contain data after upload"

[Copilot]

Variable dump_file is not used.

Suggested change

	dump_file = self.create_test_minidump("test_metadata.dmp", size_kb=5)
	self.create_test_minidump("test_metadata.dmp", size_kb=5)

[Copilot]

Variable result is not used.

Suggested change

	result = self.run_crashupload(dump_type="0", upload_flag="secure")
	self.run_crashupload(dump_type="0", upload_flag="secure")

[Copilot]

Variable initial_dump_path is not used.

Suggested change

initial_dump_path = Path(dump_file)

[Copilot]

Variable result is not used.

Suggested change

	result = self.run_crashupload(dump_type="0", upload_flag="secure")
	self.run_crashupload(dump_type="0", upload_flag="secure")

[Copilot]

Pull request overview

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

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Test complexity and maintainability concern. Similar to test_minidump_upload_success, this test has grown complex with multiple conditional branches and special case handling (lines 427-471). The test uses pytest.skip() for a known environment issue which could mask real failures. Consider refactoring this test to be more focused, or use pytest.mark.xfail with strict=False for known issues instead of skipping entirely.

[Copilot]

Missing cleanup_pytest_cache fixture. The existing test files in this repository include a session-scoped fixture to clean up pytest cache directories after all tests complete. This new test file should include the same fixture for consistency and to avoid accumulation of cache directories.

[Copilot]

Code duplication: The setup_configuration_files, verify_mock_server, verify_archive_created, verify_upload_to_mock_server, query_mock_server_admin, check_logs, and check_rate_limit_file methods are nearly identical across test_minidump_happy_path.py and test_coredump_happy_path.py. This duplication makes maintenance harder and increases the risk of inconsistencies. Consider extracting these common methods into a base test class or shared utility module that both test classes can inherit from or import.

[Copilot]

Test complexity and maintainability concern. The test_minidump_upload_success method has grown excessively complex with multiple nested conditional branches, early returns, and special case handling (lines 639-726). This makes the test difficult to understand, maintain, and debug. The test tries to handle too many edge cases and workarounds for known bugs in the test environment. Consider splitting this into multiple focused test cases, each testing a specific scenario (success path, property reading failure, no logs case, etc.), or creating helper methods to reduce complexity.

Suggested change

	if not logs:
	print("⚠ WARNING: No logs found at /opt/logs/core_log.txt")
	print(f"\nSTDOUT from crashupload:\n{result.stdout}")
	print(f"\nSTDERR from crashupload:\n{result.stderr}")
	# Check for obvious errors in output
	has_errors = any(err in result.stdout.lower() or err in result.stderr.lower()
	for err in ['error', 'failed', 'exception', 'segmentation fault', 'core dump'])
	# Check if original dump file still exists
	dump_still_exists = os.path.exists(dump_file)
	print(f"\nOriginal dump file still exists: {dump_still_exists}")
	# For exit code 1 with no errors and file processed/cleaned, consider it success
	if result.returncode == 1 and not has_errors and not dump_still_exists:
	print("✓ Exit code 1 but no errors detected and file was processed")
	print("✓ This is acceptable in test environment - binary may exit normally with code 1")
	print("✓ Test PASSED: crashupload ran without errors")
	return
	# For exit code 1, check if it's just "no files to process"
	if result.returncode == 1:
	def _verify_post_upload_state():
	"""Verify post-upload behavior: mock server state, logs, archive, and rate limiting."""
	# Step 6: Query mock server for upload data
	mock_data = self.query_mock_server_admin()
	# Step 7: Verify upload occurred
	if upload_success:
	assert len(uploaded_files) > 0, "At least one file should be uploaded"
	print(f"✓ Upload verified: {len(uploaded_files)} file(s) uploaded")
	# Step 8: Check logs for success indicators
	logs = self.check_logs()
	# If we reached here, archive was created or test already passed with early return
	if archive_created and len(archives) > 0:
	print(f"✓ Archive created successfully: {archives[0].name}")
	print(f" Size: {archives[0].stat().st_size} bytes")
	print("✓ Test PASSED: Core crashupload functionality verified")
	# Step 9: Verify rate limit tracking
	rate_limit_content = self.check_rate_limit_file()
	# Rate limit file should be updated after upload
	print("\n========== TEST PASSED: Minidump Upload Success ==========")
	_verify_post_upload_state()

[Copilot]

Extensive workarounds for known bugs. The test files contain extensive documentation and workarounds for a known bug in the test environment (buffer size validation in common_utilities). While documenting known issues is good practice, the tests have become overly complex to handle this single bug. Consider either: 1) Fixing the underlying bug in the test build (changing the validation from '>=' to '>' in rdk_fwdl_utils.c line 258, as documented), or 2) Creating a separate test suite that assumes the bug exists, and another that assumes it's fixed, rather than having every test handle both scenarios.

Suggested change

	Functional test for crashupload - Secure Minidump Upload Happy Path
	Tests the complete end-to-end upload flow with S3 presigned URL
	KNOWN ISSUE IN TEST ENVIRONMENT:
	The test binary has a bug in common_utilities/rdk_fwdl_utils.c getDevicePropertyData():
	- Buffer size validation uses '>=' instead of '>'
	- MAX_DEVICE_PROP_BUFF_SIZE = 1024
	- crashportalEndpointUrl buffer = 1024 bytes
	- Validation fails: 1024 >= 1024 (should be 1024 > 1024)
	- Result: S3_AMAZON_SIGNING_URL cannot be read from device.properties
	- Workaround: Tests validate archive creation instead of full upload flow
	- Fix: Change validation in rdk_fwdl_utils.c line 258 from '>=' to '>'
	OR change crashportalEndpointUrl size to 1023 or 2048
	This bug does not exist in production STB builds.
	Functional test for crashupload - secure minidump upload happy path.
	This test verifies the end-to-end minidump upload flow using an S3-style
	presigned URL, as implemented in the test environment. Note that behavior
	in the test harness may differ slightly from production STB builds, but
	the functional expectations of this test remain the same.

[Copilot]

Variable dump_file is not used.

Suggested change

	# Check if crashupload even attempted to process files
	self.create_test_minidump("test_metadata.dmp", size_kb=5)

[Copilot]

Variable result is not used.

Suggested change

	"crashupload exited with code 0 but no archive was created, no logs were found, "
	self.run_crashupload(dump_type="0", upload_flag="secure")

[Copilot]

Variable initial_dump_path is not used.

Suggested change

@pytest.mark.order(3)

[Copilot]

Variable result is not used.

Suggested change

	Test: Verify correct metadata sent to mock server
	self.run_crashupload(dump_type="0", upload_flag="secure")

[Copilot]

This build script automatically fetches and executes code from the external rdkcentral/common_utilities repository on the mutable feature/upload_L2 branch (rm -rf common_utilities followed by git clone ... and sh cov_build.sh), which creates a supply-chain risk. If that branch or repo is compromised, an attacker can inject malicious code into your dependency and have it compiled/executed with access to your build environment and artifacts. To mitigate this, pin the dependency to an immutable reference (e.g., a specific commit SHA or signed release) and verify its integrity before executing its build scripts.