Hani/ Fix test_multiple_saved_logins.py on Linux#1502
Conversation
|
The fix is correct and minimal — allowing One thing worth noting: on modern Linux systems using Wayland, if sysname == "Linux" and not os.environ.get("DISPLAY") and not os.environ.get("WAYLAND_DISPLAY"):Not blocking, just something to keep in mind if this surfaces on Wayland setups. |
|
Overall: Good, targeted fix. The change to conditionally load Two notes:
|
ReviewThe fix is logically sound — previously, One concern: Wayland compatibility When Worth confirming that the CI Linux environment runs with XWayland (i.e., Pre-existing minor issue (not introduced here): The Manifest change looks appropriate. |
| and not os.environ.get("DISPLAY") | ||
| and not os.environ.get("WAYLAND_DISPLAY") | ||
| ): | ||
| return None |
There was a problem hiding this comment.
When running on pure Wayland (WAYLAND_DISPLAY set, DISPLAY unset), pyautogui will be imported here but its underlying X11-based operations may fail at runtime. If CI uses Wayland without XWayland, calls like self.gui.press() / self.gui.moveTo() could raise errors instead of gracefully skipping.
Consider whether you also need to verify that at least DISPLAY is set (XWayland present) before enabling pyautogui on Wayland, or add a try/except around the import to handle environments where pyautogui loads but can't operate.
ben-c-at-moz
left a comment
There was a problem hiding this comment.
I'm actually extremely surprised this works. Let's talk about it tomorrow...I was sure we were not able to use any amount of PyAutoGUI on Wayland at the moment.
Relevant Links
Bugzilla: 2052308
Description of Code / Doc Changes
Process Changes Required
Mark the relevant boxes, delete irrelevant lines.
uv sync)./devsetup.sh)Screenshots or Explanations
N/A
Comments or Future Work
N/A
Workflow Checklist
Thank you!