Fix tests to accommodate wait for container re-download on downgrade#4368
Fix tests to accommodate wait for container re-download on downgrade#4368
Conversation
Because the OS downgrade performed in tests now triggers change in container snapshotters, all containers need to be redownloaded. Make sure that CLI container exists and increase the timeout for the time being.
📝 WalkthroughWalkthroughThis change enhances test reliability by adding container status polling to verify that Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/smoke_test/test_os_update.py (1)
44-48: Extract duplicated helper to module level.The
check_container_runningfunction is duplicated identically intest_init(lines 13-17). Extract it to module level to eliminate duplication and improve maintainability.Apply this diff to extract the helper:
_LOGGER = logging.getLogger(__name__) + +def check_container_running(container_name): + """Check if a container is running via docker inspect.""" + from labgrid.driver import ShellDriver + # Note: This assumes shell context; caller must provide shell.run_check + # Better: pass shell as parameter + raise NotImplementedError("Needs shell parameter") +Actually, since the helper needs access to
shell.run_check, a better approach is to acceptshellas a parameter:_LOGGER = logging.getLogger(__name__) + +def check_container_running(shell, container_name): + """Check if a container is running via docker inspect.""" + out = shell.run_check( + f"docker container inspect -f '{{{{.State.Status}}}}' {container_name} || true" + ) + return "running" in out + @pytest.mark.dependency() @pytest.mark.timeout(120) def test_init(shell, shell_json): - def check_container_running(container_name): - out = shell.run_check( - f"docker container inspect -f '{{{{.State.Status}}}}' {container_name} || true" - ) - return "running" in out - # wait for important containers first while True: - if check_container_running("homeassistant") and check_container_running("hassio_supervisor"): + if check_container_running(shell, "homeassistant") and check_container_running(shell, "hassio_supervisor"): break sleep(1)Then update
test_os_updatesimilarly to remove its local definition and call the module-level function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
tests/smoke_test/test_os_update.py(2 hunks)
🔇 Additional comments (1)
tests/smoke_test/test_os_update.py (1)
42-42: LGTM! Timeout increase aligns with PR objectives.The doubled timeout accommodates container re-downloads during the OS downgrade scenario. Ensure the TODO is tracked so the timeout can be reduced after the 17.0 release.
| # temporary needed for OS 17.0 -> 16.x path, where all containers must be re-downloaded | ||
| while True: | ||
| if check_container_running("hassio_supervisor") and check_container_running("hassio_cli"): | ||
| break | ||
|
|
||
| sleep(1) |
There was a problem hiding this comment.
Add explicit timeout to container wait loop.
The wait loop lacks a timeout or maximum iteration limit. While the pytest @pytest.mark.timeout(600) decorator provides an outer bound, explicit timeouts improve test reliability by providing clearer failure messages and more predictable behavior.
Apply this diff to add an explicit timeout:
# temporary needed for OS 17.0 -> 16.x path, where all containers must be re-downloaded
+ max_wait = 120 # seconds
+ elapsed = 0
while True:
if check_container_running("hassio_supervisor") and check_container_running("hassio_cli"):
break
+ if elapsed >= max_wait:
+ raise TimeoutError(
+ f"Containers hassio_supervisor and hassio_cli did not start within {max_wait}s"
+ )
+ elapsed += 1
sleep(1)Alternatively, use a more concise approach with a counter:
# temporary needed for OS 17.0 -> 16.x path, where all containers must be re-downloaded
- while True:
+ max_attempts = 120
+ for attempt in range(max_attempts):
if check_container_running("hassio_supervisor") and check_container_running("hassio_cli"):
break
sleep(1)
+ else:
+ raise TimeoutError(
+ "Containers hassio_supervisor and hassio_cli did not start within 120s"
+ )📝 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.
| # temporary needed for OS 17.0 -> 16.x path, where all containers must be re-downloaded | |
| while True: | |
| if check_container_running("hassio_supervisor") and check_container_running("hassio_cli"): | |
| break | |
| sleep(1) | |
| # temporary needed for OS 17.0 -> 16.x path, where all containers must be re-downloaded | |
| max_wait = 120 # seconds | |
| elapsed = 0 | |
| while True: | |
| if check_container_running("hassio_supervisor") and check_container_running("hassio_cli"): | |
| break | |
| if elapsed >= max_wait: | |
| raise TimeoutError( | |
| f"Containers hassio_supervisor and hassio_cli did not start within {max_wait}s" | |
| ) | |
| elapsed += 1 | |
| sleep(1) |
| # temporary needed for OS 17.0 -> 16.x path, where all containers must be re-downloaded | |
| while True: | |
| if check_container_running("hassio_supervisor") and check_container_running("hassio_cli"): | |
| break | |
| sleep(1) | |
| # temporary needed for OS 17.0 -> 16.x path, where all containers must be re-downloaded | |
| max_attempts = 120 | |
| for attempt in range(max_attempts): | |
| if check_container_running("hassio_supervisor") and check_container_running("hassio_cli"): | |
| break | |
| sleep(1) | |
| else: | |
| raise TimeoutError( | |
| "Containers hassio_supervisor and hassio_cli did not start within 120s" | |
| ) |
🤖 Prompt for AI Agents
In tests/smoke_test/test_os_update.py around lines 72-77, the infinite loop that
waits for hassio_supervisor and hassio_cli to run needs an explicit timeout;
replace the while True loop with a timed-wait using a max_wait (e.g. 600 seconds
or a shorter sensible value) by recording start = time.time(), looping while
elapsed < max_wait and sleeping briefly, and after the loop raise a clear
failure (pytest.fail or AssertionError) if containers never became ready; ensure
time is imported if not already present.
Because the OS downgrade performed in tests now triggers change in container snapshotters, all containers need to be redownloaded. Make sure that CLI container exists and increase the timeout for the time being.
Summary by CodeRabbit