Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cobalt/tools/on_device_tests_gateway_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ def main() -> int:
trigger_args.add_argument(
'--job_timeout_sec',
type=str,
default='2100',
default='2700',
Comment on lines 353 to +354
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

For a numeric timeout value, it's better practice to use type=int instead of type=str. This ensures the value is parsed correctly as an integer at the command-line level, preventing potential bugs and simplifying the code that uses this value, as it won't need to perform its own type conversion.

Suggested change
type=str,
default='2100',
default='2700',
type=int,
default=2700,

help='Timeout in seconds for the job. Must be set higher and '
'start_timeout_sec and test_timeout_sec combined.',
Comment on lines +354 to 356
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The new timeout of 2700 seconds might not be sufficient. According to the PR description, this change is to allow a second test attempt to succeed. With test_timeout_sec at 1800 seconds, two attempts could take up to 3600 seconds, plus any startup time (start_timeout_sec). The current value of 2700s doesn't cover this worst-case scenario and may lead to timeouts in the future. The job timeout should be set to a value that can reliably accommodate all attempts, for instance by following a formula like (number_of_attempts * test_timeout_sec) + start_timeout_sec.

)
Expand Down
Loading