Skip to content
Open
15 changes: 14 additions & 1 deletion scripts/tests/run_python_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,13 @@ def main_impl(app: str, factory_reset: bool, factory_reset_app_only: bool, app_a
daemon=True)
restart_monitor_thread.start()

split_args = shlex.split(script_args)
timeout_key = '--timeout'
timeout_index = split_args.index(timeout_key) if timeout_key in split_args else None
Comment on lines +323 to +326

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The manual --timeout lookup in run_python_test.py only handles the space-separated form (--timeout 20). It looks for a standalone --timeout token and reads the next one. With --timeout=20, shlex.split() produces a single token, so the lookup misses it, test_arg_timeout stays None, and the outer Subprocess.wait() falls back to the 300s default — silently undoing the fix this PR is trying to make.

No current caller uses the = form (CI metadata uses --timeout 1800, etc.), so this is latent rather than a live bug.

runner.py already parses --timeout via argparse (type=int), which handles both forms and validates the value. Worth reusing that here (e.g. a small parse_known_args with just --timeout) instead of hand-parsing, so the orchestrator and the test script read the flag the same way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I far I know we don't use that format for CI arguments in the CI TEST ARGUMENTS section from python tests.

@jtrejoespinoza-grid jtrejoespinoza-grid Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way only a few tests use "=" for arguments and for what I see only for app arguments and not for script arguments:

You can check by using sed on src/python_testing

sed -n '/BEGIN CI TEST ARGUMENTS/,/END CI TEST ARGUMENTS/p' src/python_testing/*py | grep -v "===" | grep =

test_arg_timeout = None
if timeout_index is not None:
test_arg_timeout = int(split_args[timeout_index+1])
Comment thread
jtrejoespinoza-grid marked this conversation as resolved.
Outdated

# TODO: Remove this below workaround once we understand if mobile-device-test needs to be run through Cirque and through this script for CI test pipeline, task PR: https://github.com/project-chip/matter-test-scripts/issues/681
if "mobile-device-test.py" not in script:
script_args += f" --restart-flag-file {restart_flag_file}"
Expand Down Expand Up @@ -350,7 +357,13 @@ def main_impl(app: str, factory_reset: bool, factory_reset_app_only: bool, app_a
test_script_process.p.stdin.close()

try:
test_script_exit_code = test_script_process.wait()
# Some tests have very large execution time (Nightly jobs), we use the --timeout to avoid test to use default timeout
if test_arg_timeout is None:
# This will use the DEFAULT_TIMEOUT_S
test_script_exit_code = test_script_process.wait()
else:
log.info("Executing the test with a timeout of %d seconds", test_arg_timeout)
test_script_exit_code = test_script_process.wait(timeout=test_arg_timeout)
Comment thread
jtrejoespinoza-grid marked this conversation as resolved.
Outdated

if test_script_exit_code != 0:
log.error("Test script exited with returncode %d", test_script_exit_code)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ class Subprocess(threading.Thread):
"""Run a subprocess in a thread."""

DEFAULT_TIMEOUT_S: float = 300.0
TERMINATION_TIMEOUT_S: float = 5.0
TERMINATION_TIMEOUT_S: float = 10.0

def __init__(self, program: str, *args: str, output_cb: Callable[[bytes, bool], bytes] | None = None,
f_stdout: BinaryIO = sys.stdout.buffer, f_stderr: BinaryIO = sys.stderr.buffer) -> None:
Expand Down Expand Up @@ -217,4 +217,8 @@ def terminate(self) -> None:
def wait(self, timeout: float = DEFAULT_TIMEOUT_S) -> int | None:
"""Wait for the subprocess to finish."""
self.join(timeout)
# join() does not return a value, so we call is_alive(), if is alive this mean it timed out.
if self.is_alive():
# Terminate the process to get a returncode
self.terminate()
return self.returncode
Comment thread
jtrejoespinoza-grid marked this conversation as resolved.
Loading