Update test_supervisor.py #3647
Conversation
The while loops in test_start_supervisor and other tests do not have timeouts, which may cause the tests to hang indefinitely if something goes wrong (e.g., the supervisor never starts or the API never returns the expected result). Fix: Add a maximum retry count or timeout to prevent infinite looping:
📝 Walkthrough📝 WalkthroughWalkthroughThe changes made in the pull request primarily focus on the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant Supervisor as Supervisor
participant Container as Container
Test->>Supervisor: Start test
Supervisor->>Container: Check if "homeassistant" is running
alt Container not running
loop Retry up to 120 times
Supervisor->>Container: Check again
Container-->>Supervisor: Status
Note over Supervisor: Wait 1 second
end
end
Supervisor->>Test: Proceed if running
alt Timeout
Supervisor->>Test: Raise TimeoutError
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/supervisor_test/test_supervisor.py (1)
Line range hint
1-1: Consider adding timeout handling to other test functions.The same infinite loop issue exists in other test functions:
test_update_supervisor: Contains an infinite loop waiting for supervisor updatetest_restart_supervisor: Contains an infinite loop waiting for supervisor restartTo maintain consistency and prevent hanging tests, consider applying similar timeout handling to these functions. Would you like me to provide the implementation for these functions as well?
🧰 Tools
🪛 Ruff
37-37: SyntaxError: unindent does not match any outer indentation level
38-38: SyntaxError: Unexpected indentation
38-39: SyntaxError: Expected an indented block after
whilestatement
43-43: SyntaxError: Expected a statement
43-43: SyntaxError: Expected a statement
43-44: SyntaxError: Expected a statement
44-44: SyntaxError: Unexpected indentation
46-46: SyntaxError: Unexpected indentation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/supervisor_test/test_supervisor.py (1 hunks)
🧰 Additional context used
🪛 Ruff
tests/supervisor_test/test_supervisor.py
37-37: SyntaxError: unindent does not match any outer indentation level
38-38: SyntaxError: Unexpected indentation
38-39: SyntaxError: Expected an indented block after
whilestatement
43-43: SyntaxError: Expected a statement
43-43: SyntaxError: Expected a statement
43-44: SyntaxError: Expected a statement
44-44: SyntaxError: Unexpected indentation
| retries = 0 | ||
| max_retries = 300 # example: wait for max 300 seconds | ||
| while retries < max_retries: | ||
| if check_container_running("homeassistant") and check_container_running("hassio_supervisor"): | ||
| break | ||
| retries += 1 | ||
| sleep(1) | ||
| else: | ||
| raise TimeoutError("Containers did not start in time") |
There was a problem hiding this comment.
Fix indentation issues and improve timeout implementation.
The current implementation has several issues:
- Severe indentation problems that would cause syntax errors
- Only the first while loop has timeout handling
- The timeout message could be more descriptive
Here's the corrected implementation with improved timeout handling for both loops:
- retries = 0
- max_retries = 300 # example: wait for max 300 seconds
- while retries < max_retries:
- if check_container_running("homeassistant") and check_container_running("hassio_supervisor"):
- break
- retries += 1
- sleep(1)
-else:
- raise TimeoutError("Containers did not start in time")
+ MAX_WAIT_TIME = 120 # 2 minutes should be sufficient
+ retries = 0
+ # Wait for containers to start
+ while retries < MAX_WAIT_TIME:
+ if check_container_running("homeassistant") and check_container_running("hassio_supervisor"):
+ break
+ retries += 1
+ sleep(1)
+ else:
+ raise TimeoutError(
+ f"Containers did not start within {MAX_WAIT_TIME} seconds. "
+ "Container status: "
+ f"homeassistant={check_container_running('homeassistant')}, "
+ f"hassio_supervisor={check_container_running('hassio_supervisor')}"
+ )
+
+ # Wait for supervisor ping
+ retries = 0
+ while retries < MAX_WAIT_TIME:
+ try:
+ if shell_json(f"curl -sSL http://{supervisor_ip}/supervisor/ping").get("result") == "ok":
+ break
+ except ExecutionError:
+ pass # avoid failure when the container is restarting
+ retries += 1
+ sleep(1)
+ else:
+ raise TimeoutError(
+ f"Supervisor ping did not respond within {MAX_WAIT_TIME} seconds"
+ )Key improvements:
- Fixed indentation
- Added timeout to both loops
- Reduced timeout to 120 seconds (2 minutes)
- Added detailed error messages with container status
- Consistent timeout handling between loops
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| retries = 0 | |
| max_retries = 300 # example: wait for max 300 seconds | |
| while retries < max_retries: | |
| if check_container_running("homeassistant") and check_container_running("hassio_supervisor"): | |
| break | |
| retries += 1 | |
| sleep(1) | |
| else: | |
| raise TimeoutError("Containers did not start in time") | |
| MAX_WAIT_TIME = 120 # 2 minutes should be sufficient | |
| retries = 0 | |
| # Wait for containers to start | |
| while retries < MAX_WAIT_TIME: | |
| if check_container_running("homeassistant") and check_container_running("hassio_supervisor"): | |
| break | |
| retries += 1 | |
| sleep(1) | |
| else: | |
| raise TimeoutError( | |
| f"Containers did not start within {MAX_WAIT_TIME} seconds. " | |
| "Container status: " | |
| f"homeassistant={check_container_running('homeassistant')}, " | |
| f"hassio_supervisor={check_container_running('hassio_supervisor')}" | |
| ) | |
| # Wait for supervisor ping | |
| retries = 0 | |
| while retries < MAX_WAIT_TIME: | |
| try: | |
| if shell_json(f"curl -sSL http://{supervisor_ip}/supervisor/ping").get("result") == "ok": | |
| break | |
| except ExecutionError: | |
| pass # avoid failure when the container is restarting | |
| retries += 1 | |
| sleep(1) | |
| else: | |
| raise TimeoutError( | |
| f"Supervisor ping did not respond within {MAX_WAIT_TIME} seconds" | |
| ) |
🧰 Tools
🪛 Ruff
37-37: SyntaxError: unindent does not match any outer indentation level
38-38: SyntaxError: Unexpected indentation
38-39: SyntaxError: Expected an indented block after
whilestatement
43-43: SyntaxError: Expected a statement
43-43: SyntaxError: Expected a statement
43-44: SyntaxError: Expected a statement
44-44: SyntaxError: Unexpected indentation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
tests/supervisor_test/test_supervisor.py (3)
Line range hint
24-28: Remove redundant infinite loop.This infinite loop should be removed since it's being replaced by the new implementation with proper timeout handling below.
- while True: - if check_container_running("homeassistant") and check_container_running("hassio_supervisor"): - break - - sleep(1)🧰 Tools
🪛 Ruff
36-36: SyntaxError: Invalid assignment target
37-38: SyntaxError: Expected an expression
38-38: SyntaxError: Expected an identifier, but found a keyword 'while' that cannot be used here
38-38: SyntaxError: Simple statements must be separated by newlines or semicolons
38-39: SyntaxError: Expected an expression
39-39: SyntaxError: Expected an identifier, but found a keyword 'if' that cannot be used here
39-39: SyntaxError: Simple statements must be separated by newlines or semicolons
39-40: SyntaxError: Expected an expression
40-40: SyntaxError: Expected an identifier, but found a keyword 'break' that cannot be used here
41-41: SyntaxError: Invalid augmented assignment target
43-43: SyntaxError: Invalid annotated assignment target
43-43: SyntaxError: Expected an identifier, but found a keyword 'else' that cannot be used here
43-44: SyntaxError: Expected an expression
44-44: SyntaxError: Expected an identifier, but found a keyword 'raise' that cannot be used here
44-44: SyntaxError: Simple statements must be separated by newlines or semicolons
49-49: SyntaxError: Expected an expression
50-51: SyntaxError: Expected an expression
51-52: SyntaxError: Expected an expression
52-52: SyntaxError: Invalid assignment target
53-53: SyntaxError: Expected an identifier, but found a keyword 'while' that cannot be used here
53-53: SyntaxError: Simple statements must be separated by newlines or semicolons
53-54: SyntaxError: Expected an expression
54-54: SyntaxError: Invalid annotated assignment target
54-54: SyntaxError: Expected an identifier, but found a keyword 'try' that cannot be used here
54-55: SyntaxError: Expected an expression
55-55: SyntaxError: Expected an identifier, but found a keyword 'if' that cannot be used here
55-55: SyntaxError: Simple statements must be separated by newlines or semicolons
55-56: SyntaxError: Expected an expression
56-56: SyntaxError: Expected an identifier, but found a keyword 'break' that cannot be used here
57-57: SyntaxError: Expected an identifier, but found a keyword 'except' that cannot be used here
57-57: SyntaxError: Simple statements must be separated by newlines or semicolons
57-58: SyntaxError: Expected an expression
58-58: SyntaxError: Expected an identifier, but found a keyword 'pass' that cannot be used here
59-59: SyntaxError: Invalid augmented assignment target
61-61: SyntaxError: Invalid annotated assignment target
61-61: SyntaxError: Expected an identifier, but found a keyword 'else' that cannot be used here
61-62: SyntaxError: Expected an expression
62-62: SyntaxError: Expected an identifier, but found a keyword 'raise' that cannot be used here
62-62: SyntaxError: Simple statements must be separated by newlines or semicolons
64-64: SyntaxError: Expected an expression
35-64: LGTM! Consider minor improvements for better maintainability.The timeout implementation effectively addresses the PR objectives by preventing infinite loops. The code is well-structured with proper error handling and descriptive error messages.
Suggestions for improvement:
- Consider moving
MAX_WAIT_TIMEto module level for reusability across test functions- Add logging statements before raising timeout errors to help with debugging
import logging from time import sleep import pytest from labgrid.driver import ExecutionError +# Maximum time to wait for operations (in seconds) +MAX_WAIT_TIME = 120 # 2 minutes should be sufficient logger = logging.getLogger(__name__) @pytest.fixture(scope="module") def stash() -> dict: """Simple stash for sharing data between tests in this module.""" stash = {} return stash @pytest.mark.dependency() @pytest.mark.timeout(600) def test_start_supervisor(shell, shell_json): - MAX_WAIT_TIME = 120 # 2 minutes should be sufficient retries = 0 # Wait for containers to start while retries < MAX_WAIT_TIME: if check_container_running("homeassistant") and check_container_running("hassio_supervisor"): break retries += 1 sleep(1) else: + logger.error( + "Container startup timeout: homeassistant=%s, hassio_supervisor=%s", + check_container_running("homeassistant"), + check_container_running("hassio_supervisor") + ) raise TimeoutError( f"Containers did not start within {MAX_WAIT_TIME} seconds. " "Container status: " f"homeassistant={check_container_running('homeassistant')}, " f"hassio_supervisor={check_container_running('hassio_supervisor')}" )🧰 Tools
🪛 Ruff
36-36: SyntaxError: Invalid assignment target
37-38: SyntaxError: Expected an expression
38-38: SyntaxError: Expected an identifier, but found a keyword 'while' that cannot be used here
38-38: SyntaxError: Simple statements must be separated by newlines or semicolons
38-39: SyntaxError: Expected an expression
39-39: SyntaxError: Expected an identifier, but found a keyword 'if' that cannot be used here
39-39: SyntaxError: Simple statements must be separated by newlines or semicolons
39-40: SyntaxError: Expected an expression
40-40: SyntaxError: Expected an identifier, but found a keyword 'break' that cannot be used here
41-41: SyntaxError: Invalid augmented assignment target
43-43: SyntaxError: Invalid annotated assignment target
43-43: SyntaxError: Expected an identifier, but found a keyword 'else' that cannot be used here
43-44: SyntaxError: Expected an expression
44-44: SyntaxError: Expected an identifier, but found a keyword 'raise' that cannot be used here
44-44: SyntaxError: Simple statements must be separated by newlines or semicolons
49-49: SyntaxError: Expected an expression
50-51: SyntaxError: Expected an expression
51-52: SyntaxError: Expected an expression
52-52: SyntaxError: Invalid assignment target
53-53: SyntaxError: Expected an identifier, but found a keyword 'while' that cannot be used here
53-53: SyntaxError: Simple statements must be separated by newlines or semicolons
53-54: SyntaxError: Expected an expression
54-54: SyntaxError: Invalid annotated assignment target
54-54: SyntaxError: Expected an identifier, but found a keyword 'try' that cannot be used here
54-55: SyntaxError: Expected an expression
55-55: SyntaxError: Expected an identifier, but found a keyword 'if' that cannot be used here
55-55: SyntaxError: Simple statements must be separated by newlines or semicolons
55-56: SyntaxError: Expected an expression
56-56: SyntaxError: Expected an identifier, but found a keyword 'break' that cannot be used here
57-57: SyntaxError: Expected an identifier, but found a keyword 'except' that cannot be used here
57-57: SyntaxError: Simple statements must be separated by newlines or semicolons
57-58: SyntaxError: Expected an expression
58-58: SyntaxError: Expected an identifier, but found a keyword 'pass' that cannot be used here
59-59: SyntaxError: Invalid augmented assignment target
61-61: SyntaxError: Invalid annotated assignment target
61-61: SyntaxError: Expected an identifier, but found a keyword 'else' that cannot be used here
61-62: SyntaxError: Expected an expression
62-62: SyntaxError: Expected an identifier, but found a keyword 'raise' that cannot be used here
62-62: SyntaxError: Simple statements must be separated by newlines or semicolons
64-64: SyntaxError: Expected an expression
Line range hint
108-125: Add timeout handling to remaining infinite loops.There are two more infinite loops in the file that should be updated with similar timeout handling:
- In
test_update_supervisor(lines 108-125)- In
test_restart_supervisor(lines 156-166)This ensures consistent behavior across all test functions and prevents any potential test hangs.
Would you like me to provide the implementation for adding timeout handling to these loops as well?
Also applies to: 156-166
🧰 Tools
🪛 Ruff
36-36: SyntaxError: Invalid assignment target
37-38: SyntaxError: Expected an expression
38-38: SyntaxError: Expected an identifier, but found a keyword 'while' that cannot be used here
38-38: SyntaxError: Simple statements must be separated by newlines or semicolons
38-39: SyntaxError: Expected an expression
39-39: SyntaxError: Expected an identifier, but found a keyword 'if' that cannot be used here
39-39: SyntaxError: Simple statements must be separated by newlines or semicolons
39-40: SyntaxError: Expected an expression
40-40: SyntaxError: Expected an identifier, but found a keyword 'break' that cannot be used here
41-41: SyntaxError: Invalid augmented assignment target
43-43: SyntaxError: Invalid annotated assignment target
43-43: SyntaxError: Expected an identifier, but found a keyword 'else' that cannot be used here
43-44: SyntaxError: Expected an expression
44-44: SyntaxError: Expected an identifier, but found a keyword 'raise' that cannot be used here
44-44: SyntaxError: Simple statements must be separated by newlines or semicolons
49-49: SyntaxError: Expected an expression
50-51: SyntaxError: Expected an expression
51-52: SyntaxError: Expected an expression
52-52: SyntaxError: Invalid assignment target
53-53: SyntaxError: Expected an identifier, but found a keyword 'while' that cannot be used here
53-53: SyntaxError: Simple statements must be separated by newlines or semicolons
53-54: SyntaxError: Expected an expression
54-54: SyntaxError: Invalid annotated assignment target
54-54: SyntaxError: Expected an identifier, but found a keyword 'try' that cannot be used here
54-55: SyntaxError: Expected an expression
55-55: SyntaxError: Expected an identifier, but found a keyword 'if' that cannot be used here
55-55: SyntaxError: Simple statements must be separated by newlines or semicolons
55-56: SyntaxError: Expected an expression
56-56: SyntaxError: Expected an identifier, but found a keyword 'break' that cannot be used here
57-57: SyntaxError: Expected an identifier, but found a keyword 'except' that cannot be used here
57-57: SyntaxError: Simple statements must be separated by newlines or semicolons
57-58: SyntaxError: Expected an expression
58-58: SyntaxError: Expected an identifier, but found a keyword 'pass' that cannot be used here
59-59: SyntaxError: Invalid augmented assignment target
61-61: SyntaxError: Invalid annotated assignment target
61-61: SyntaxError: Expected an identifier, but found a keyword 'else' that cannot be used here
61-62: SyntaxError: Expected an expression
62-62: SyntaxError: Expected an identifier, but found a keyword 'raise' that cannot be used here
62-62: SyntaxError: Simple statements must be separated by newlines or semicolons
64-64: SyntaxError: Expected an expression
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/supervisor_test/test_supervisor.py (1 hunks)
🧰 Additional context used
🪛 Ruff
tests/supervisor_test/test_supervisor.py
36-36: SyntaxError: Invalid assignment target
37-38: SyntaxError: Expected an expression
38-38: SyntaxError: Expected an identifier, but found a keyword 'while' that cannot be used here
38-38: SyntaxError: Simple statements must be separated by newlines or semicolons
38-39: SyntaxError: Expected an expression
39-39: SyntaxError: Expected an identifier, but found a keyword 'if' that cannot be used here
39-39: SyntaxError: Simple statements must be separated by newlines or semicolons
39-40: SyntaxError: Expected an expression
40-40: SyntaxError: Expected an identifier, but found a keyword 'break' that cannot be used here
41-41: SyntaxError: Invalid augmented assignment target
43-43: SyntaxError: Invalid annotated assignment target
43-43: SyntaxError: Expected an identifier, but found a keyword 'else' that cannot be used here
43-44: SyntaxError: Expected an expression
44-44: SyntaxError: Expected an identifier, but found a keyword 'raise' that cannot be used here
44-44: SyntaxError: Simple statements must be separated by newlines or semicolons
49-49: SyntaxError: Expected an expression
50-51: SyntaxError: Expected an expression
51-52: SyntaxError: Expected an expression
52-52: SyntaxError: Invalid assignment target
53-53: SyntaxError: Expected an identifier, but found a keyword 'while' that cannot be used here
53-53: SyntaxError: Simple statements must be separated by newlines or semicolons
53-54: SyntaxError: Expected an expression
54-54: SyntaxError: Invalid annotated assignment target
54-54: SyntaxError: Expected an identifier, but found a keyword 'try' that cannot be used here
54-55: SyntaxError: Expected an expression
55-55: SyntaxError: Expected an identifier, but found a keyword 'if' that cannot be used here
55-55: SyntaxError: Simple statements must be separated by newlines or semicolons
55-56: SyntaxError: Expected an expression
56-56: SyntaxError: Expected an identifier, but found a keyword 'break' that cannot be used here
57-57: SyntaxError: Expected an identifier, but found a keyword 'except' that cannot be used here
57-57: SyntaxError: Simple statements must be separated by newlines or semicolons
57-58: SyntaxError: Expected an expression
58-58: SyntaxError: Expected an identifier, but found a keyword 'pass' that cannot be used here
59-59: SyntaxError: Invalid augmented assignment target
61-61: SyntaxError: Invalid annotated assignment target
61-61: SyntaxError: Expected an identifier, but found a keyword 'else' that cannot be used here
61-62: SyntaxError: Expected an expression
62-62: SyntaxError: Expected an identifier, but found a keyword 'raise' that cannot be used here
62-62: SyntaxError: Simple statements must be separated by newlines or semicolons
64-64: SyntaxError: Expected an expression
The while loops in test_start_supervisor and other tests do not have timeouts, which may cause the tests to hang indefinitely if something goes wrong (e.g., the supervisor never starts or the API never returns the expected result).
Fix: Add a maximum retry count or timeout to prevent infinite looping:
Summary by CodeRabbit