VTS L3 Test Framework Enhancement

[Abhishek-0412]

No description provided.

[Copilot]

🟡 Changes recommended

The updated L3 executor has confirmed runtime-breaking issues (missing log_to_excel module import and several stream/cleanup logic defects) that must be fixed before it can be safely approved.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR enhances the VTS L3 execution flow by adding stream download/cleanup automation and platform-specific player config handling, and aligns VTS package installation/layout with updated module directory names for L1/L2 execution.

Changes:

• Updated VTS binary base paths/config names for rmfAudioCapture, PowerManager, and DeepSleep modules.
• Enhanced vts_l3_executor.py with SSH-based stream management and utPlayerConfig.yml vendor block updates, plus updated reporting flow.
• Extended InstallVTSPackage.sh to set up module directories/symlinks (and preserve vts_version.txt) after extracting the VTS package.

File summaries

File	Description
framework/fileStore/VTSTestVariables.py	Aligns module base paths/configs and adds warnings about common list/skiplist edits impacting all platforms.
framework/fileStore/VTS_L3/vts_l3_executor.py	Adds L3 automation for stream handling and utPlayerConfig updates; updates execution/reporting pipeline.
framework/fileStore/VTS_L3/vts_common_config.py	Derives SOC_VENDOR from CPE_PLATFORM and introduces PLATFORM_EXPORTS hook for vendor prerequisites.
framework/fileStore/InstallVTSPackage.sh	Adds module setup helper to create/move expected module directories and preserves version file during extraction.

Review details

Suppressed comments (2)

framework/fileStore/VTS_L3/vts_l3_executor.py:290

• already_pattern is assigned twice and the log message still says testCleanSingleAsset(), but the regex now targets testCleanAssets(). This can lead to misleading "already patched" output.

    already_pattern = r'def\s+testCleanSingleAsset\s*\(\s*self\s*\)\s*:\s*[\s\S]*?print\(\s*[\'"]Cleanup handled by external framework[\'"]\s*\)\s*[\s\S]*?return'
    already_pattern = r'def\s+testCleanAssets\s*\(\s*self\s*\)\s*:\s*[\s\S]*?print\(\s*[\'"]Cleanup handled by external framework[\'"]\s*\)\s*[\s\S]*?return'
    if re.search(already_pattern, content):
        print(f"✅ Already patched: testCleanSingleAsset() in {file_path}")
        return True

framework/fileStore/VTS_L3/vts_l3_executor.py:973

• The delete path uses rm -f {stream_base}{s} and does not quote remote_dir/s. For deletes, stream_base should not be prefixed (it is a URL base), and quoting avoids shell interpretation issues.

                s = os.path.basename(s)
                cmd = (
                    f'cd {remote_dir} && '
                    f'rm -f {stream_base}{s}'
                )

• Files reviewed: 5/5 changed files
• Comments generated: 11
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

There are confirmed functional issues in stream cleanup/normalization return values and install-module setup logic, plus unquoted SSH command construction that poses correctness and injection risks.

Review details

Suppressed comments (8)

Previously missed (3) — in code that hasn't changed since the last review.

framework/fileStore/VTS_L3/vts_l3_executor.py:692

• get_normalized_streams_for_target() builds normalized but never returns it, so callers get None (e.g., the preview printed in main()).
  framework/fileStore/VTS_L3/vts_l3_executor.py:40
• Avoid from log_to_excel import * here; only process_log_to_excel() is used, and star-imports make it harder to reason about names and can shadow builtins/imports.
  framework/fileStore/VTS_L3/vts_l3_executor.py:978
• This timeout message contains garbled characters (likely an encoding artifact) which will render inconsistently in terminals/logs.

framework/fileStore/VTS_L3/vts_l3_executor.py:282

• func_pattern/already_pattern are assigned twice (the first value is overwritten) and the log messages still refer to testCleanSingleAsset even though the regex ends up matching testCleanAssets. This makes the patcher misleading and harder to debug; it should consistently use one target function name.

    # ✅ Check function exists
    func_pattern = r'def\s+testCleanSingleAsset\s*\(\s*self\s*\)\s*:'
    func_pattern = r'def\s+testCleanAssets\s*\(\s*self\s*\)\s*:'
    if not re.search(func_pattern, content):
        print(f"❌ testCleanSingleAsset(self) not found in {file_path}")

framework/fileStore/VTS_L3/vts_l3_executor.py:1114

• cleanup_streams_for_target() computes files_to_delete (after applying the rename map/rules) but then calls _download_delete_streams() with the original streams list, so the rename logic is ignored during deletion. Also, when remove_dir=True the function currently only changes log messaging but never removes the directory.

    if verbose:
        if remove_dir:
            print(f"[streams-clean] Removing files and directory for '{target}' at {remote_dir} on {device_ip} ...")
        else:
            print(f"[streams-clean] Removing {len(files_to_delete)} files for '{target}' at {remote_dir} on {device_ip} ...")

    _download_delete_streams(False, streams, remote_dir, device_ip, ssh_port, ssh_user, ssh_password, target, "", timeout=30, stop_on_failure=False)

framework/fileStore/VTS_L3/vts_l3_executor.py:967

• The SSH commands constructed for stream download/delete do not quote remote_dir or the URL/filename. This can break when paths contain special characters and also makes command injection possible if a stream name is ever untrusted (it comes from YAML).

                cmd = (
                    f'cd {remote_dir} && '
                    f'curl -sS -L --retry 3 --retry-connrefused -O {stream_path}'
                )
                print(cmd)

framework/fileStore/VTS_L3/vts_l3_executor.py:1256

• add_platform_config_if_missing() verifies presence using the literal string 'platform:' and returns platform in content, which can produce false positives/negatives. It should check for the actual YAML key ("<platform>:") consistently.

    print(f"{platform} present: {'platform:' in content}")
    return (platform in content)

framework/fileStore/InstallVTSPackage.sh:60

• In setup_vts_module, the special-case _new_dir assignments are overwritten immediately, and the mv path returns 1 even when the move succeeds. This makes success look like failure and hides real errors.

    # Special-case: power_manager source dir maps to "power" module name
    if [ "$_src_module" = "power_manager" ] || [ "$_src_module" = "deepsleep_manager" ]; then
        _new_dir="${_vts_root}/${_src_module%_manager}"
    fi
    if [ "$_src_module" = "rmf_audio_capture" ]; then
        _new_dir="${_vts_root}/rmfaudiocapture"
    fi

    _src_dir="${_vts_root}/${_src_module}"
    _new_dir="${_vts_root}/${_new_module}"

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

There are confirmed functional bugs in the updated installer logic and in the L3 executor’s stream cleanup/patching logic that can cause failures or incorrect behavior at runtime.

Review details

Suppressed comments (14)

Previously missed (5) — in code that hasn't changed since the last review.

framework/fileStore/InstallVTSPackage.sh:148

• InstallVTSPackage.sh: ${root_dir}VTS_Package / ${root_dir}vts_installed rely on root_dir ending with '/'. If root_dir is changed (or passed in), these paths break. Prefer ${root_dir}/... and quote the argument to avoid word-splitting.
  framework/fileStore/VTS_L3/vts_common_config.py:45
• vts_common_config.py: SOC_VENDOR is set to CPE_PLATFORM.lower(), but CPE_PLATFORM defaults to an empty string and most vtsconfig_* modules don't override it. Callers that expect a valid vendor (e.g. update_ut_player_config) will see '' and fail; provide a non-empty default or require CPE_PLATFORM to be set.
  framework/fileStore/VTS_L3/vts_l3_executor.py:720
• rewrite_testsetup_yaml_streams_with_renames() has the same AttributeError risk as get_normalized_streams_for_target(): it dereferences config.STREAM_RENAME_MAP_BY_MODULE / STREAM_RENAME_RULES_BY_MODULE without guarding for configs that don't define them.

This issue also appears on line 808 of the same file.
framework/fileStore/VTS_L3/vts_l3_executor.py:1300

• update_ut_player_config(): when vendor is empty (common if SOC_VENDOR isn't configured), this code raises and aborts --update-config. Consider treating empty vendor as "not configured" (warn + skip) instead of raising.
  framework/fileStore/VTS_L3/vts_l3_executor.py:40
• Avoid wildcard import from log_to_excel; only process_log_to_excel() is used. Wildcard imports make it harder to track dependencies and can unintentionally shadow local names.

framework/fileStore/VTS_L3/vts_l3_executor.py:282

• In patch_testCleanSingleAsset_skip_cleanup(), func_pattern/already_pattern are assigned twice (testCleanSingleAsset then overwritten by testCleanAssets), but the error/"already patched" messages still reference testCleanSingleAsset. This makes the patch logic and diagnostics inconsistent.

    # ✅ Check function exists
    func_pattern = r'def\s+testCleanSingleAsset\s*\(\s*self\s*\)\s*:'
    func_pattern = r'def\s+testCleanAssets\s*\(\s*self\s*\)\s*:'
    if not re.search(func_pattern, content):

framework/fileStore/VTS_L3/vts_l3_executor.py:973

• In _download_delete_streams() the delete path builds rm -f {stream_base}{s} even though it has already cd'd into remote_dir. If stream_base is a URL/prefix this will try to delete a non-existent path. Also remote_dir / filename should be quoted to avoid shell parsing issues.

                s = os.path.basename(s)
                cmd = (
                    f'cd {remote_dir} && '
                    f'rm -f {stream_base}{s}'
                )

framework/fileStore/VTS_L3/vts_l3_executor.py:979

• _download_delete_streams() checks exit_status == -1 for timeouts, but _run_shell_cmd() never returns -1; it raises TimeoutError. As written, a timeout will crash the run instead of being recorded as a failure.

            timeout=90
            exit_status, out = _run_shell_cmd(session, cmd, timeout=timeout)
            if exit_status == -1:
                print(f"[streams] TIMEOUT (or unparsed output) for {fname} □~@~T command may still be running on device")
                failures.append(fname)

framework/fileStore/VTS_L3/vts_l3_executor.py:1072

• cleanup_streams_for_target() directly accesses config.STREAM_RENAME_MAP / config.STREAM_RENAME_RULES, but some configs (e.g. vtsconfig_dsVideoDevice.py, vtsconfig_rmfaudiocapture.py) don't define them, causing AttributeError during cleanup.

    mapping = config.STREAM_RENAME_MAP
    rules   = config.STREAM_RENAME_RULES

framework/fileStore/VTS_L3/vts_l3_executor.py:1209

• add_platform_config_if_missing() docstring says it returns True if the platform is present after the operation, but when the platform already exists it returns False. That makes callers interpret "already present" as failure.

    if platform_exists:
        print(f"{platform} player config already exists")
        return False

framework/fileStore/VTS_L3/vts_l3_executor.py:1256

• add_platform_config_if_missing() verifies presence using 'platform:' in content and returns (platform in content), both of which can give incorrect results (e.g., matching a value or the literal word 'platform'). This should check for the actual YAML key <platform>:.

    print(f"{platform} present: {'platform:' in content}")
    return (platform in content)

framework/fileStore/InstallVTSPackage.sh:55

• setup_vts_module(): the special-case _new_dir assignments are immediately overwritten by _new_dir="${_vts_root}/${_new_module}", so the special-case code has no effect (and is misleading).

    # Special-case: power_manager source dir maps to "power" module name
    if [ "$_src_module" = "power_manager" ] || [ "$_src_module" = "deepsleep_manager" ]; then
        _new_dir="${_vts_root}/${_src_module%_manager}"
    fi
    if [ "$_src_module" = "rmf_audio_capture" ]; then

framework/fileStore/InstallVTSPackage.sh:76

• setup_vts_module(): after successfully mv-ing the special modules, the function returns 1 (error). That can cause callers/CI to treat a successful install as failure.

    if [ "$_src_module" = "power_manager" ] || [ "$_src_module" = "deepsleep_manager" ] || [ "$_src_module" = "rmf_audio_capture" ];then
        mv "$_src_dir" "$_new_dir"
        return 1

framework/fileStore/VTS_L3/vts_l3_executor.py:810

• startSession() uses AutoAddPolicy(), which disables SSH host key verification and is vulnerable to MITM in untrusted networks. Consider loading known_hosts and rejecting unknown keys, or making this behavior configurable.

    try:
        client = paramiko.SSHClient()
        client.set_missing_host_key_policy(paramiko.AutoAddPolicy())

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Multiple confirmed runtime-breaking issues in the new/modified execution paths (vendor defaulting, SSH stream command handling, and stream lookup using script paths) need fixes before this can be safely run.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (13)

Previously missed (5) — in code that hasn't changed since the last review.

framework/fileStore/VTS_L3/vts_l3_executor.py:693

• get_normalized_streams_for_target() builds a normalized list but never returns it, so callers (e.g. the preview in main) receive None.
  framework/fileStore/VTS_L3/vts_l3_executor.py:723
• rewrite_testsetup_yaml_streams_with_renames() uses the same non-existent config.STREAM_RENAME_*_BY_MODULE fallback as get_normalized_streams_for_target(), which will crash if STREAM_RENAME_MAP/RULES are missing.
  framework/fileStore/VTS_L3/vts_l3_executor.py:973
• The SSH cleanup command uses rm -f {stream_base}{s} without quoting; since the code already cds into remote_dir, it should delete the local filename and protect against special characters by quoting and using --.
  framework/fileStore/VTS_L3/vts_l3_executor.py:1602
• run_interactive_with_logging() passes script (often a path) into get_streams_for_testfile(), which searches by filename in os.walk(); if config.TEST_SCRIPT is a path, this will fail to locate the file. Pass the basename instead.
  framework/fileStore/VTS_L3/vts_l3_executor.py:41
• Wildcard-importing from log_to_excel makes it harder to see which API is used here and risks name collisions; only process_log_to_excel is referenced in this file.

framework/fileStore/VTS_L3/vts_l3_executor.py:283

• patch_testCleanSingleAsset_skip_cleanup() overwrites func_pattern/already_pattern and still logs the old function name; this can cause false negatives and confusing output. It should support both helper function names (testCleanSingleAsset/testCleanAssets) consistently.

    # ✅ Check function exists
    func_pattern = r'def\s+testCleanSingleAsset\s*\(\s*self\s*\)\s*:'
    func_pattern = r'def\s+testCleanAssets\s*\(\s*self\s*\)\s*:'
    if not re.search(func_pattern, content):
        print(f"❌ testCleanSingleAsset(self) not found in {file_path}")

framework/fileStore/VTS_L3/vts_l3_executor.py:966

• The SSH download command does not quote remote_dir or stream_path; paths/URLs containing special characters (spaces, ?, &, etc.) will break the command.

                cmd = (
                    f'cd {remote_dir} && '
                    f'curl -sS -L --retry 3 --retry-connrefused -O {stream_path}'
                )

framework/fileStore/VTS_L3/vts_l3_executor.py:983

• _download_delete_streams() checks exit_status == -1 for timeouts, but _run_shell_cmd() raises TimeoutError instead. As written, timeouts will raise and bypass failure collection, and the exit_status == -1 branch is unreachable.

            timeout=90
            exit_status, out = _run_shell_cmd(session, cmd, timeout=timeout)
            if exit_status == -1:
                print(f"[streams] TIMEOUT (or unparsed output) for {fname} □~@~T command may still be running on device")
                failures.append(fname)
            elif exit_status != 0:
                print(f"[streams] FAILED: {fname} (exit {exit_status})")
                failures.append(fname)

framework/fileStore/VTS_L3/vts_l3_executor.py:1114

• cleanup_streams_for_target() computes a normalized files_to_delete list but then passes the original streams list to _download_delete_streams(), so the normalization work is ignored and the wrong filenames may be deleted.

            print(f"[streams-clean] Removing {len(files_to_delete)} files for '{target}' at {remote_dir} on {device_ip} ...")

    _download_delete_streams(False, streams, remote_dir, device_ip, ssh_port, ssh_user, ssh_password, target, "", timeout=30, stop_on_failure=False)

framework/fileStore/VTS_L3/vts_l3_executor.py:1196

• add_platform_config_if_missing(): the Returns docstring says the function returns True when the platform is present after the operation, but the implementation returns False when the platform already exists. This is misleading for callers and future maintenance.

    Returns:
        bool: True if the platform is present after the operation, False otherwise.
    """

framework/fileStore/VTS_L3/vts_l3_executor.py:1256

• add_platform_config_if_missing() prints and returns using the literal string 'platform:' rather than the actual vendor key, and return (platform in content) can be a false positive if the vendor appears elsewhere in the file. Track presence using the top-level key string and return that boolean.

    print(f"{platform} present: {'platform:' in content}")
    return (platform in content)

framework/fileStore/InstallVTSPackage.sh:60

• setup_vts_module(): the special-casing of _new_dir is dead code because _new_dir is unconditionally overwritten immediately after. This makes the function harder to reason about and suggests the special-case behavior isn't actually applied.

    # Special-case: power_manager source dir maps to "power" module name
    if [ "$_src_module" = "power_manager" ] || [ "$_src_module" = "deepsleep_manager" ]; then
        _new_dir="${_vts_root}/${_src_module%_manager}"
    fi
    if [ "$_src_module" = "rmf_audio_capture" ]; then
        _new_dir="${_vts_root}/rmfaudiocapture"
    fi

    _src_dir="${_vts_root}/${_src_module}"
    _new_dir="${_vts_root}/${_new_module}"

framework/fileStore/InstallVTSPackage.sh:77

• setup_vts_module(): for modules that are moved with mv, the function returns 1 even if the mv succeeds. This can silently signal failure to callers and is inconsistent with the rest of the function's return codes.

    if [ "$_src_module" = "power_manager" ] || [ "$_src_module" = "deepsleep_manager" ] || [ "$_src_module" = "rmf_audio_capture" ];then
        mv "$_src_dir" "$_new_dir"
        return 1
    fi

• Files reviewed: 5/5 changed files
• Comments generated: 1
• Review effort level: Lite