Add polling based window waiting for testbed #3047
Conversation
There was a problem hiding this comment.
I've taken an initial look at this... and you need to slow down and think about the problem a little more.
We already have tests. They're testing the right thing. If I wanted to see a different structure to those tests, I would have commented to that effect during the review process of those tests. This piece of work is to improve the implementation of the assertion to be polling based, rather than timing based.
The assertion already has most, if not all the information we need about the thing being asserted. wait_for_window(full_screen=True) means... wait until the window is full screen. That's all it needs to do. It might be worth considering a minor refactor so that wait_for_window(full_screen=True) is wait_for_window_state(FULL_SCREEN)... but that's essentially the only change we need here.
You're trying to do a radical redesign that mixes test assertions in with probe monitoring. When you're having to introduce a new hundred line test assertion helper method to make a refactor work, that's a pretty good sign that you're not on the right track.
I'm guessing you've gone down this track because of the change that was recently introduced to handle assertions in closing dialogs... but you need to remember why that was introduced. We literally needed to insert an assertion before a dialog was closed. There was no option but to pass that test method in to the method displaying the dialog. In this case, we have no such restriction.
|
You were right, the solution was much more simpler than what I had presented. In the heat of the moment, I didn't realize that the assertion condition related to polling was ultimately only related to state. |
proneon267
force-pushed
the
polling_based_window_wait
branch
from
1464115 to
081ac8f
Compare
|
Sometimes, the macOS-arm64 CI stops at the start for no apparent reason at all, like in: https://github.com/beeware/toga/actions/runs/12416992758/job/34667120013. This doesn't seem to be related to #3016 and looks like github action randomly stopping the test run. So, I am not sure how to address it. |
testbed/tests/window/test_window.py
Outdated
| # Required to ensure complete coverage of fullscreen handling on macOS. | ||
| if states[-1] == WindowState.FULLSCREEN: | ||
| await second_window_probe.wait_for_window( | ||
| "Waiting for completion of fullscreen pending state handling", | ||
| full_screen=True, | ||
| ) | ||
|
|
There was a problem hiding this comment.
This is required to ensure coverage on line 99:
toga/cocoa/src/toga_cocoa/window.py
Lines 88 to 100 in c69f5e4
This is because, wait_for_window() waits only for the window to switch to FULLSCREEN(i.e., states[-1]), and then the test is completed. Then window is closed in window_cleanup. wait_for_window() doesn't wait for windowDidEnterFullScreen_ notification, and so line 99 doesn't get the chance to execute.
Earlier, line 99 was getting covered as we were using fixed length delays instead of polling, and so the test waited long enough for windowDidEnterFullScreen_ to be notified to the window and allowed line 99 to be executed.
There was a problem hiding this comment.
I understand the reasoning, but I don't like the approach you've taken here. AFAICT, the issue is that the assertion on window states is evaluating the instantaneous state, and not checking for pending state.
There was a problem hiding this comment.
I agree that's the issue. Could wait_for_window() include an optional assertion_test_method parameter? This would allow checking additional window properties beyond just the state.
There was a problem hiding this comment.
But, I think since the condition to ensure coverage is much simpler and only a single assertion, so I have ensured coverage by doing the assertion at the window probe.
That mode of failure usually indicates a hard crash of the app - usually a segfault due to some sort of memory allocation issue. We've had these in the past, and they're really hard to diagnose and fix. The good news is that #2978 should indirectly address this. The reason we get the memory allocation issues is because of race conditions in the way the pytest suite cleans up async fixtures. #2978 uses an updated version of Rubicon that prevents that class of memory allocation issue from ever occurring. Unfortunately, we can't land that patch until we cut a new release of Rubicon, and I'm currently on leave, so I have restricted availability to manage a release until the new year. |
|
I see, does it also mean this PR will get merged after #2978 is merged? |
I haven't done a final review of this PR yet, so merging is dependent on that review being completed. |
There was a problem hiding this comment.
A few comments inline, mostly about further opportunities for simplification.
The biggest concern, though, is the extent of the changes to the tests themselves. You've added an explanation of one specific change - which is good - but there's a lot of other modifications to the tests that need to be explained. It's entirely possible that these changes are necessary - but it's not obvious that they're necessary, and as such, that's something that should be explained in any notes to the reviewer.
The purpose of regression tests is to prevent... regressions - that is, changes in behavior. If you change an implementation and the behaviour of a test, it's a lot harder to prove that you haven't introduced a regression. Not impossible - just a lot harder. If I change only the implementation, and the tests still pass, then it's obvious that I haven't changed behaviour.
android/tests_backend/window.py
Outdated
| message, | ||
| delay=(0.5 if (full_screen or state_switch_not_from_normal) else 0.1), | ||
| ) | ||
| await self.redraw(message, delay=(0.5 if full_screen else 0.1)) |
There was a problem hiding this comment.
What's the difference between wait_for_window(full_screen=True) and wait_for_window(expected_state=FULLSCREEN)? Looking at the tests, you've updated most (if not all) of the existing calls to use the new parameter... so why keep the old one? If it's purely because of the "cleanup" mechanism - that's something that can be replaced with a literal self.redraw() - essentially reproducing the old implementation of wait_for_window for the one place we need a "fixed" interval. Or, add a wait_for_window_close() probe method, if that capability is needed in more than one place.
There was a problem hiding this comment.
Yes, the old parameters are only used for window_cleanup. Adding a wait_for_window_close() probe method would allow us to remove the old parameters entirely.
testbed/tests/app/test_desktop.py
Outdated
| "App is in presentation mode", full_screen=True | ||
| ) | ||
| assert app.in_presentation_mode | ||
|
|
There was a problem hiding this comment.
Why is the test changing? The test shouldn't be changing at all if all we're doing is changing how the test is evaluated.
There was a problem hiding this comment.
I have only changed wait_for_window() to use the new parameter. The reason for why I have moved the wait_for_window() call to inside the for loop is because, we need to wait for each window to transition to the expected state. The polling mechanism checks each window individually to confirm its state transition.
There was a problem hiding this comment.
Ok, but as a result of this change, you're not waiting for main_window any more. Adding the extra window checks makes sense, but removing the main window check seems like a fairly major change in behavior.
There was a problem hiding this comment.
Previously, wait_for_window() was pausing the test for a fixed delay, and so it didn't matter on which window probe wait_for_window() was called. Hence, I had used main_window probe for wait_for_window(). But, now since wait_for_window() checks each window individually to confirm its state transition, so waiting for main_window isn't required anymore.
There was a problem hiding this comment.
Yes, but main window isn't in window_list - so the main window isn't being checked any more. A check that was previously being performed isn't being performed now. If the main_window is already in the right state, it should be a no-op.
There was a problem hiding this comment.
I have reverted the call to main_window_probe.wait_for_window(), but the assert app.in_presentation_mode assertion needs to be inside the loop. This is because the assertion will be true only when the polling mechanism has completed waiting for the specified window to transition into the new state.
testbed/tests/window/test_window.py
Outdated
| # Required to ensure complete coverage of fullscreen handling on macOS. | ||
| if states[-1] == WindowState.FULLSCREEN: | ||
| await second_window_probe.wait_for_window( | ||
| "Waiting for completion of fullscreen pending state handling", | ||
| full_screen=True, | ||
| ) | ||
|
|
There was a problem hiding this comment.
I understand the reasoning, but I don't like the approach you've taken here. AFAICT, the issue is that the assertion on window states is evaluating the instantaneous state, and not checking for pending state.
There was a problem hiding this comment.
Getting close, but not quite. Details inline.
The Textual failure should be fixed by #3052, which should be merged shortly.
cocoa/tests_backend/window.py
Outdated
| continue | ||
| raise exception | ||
|
|
||
| async def wait_for_window_close(self, pre_close_window_state): |
There was a problem hiding this comment.
Can't pre_close_window_state be determined from the window itself?
There was a problem hiding this comment.
Once, window.close() is called, then determining the window state would be unreliable. To avoid passing the parameter, we could determine the pre_close_window_state and close the window at the window probe.
There was a problem hiding this comment.
Should I change wait_for_window_close() to close_and_wait_for_window(), so that we could determine the pre_close_window_state and close the window at the window probe?
There was a problem hiding this comment.
When you start proposing method names like close_and_wait_for_window(), you should be stepping back and asking "is there a better name for this?". In this case - it's literally a cleanup method.. so... window_probe.cleanup() (doing both the close and wait) would make sense to me.
There was a problem hiding this comment.
On the subject of naming, expected_state is also fairly verbose - unless you're expecting a different state to be passed into the window, state is fine as an argument name.
There was a problem hiding this comment.
I have added a window_probe.cleanup() method and replaced expected_state with state.
cocoa/tests_backend/window.py
Outdated
| @@ -27,16 +30,34 @@ async def wait_for_window( | |||
| message, | |||
| minimize=False, | |||
| full_screen=False, | |||
| state_switch_not_from_normal=False, | |||
| expected_state=None, | |||
There was a problem hiding this comment.
Are we still using the minimize and full_screen arguments? If so... why? My original question about this was "What's the difference between full_screen=True and expected_state=FULL_SCREEN" - if the answer is "nothing", then why do we need the other arguments?
There was a problem hiding this comment.
Looks like, I had missed cleanup of unused parameters. I have removed them now.
testbed/tests/app/test_desktop.py
Outdated
| "App is in presentation mode", full_screen=True | ||
| ) | ||
| assert app.in_presentation_mode | ||
|
|
There was a problem hiding this comment.
Ok, but as a result of this change, you're not waiting for main_window any more. Adding the extra window checks makes sense, but removing the main window check seems like a fairly major change in behavior.
testbed/tests/app/test_desktop.py
Outdated
| "App is in presentation mode", full_screen=True | ||
| ) | ||
| assert app.in_presentation_mode | ||
|
|
There was a problem hiding this comment.
Yes, but main window isn't in window_list - so the main window isn't being checked any more. A check that was previously being performed isn't being performed now. If the main_window is already in the right state, it should be a no-op.
cocoa/tests_backend/window.py
Outdated
| continue | ||
| raise exception | ||
|
|
||
| async def wait_for_window_close(self, pre_close_window_state): |
There was a problem hiding this comment.
When you start proposing method names like close_and_wait_for_window(), you should be stepping back and asking "is there a better name for this?". In this case - it's literally a cleanup method.. so... window_probe.cleanup() (doing both the close and wait) would make sense to me.
cocoa/tests_backend/window.py
Outdated
| continue | ||
| raise exception | ||
|
|
||
| async def wait_for_window_close(self, pre_close_window_state): |
There was a problem hiding this comment.
On the subject of naming, expected_state is also fairly verbose - unless you're expecting a different state to be passed into the window, state is fine as an argument name.
|
On reviewing previously failed CI test runs:https://github.com/beeware/toga/actions/runs/12415226949/job/34661335696, I noticed that on Android: The assertion failure occurs since the polling mechanism waits only until the window transitions into the expected state(FULLSCREEN) and then starts asserting other properties, which may not have updated. To account for this, Could |
Agreed this is failure mode that should be accounted for; but I'm not sure I follow what you're proposing. If there's something on Android that needs to complete to guarantee that the window is in Full Screen format in addition to the state variable, then that should be part of the Android probe implementation for |
|
Currently, the polling mechanism only waits until a single assertion is true(i.e., a window state assertion). There could situations(like on Android), where the polling mechanism should wait until a set of assertions are true(i.e., assertions in addition to window state assertion). For this, I want to propose a general purpose additional parameter Although, currently the prime use for this would be in the failing test, but I think this could become useful for eliminating intermittent failures in more tests. |
testbed/tests/app/test_desktop.py
Outdated
| # Wait for window animation before assertion. | ||
| await window1_probe.wait_for_window( | ||
| "All test windows are in WindowState.NORMAL", state=WindowState.NORMAL | ||
| ) |
There was a problem hiding this comment.
This assertion should be after the window2.state assignment, so that both windows can be restored in parallel, rather than running strictly in series.
There was a problem hiding this comment.
Thanks, I have changed it.
testbed/tests/app/test_desktop.py
Outdated
| # Wait for window animation before assertion. | ||
| await window1_probe.wait_for_window( | ||
| "All test windows are in WindowState.NORMAL", state=WindowState.NORMAL | ||
| ) |
There was a problem hiding this comment.
This assertion should be after the window2.state assignment, so that both windows can be restored in parallel, rather than running strictly in series. That will make the test marginally faster.
There was a problem hiding this comment.
Thanks, I have changed it.
testbed/tests/app/test_desktop.py
Outdated
| await window_information["window_probe"].wait_for_window( | ||
| "App is in presentation mode", state=WindowState.PRESENTATION | ||
| ) | ||
| assert app.in_presentation_mode |
There was a problem hiding this comment.
This assertion only needs to be executed once, not once per window. It needs to be after we've waited for all windows, so putting it after the for loop would make sense to me.
There was a problem hiding this comment.
I have put the assertion after the for loop.
Can I implement this to resolve the Android intermittent failure? |
As I said previously - this isn't a situation where there is a generically configurable property that some tests might find useful to be able to pass in. No other platform requires this modification (as best as we can tell). It's a situation where, on Android, checking the window state isn't a sufficient validation that the window is in a FULLSCREEN state. So - modify the |
|
The native Android backend does report correctly when the window gas completed transition to FULLSCREEN. The test fails as the general purpose The reason for proposing to have the additional parameter is because |
proneon267
force-pushed
the
polling_based_window_wait
branch
from
665e199 to
3a1a9ed
Compare
|
I also tried to fix the intermittent Android failure by adding a slight delay to the @property
def content_size(self):
if self.window.state in {WindowState.FULLSCREEN, WindowState.PRESENTATION}:
# Wait to ensure that content size is updated according to the new state.
asyncio.get_running_loop().run_until_complete(self.redraw(delay=0.1))
return (
self.root_view.getWidth() / self.scale_factor,
self.root_view.getHeight() / self.scale_factor,
)But it didn't work and crashed the testbed. I also tried to do the same with diff --git a/android/tests_backend/window.py b/android/tests_backend/window.py
index 1a03342ba..8937b5d1c 100644
--- a/android/tests_backend/window.py
+++ b/android/tests_backend/window.py
@@ -23,9 +23,10 @@ class WindowProbe(BaseProbe, DialogsMixin):
self,
message,
state=None,
+ assertion_test_method=None,
):
await self.redraw(message, delay=0.1)
- if state:
+ if state or assertion_test_method:
timeout = 5
polling_interval = 0.1
exception = None
@@ -33,7 +34,10 @@ class WindowProbe(BaseProbe, DialogsMixin):
start_time = loop.time()
while (loop.time() - start_time) < timeout:
try:
- assert self.instantaneous_state == state
+ if state:
+ assert self.instantaneous_state == state
+ if assertion_test_method:
+ assertion_test_method()
return
except AssertionError as e:
exception = e
diff --git a/testbed/tests/window/test_window.py b/testbed/tests/window/test_window.py
index cba9d1b2b..a8b49b98f 100644
--- a/testbed/tests/window/test_window.py
+++ b/testbed/tests/window/test_window.py
@@ -259,36 +259,41 @@ if toga.platform.current_platform in {"iOS", "android"}:
assert main_window_probe.instantaneous_state == WindowState.NORMAL
initial_content_size = main_window_probe.content_size
+ def assert_content_size_increase():
+ # At least one of the dimension should have increased.
+ assert (
+ main_window_probe.content_size[0] > initial_content_size[0]
+ or main_window_probe.content_size[1] > initial_content_size[1]
+ )
+
+ def assert_content_size_original():
+ assert main_window_probe.content_size == initial_content_size
+
main_window.state = state
# Wait for window animation before assertion.
await main_window_probe.wait_for_window(
- f"Main window is in {state}", state=state
+ f"Main window is in {state}",
+ state=state,
+ assertion_test_method=assert_content_size_increase,
)
assert main_window_probe.instantaneous_state == state
- # At least one of the dimension should have increased.
- assert (
- main_window_probe.content_size[0] > initial_content_size[0]
- or main_window_probe.content_size[1] > initial_content_size[1]
- )
main_window.state = state
await main_window_probe.wait_for_window(
- f"Main window is still in {state}", state=state
+ f"Main window is still in {state}",
+ state=state,
+ assertion_test_method=assert_content_size_increase,
)
assert main_window_probe.instantaneous_state == state
- # At least one of the dimension should have increased.
- assert (
- main_window_probe.content_size[0] > initial_content_size[0]
- or main_window_probe.content_size[1] > initial_content_size[1]
- )
main_window.state = WindowState.NORMAL
# Wait for window animation before assertion.
await main_window_probe.wait_for_window(
- f"Main window is not in {state}", state=WindowState.NORMAL
+ f"Main window is not in {state}",
+ state=WindowState.NORMAL,
+ assertion_test_method=assert_content_size_original,
)
assert main_window_probe.instantaneous_state == WindowState.NORMAL
- assert main_window_probe.content_size == initial_content_size
@pytest.mark.parametrize(
"state", |
There was a problem hiding this comment.
I also tried to fix the intermittent Android failure by adding a slight delay to the
content_sizeprobe property on the Android backend.
I've now told you twice that adding an assertion method parameter to wait_for_window is the wrong approach here.
Yes, "I'm waiting for content size" is a generic ask. But it's also a "generic" ask that is needed for exactly one case, on one platform.
You're proposing making the entire test harness more complex to support one platform in one case - and that case is "When moving to full screen on Android, waiting for the FULLSCREEN state alone isn't sufficient to know that the window is in a fully rendered state". That is - wait_for_window(FULLSCREEN) on Android doesn't fully reflect the window state. So... fix the problem there.
… new state on Android
| try: | ||
| assert self.instantaneous_state == state | ||
| if state in {WindowState.FULLSCREEN, WindowState.PRESENTATION}: | ||
| # Add a slight delay to ensure window properties like | ||
| # `content_size` are updated according to the new state. | ||
| await self.redraw(delay=0.1) | ||
| return |
There was a problem hiding this comment.
I would have added another assertion of assert content_size > initial_content_size on the android wait_for_window() probe method, like I have done on the cocoa backend for assert self.window._impl._pending_state_transition is None.
But, for content_size, we donot have access to the initial_content_size on the Android probe, so the content_size assertion cannot be done on the polling mechanism. Therefore, I have added a slight delay in order to ensure that the window properties like content_size are updated according to the new state.
I had also thought of adding a flag to the Android's LayoutListener, but I realized that there would no suitable place to reset the flag, and overall would cause more code churning.
There was a problem hiding this comment.
Would this be an acceptable solution?
There was a problem hiding this comment.
Yes - it's not ideal (as you've noted ideally we'd be polling for an observable condition), but it's at least as good as we had before.
| try: | ||
| assert self.instantaneous_state == state | ||
| if state in {WindowState.FULLSCREEN, WindowState.PRESENTATION}: | ||
| # Add a slight delay to ensure window properties like | ||
| # `content_size` are updated according to the new state. | ||
| await self.redraw(delay=0.1) | ||
| return |
There was a problem hiding this comment.
Yes - it's not ideal (as you've noted ideally we'd be polling for an observable condition), but it's at least as good as we had before.
Fixes #3016
This PR adds polling based waiting mechanism to
window_probe.wait_for_window().PR Checklist: