Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix test_window_state_assignment testbed on macOS-arm64 #3018

Merged
merged 5 commits into from
Dec 5, 2024

Conversation

proneon267
Copy link
Contributor

@proneon267 proneon267 commented Dec 4, 2024

Fixes #3016

The testbed test was failing since the test assertion was being done even before the window state change was completed. The failure only affected macOS-arm64, since a longer delay before assertion is required on macOS-arm64 for fullscreen transition. Since, we cannot differentiate between macOS-x86_64 and macOS-arm64, so on the cocoa backend the delay for fullscreen is 1s now.

I apologise for causing any inconveniences.

PR Checklist:

  • All new features have been tested
  • All new features have been documented
  • I have read the CONTRIBUTING.md file
  • I will abide by the code of conduct

@proneon267 proneon267 force-pushed the window_state_testbed_fix branch from d547c88 to 2b279cc Compare December 4, 2024 16:00
@proneon267 proneon267 changed the title Fix test_window_state_assignment testbed on macOS-arm64 Fix test_window_state_assignment testbed on macOS-arm64 Dec 4, 2024
@proneon267 proneon267 force-pushed the window_state_testbed_fix branch from d13c425 to eaad6a6 Compare December 4, 2024 20:13
@proneon267
Copy link
Contributor Author

@mhsmith could you please confirm if this PR fixes the test failure on your local machine?

@mhsmith
Copy link
Member

mhsmith commented Dec 4, 2024

No, it doesn't, and even increasing the 1 to 2 or 5 doesn't make any difference.

See attached screen recording. My dock is hidden on the left edge of the screen, but you can see when a window minimizes to it.

Screen.Recording.2024-12-04.at.21.55.31.mov

@freakboy3742
Copy link
Member

FWIW: I don't see this on my machine - I only see the issue in CI. I'm on macOS 14.7; CI indicates its running on 14.7.1.

Comment on lines 35 to 37
1.75
if state_switch_not_from_normal
else 0.75 if full_screen else 0.5 if minimize else 0.1
else 1 if full_screen else 0.5 if minimize else 0.1
Copy link
Member

Choose a reason for hiding this comment

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

I'm unclear how this change would impact the failure mode that has been reported. The test failure is on L812; the preceding wait_for_window call explicitly sets state_switch_not_from_normal=True. So... shouldn't this be a change to the 1.75, not the line modified here?

Copy link
Contributor Author

@proneon267 proneon267 Dec 5, 2024

Choose a reason for hiding this comment

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

My running theory about this error is that the window cleanup state change isn't getting enough delay in case of fullscreen when it finishes running the first test case.

# Complex state changes on cocoa:
(WindowState.MINIMIZED, WindowState.FULLSCREEN),
(WindowState.FULLSCREEN, WindowState.MINIMIZED),
# Complex state changes on gtk:
(WindowState.FULLSCREEN, WindowState.PRESENTATION),
(WindowState.PRESENTATION, WindowState.NORMAL),

async def window_cleanup(app, app_probe, main_window, main_window_probe):
# Ensure that at the end of every test, all windows that aren't the
# main window have been closed and deleted. This needs to be done in
# 2 passes because we can't modify the list while iterating over it.
kill_list = []
for window in app.windows:
if window != main_window:
kill_list.append(window)
# Then purge everything on the kill list.
while kill_list:
window = kill_list.pop()
window.close()
await main_window_probe.wait_for_window("Closing window")
del window
# Force a GC pass on the main thread. This isn't perfect, but it helps
# minimize garbage collection on the test thread.
gc.collect()
main_window_state = main_window.state
main_window.state = WindowState.NORMAL
app.current_window = main_window
await main_window_probe.wait_for_window(
"Resetting main_window",
minimize=True if main_window_state == WindowState.MINIMIZED else False,
full_screen=True if main_window_state == WindowState.FULLSCREEN else False,
)

I tried to increase state_switch_not_from_normal delay to 3, but it still fails. So, I am going to try 2 solutions:
* Add a waiting delay to the start of the test
* Manually reset the window state to NORMAL at the end of the test

My earlier guess was incorrect, a delay is required when closing the window from a non-normal window state.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The complication here is that I can't reproduce the problem locally; apparently @mhsmith can though, so he may be able to provide better confirmation that this works.

However, this fix doesn't make a whole lot of sense to me. I can see how an end-of-test minimized state might cause problems - but that's not where the problem test was failing - it was failing at L812, after a wait_for_window has successfully completed asserting the initial state of the window under test, and a wait_for_window which should allow for the final state to be applied.

Yes, it's passed CI; but the problem was intermittent; I'll poke it a few times to see if I can manifest a failure.

return WindowState.PRESENTATION
else:
return WindowState.FULLSCREEN
if getattr(self, "app", None) is not None:
Copy link
Member

Choose a reason for hiding this comment

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

It's not obvious to me why this is possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since, get_window_state() is being called in window_cleanup, so it might be getting called after window.close() is called on the window. Hence, leading to a situation where self.app might not exist.

@freakboy3742
Copy link
Member

I can't argue with success though... 6 CI passes and no failure. I'll wait to see if the problem is resolved for @mhsmith, but otherwise this is looking promising.

Copy link
Member

@mhsmith mhsmith left a comment

Choose a reason for hiding this comment

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

Works for me. I don't know why my machine was different from yours, since I'm also on macOS 14.7. Maybe it's something to do with my screen configuration.

I can see how an end-of-test minimized state might cause problems - but that's not where the problem test was failing - it was failing at L812

Since it was failing on the second parametrization, maybe it was caused by insufficient cleanup from the first one?

@mhsmith mhsmith merged commit 0ac9ec5 into beeware:main Dec 5, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Testbed failure in test_window_state_rapid_assignment
3 participants