Issue #5383 - Feature: Added global execution cli timeout#5594
Issue #5383 - Feature: Added global execution cli timeout#5594hassineabd wants to merge 3 commits into
Conversation
pekkaklarck
left a comment
pekkaklarck
left a comment
There was a problem hiding this comment.
This feature touches many different parts of Robot and many of those parts are old and not that great code. Considering that, this PR is pretty impressive!
I didn't have time to go through the PR in detail, but I noticed some things that need to be changed. If you have questions, we can discuss this further on the #devel channel on Slack.
| old_timeout.string = new_timeout_obj.string | ||
| # this way do NOT reset start_time, so it counts from original start. | ||
|
|
||
| print(f"DEBUG: Timeout after: {EXECUTION_CONTEXTS.total_timeout}") |
There was a problem hiding this comment.
EXECUTION_CONTEXT is not part of the public API and we have a plan to replaces this global variable with execution related context object in the future. EXECUTION_CONTEXT can be used in tests if needed, but it shouldn't be used in examples demonstrating how users could utilize it.
In general I don't consider the possibility increase the global timeout important and it should be part of this overall issue. Setting a maximum timeout for the whole execution is fine, but there are already other ways to stop the whole execution conditionally if needed.
| if not value or value.upper() == "NONE": | ||
| return None | ||
| if contains_variable(value): | ||
| return value |
There was a problem hiding this comment.
No other command line option supports variables and this option shouldn't support them either. Environment variables can be used like --timeout $TIMEOUT if something like this is needed.
| def __init__(self): | ||
| self._contexts = [] | ||
| self._asynchronous = Asynchronous() | ||
| self.total_timeout = None |
There was a problem hiding this comment.
This should be _total_timeout to not encourage anyone to touch it. Such code will break at some point.
|
|
||
| @property | ||
| def total_timeout(self): | ||
| return EXECUTION_CONTEXTS.total_timeout |
There was a problem hiding this comment.
This should be _total_timeout or not exist at all to not encourage users to touch it.
| self.suite = suite | ||
| self.test = None | ||
| self.timeouts = [] | ||
| self.timeouts = [EXECUTION_CONTEXTS.total_timeout] if EXECUTION_CONTEXTS.total_timeout else [] |
There was a problem hiding this comment.
The current execution context handling is a big mess that I hope to clean up at some point, but this code adds even more coupling between classes. A simple way to simplify this is adding global_timeout=None to __init__ signature and then doing something like
self.timeouts = [global_timeout] if global_timeout else []here. If self.timeouts would need to be re-initialized later, global_timeout could be stored to self.global_timeout, but I hope we could avoid that.
| def end_test(self, test): | ||
| self.test = None | ||
| self.timeouts = [] | ||
| self.timeouts = [EXECUTION_CONTEXTS.total_timeout] if EXECUTION_CONTEXTS.total_timeout else [] |
There was a problem hiding this comment.
I think we should be able to pop the possible test timeout from self.timeout here instead of re-initializing whole self.timeouts. Earlier doing self.timeouts = [] was so simple that it made sense, but popping ought to be simpler than re-initialization if global timeout exists.
| timeout = self.settings["Timeout"] | ||
| if timeout: | ||
| return TotalTimeout(timeout, self.variables) | ||
| return None |
There was a problem hiding this comment.
Does SuiteRunner need total_timeout? Wouldn't it be enough if contexts has it?
| with self.context.suite_teardown(): | ||
| failure = self._run_teardown(suite, self.suite_status, self.suite_result) | ||
| if not failure and self.context.total_timeout and self.context.total_timeout.timed_out(): | ||
| failure = ExecutionFailed(self.context.total_timeout.get_message(), test_timeout=True) |
There was a problem hiding this comment.
test_timeoout=True isn't exactly correct in this case. Not sure how much it matters.
|
|
||
|
|
||
| class TotalTimeout(Timeout): | ||
| kind = "TOTAL EXECUTION" |
There was a problem hiding this comment.
As discussed on Slack, this should be GlobalTimeout instead of TotalTimeout and that terminology should be used consistently. The kind attribute should probably have value GLOBAL.
| from robot.output import LOGGER | ||
| msg = f"Execution stopped: Total execution timeout of {self.string} exceeded." | ||
| LOGGER.error(msg) | ||
| self._reported = True |
There was a problem hiding this comment.
Why is this situation reported separately as an error?
2 participants
Issue #5383
Key Changes:
acceptance tests : atest/robot/running/global_timeout.robot validates 16 distinct scenarios including nested suites, dry-run, variable expansion, and CLI validation.