[fix] comp text test for headless env#3484
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates GUI text control tests to behave reliably in headless display environments by ensuring the “on focus selects text” behavior still occurs even when SetFocus() does not actually grant OS-level focus.
Changes:
- Added a
_set_focus()helper that attemptsSetFocus()and falls back to triggering focus-selection logic when focus cannot be obtained. - Replaced several direct
SetFocus()+gui_loop()sequences with_set_focus()for consistency across the test cases.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds NumberTextCtrlTestCase._set_focus(ctrl) which calls ctrl.SetFocus(), checks ctrl.HasFocus(), and—if focus wasn't acquired—creates and dispatches a synthetic wx.FocusEvent to invoke the control's focus handler. Five test sites that previously called SetFocus() directly were changed to call self._set_focus(...) so the tests exercise focus/selection logic in both GUI and headless environments. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
🧹 Nitpick comments (1)
src/odemis/gui/test/comp_text_test.py (1)
122-123: ⚡ Quick winInitialize the synthetic event with the target control.
For completeness and to follow wxPython best practices, call
SetEventObject(ctrl)on the synthetic event before passing it to the handler. Although the current implementation works becauseon_focusonly callsevt.Skip(), proper initialization ensures the event object is fully formed.🔧 Proposed fix
# Headless fallback: directly invoke the focus handler evt = wx.FocusEvent(wx.wxEVT_SET_FOCUS) + evt.SetEventObject(ctrl) ctrl.on_focus(evt)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/odemis/gui/test/comp_text_test.py` around lines 122 - 123, The synthetic wx.FocusEvent created in the test is not initialized with its target control; call evt.SetEventObject(ctrl) before invoking ctrl.on_focus(evt) so the event's event object is properly set—update the test to set the event object on the evt instance (evt) prior to calling the control's on_focus handler (ctrl.on_focus).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/odemis/gui/test/comp_text_test.py`:
- Around line 115-117: The _set_focus function lacks type hints and its
docstring must be converted to reStructuredText; update the signature to include
a concrete type for ctrl (e.g., ctrl: wx.Control or a suitable control
protocol/ABC) and a return type of None, and replace the triple-quoted docstring
with an rst-style description (one-line summary and an extended description)
that documents the parameter and return value; keep the body invoking
ctrl.SetFocus() and retain the headless note in the rst docstring.
---
Nitpick comments:
In `@src/odemis/gui/test/comp_text_test.py`:
- Around line 122-123: The synthetic wx.FocusEvent created in the test is not
initialized with its target control; call evt.SetEventObject(ctrl) before
invoking ctrl.on_focus(evt) so the event's event object is properly set—update
the test to set the event object on the evt instance (evt) prior to calling the
control's on_focus handler (ctrl.on_focus).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ccef6d9-c3ba-4e0b-b0f5-9fc7c7fa560b
📒 Files selected for processing (1)
src/odemis/gui/test/comp_text_test.py
| def _set_focus(self, ctrl): | ||
| """Set focus, falling back to directly invoking on_focus if run in a headless environment.""" | ||
| ctrl.SetFocus() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type hints and convert docstring to reStructuredText format.
The coding guidelines require type hints for all function parameters and return types, and reStructuredText-style docstrings for all functions.
📝 Proposed fix
- def _set_focus(self, ctrl):
- """Set focus, falling back to directly invoking on_focus if run in a headless environment."""
+ def _set_focus(self, ctrl: wx.TextCtrl) -> None:
+ """Set focus on a text control, with headless environment fallback.
+
+ Attempts to acquire GUI focus via SetFocus(). If the control does not
+ report focus after a brief wait (common in headless environments), directly
+ invokes the control's on_focus handler with a synthetic event.
+
+ Args:
+ ctrl: The text control to focus.
+ """📝 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.
| def _set_focus(self, ctrl): | |
| """Set focus, falling back to directly invoking on_focus if run in a headless environment.""" | |
| ctrl.SetFocus() | |
| def _set_focus(self, ctrl: wx.TextCtrl) -> None: | |
| """Set focus on a text control, with headless environment fallback. | |
| Attempts to acquire GUI focus via SetFocus(). If the control does not | |
| report focus after a brief wait (common in headless environments), directly | |
| invokes the control's on_focus handler with a synthetic event. | |
| Args: | |
| ctrl: The text control to focus. | |
| """ | |
| ctrl.SetFocus() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/odemis/gui/test/comp_text_test.py` around lines 115 - 117, The _set_focus
function lacks type hints and its docstring must be converted to
reStructuredText; update the signature to include a concrete type for ctrl
(e.g., ctrl: wx.Control or a suitable control protocol/ABC) and a return type of
None, and replace the triple-quoted docstring with an rst-style description
(one-line summary and an extended description) that documents the parameter and
return value; keep the body invoking ctrl.SetFocus() and retain the headless
note in the rst docstring.
Source: Coding guidelines
… obtained in case of a headless display environment
nandishjpatel
force-pushed
the
fix-comp-text-test-for-headless-env
branch
from
ab4da63 to
c94151d
Compare
| if not ctrl.HasFocus(): | ||
| # Headless fallback: directly invoke the focus handler | ||
| evt = wx.FocusEvent(wx.wxEVT_SET_FOCUS, ctrl.Id) | ||
| evt.SetEventObject(ctrl) | ||
| ctrl.GetEventHandler().ProcessEvent(evt) | ||
| test.gui_loop(0.1) |
There was a problem hiding this comment.
Did you verify that this works? What do you mean by "headless"? As far as I understood, our VMs run with a simulated screen, with Gnome running. Why would it be "headless"? Do you know why such setup work differently from a normal computer?
There was a problem hiding this comment.
I haven't tested this fix.
This fix is needed for Ubuntu 24, where xvfb a virtual display is needed to run the testcases. By headless I mean the testcases/programs run headless in the xvfb virtual display.
Ubuntu 24 testcases run command: https://bitbucket.org/delmic/odemis-testing/src/30268666bced16e45931ccaad349d5db38b6017c/ci/canonical_lxd/action.sh#lines-58
There was a problem hiding this comment.
@nandishjpatel I think that there are two options forward:
- we find a way to test this fix without merging it. Maybe one way is to temporarily use this branch as target for testing on the Ubuntu 24 run. Another option would be to start the Ubuntu 24 image on your computer and run the test suite (or just this GUI part).
- we merge this change, wait for the test cases to run, and if it doesn't help, we revert it.
I'd rather go with the first route. Can you try finding a way to do it?
directly invoke the ctrl's focus handler if SetFocus() didn't succeed in case of a headless display environment