Add macOS CI workflow and fix platform-specific test issues #83
Conversation
WalkthroughAdds a macOS CI job; refactors Qt6 test apps for explicit DPI, font, pixmap, and layout handling and moves QApplication creation under main guards; standardizes platform checks to platform.system() and updates test skip annotations and several expected coordinates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
172-212: macOS CI job structure looks good, with one minor typo.The macOS job correctly mirrors the Windows configuration with appropriate feature disabling. However, there's a typo on line 205:
- # TODO: is tesseract availalbe for MacOS + # TODO: is tesseract available for macOSNote: Tesseract is available via Homebrew on macOS (
brew install tesseract). You could potentially enable OCR tests on macOS by adding the installation step.Would you like me to suggest how to enable OCR support on macOS?
🧹 Nitpick comments (1)
tests/test_region_expect.py (1)
79-79: Consider updating platform detection for consistency.This line still uses
os.name == 'nt'whiletest_region_control.pyhas been updated to useplatform.system() == 'Windows'. Consider aligning this for consistency across the test suite.+import platform + def show_image(self, filename: str) -> None: filename = self.file_resolver.search(filename) - python = 'python.exe' if os.name == 'nt' else 'python3' + python = 'python.exe' if platform.system() == 'Windows' else 'python3' self.child_img = subprocess.Popen([python, self.script_img, filename])
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.yml(1 hunks)tests/qt6_application.py(5 hunks)tests/qt6_image.py(2 hunks)tests/test_region_control.py(5 hunks)tests/test_region_expect.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_region_expect.py (1)
guibot/region.py (2)
find(422-434)find_all(436-516)
🪛 GitHub Check: CodeQL
tests/qt6_image.py
[notice] 65-65: Unused global variable
The global variable 'widget' is not used.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Linux deb.ubuntu.noble with Python 3.12
- GitHub Check: Linux pip with Python 3.12
- GitHub Check: Linux pip with Python 3.11
- GitHub Check: Linux pip with Python 3.10.8
- GitHub Check: Linux pip with Python 3.13
- GitHub Check: Linux rpm.fedora.40 with Python 3.12
- GitHub Check: macos
- GitHub Check: windows
🔇 Additional comments (13)
tests/test_region_control.py (2)
17-18: Platform detection change looks good, but there's an inconsistency.The switch from
os.name == 'nt'toplatform.system() == 'Windows'is a valid approach for cross-platform detection. However,tests/test_region_expect.py(line 79) still usesos.name == 'nt'for the same purpose inshow_image(). Consider updating that file as well to maintain consistency across the test suite.Also applies to: 81-81
330-332: LGTM!The Windows skip decorators are consistently applied to typing-related tests that trigger AutoPy issues. The platform detection is now uniform across the file.
Also applies to: 339-341, 354-356
tests/test_region_expect.py (2)
124-125: Coordinate adjustments align with layout changes.The updated expected coordinates (from ~30,190 to ~20,180) are consistent with the zero-margin layout and explicit DPI settings introduced in
qt6_image.py. The image now renders closer to the top-left corner as expected.Also applies to: 131-132
185-186: LGTM!All
expected_matchescoordinate adjustments follow the same pattern astest_find, reflecting the tighter layout margins in the test application.Also applies to: 192-192, 208-208, 222-222
.github/workflows/ci.yml (1)
165-170: LGTM!Windows test isolation is properly extended with documented reasons for each disabled feature.
tests/qt6_application.py (5)
24-33: LGTM! Proper DPI configuration for cross-platform consistency.Setting these environment variables and the DPI rounding policy before
QApplicationcreation is the correct approach for consistent rendering across platforms with different display scaling.
42-49: LGTM! Font-based styling improves cross-platform consistency.Moving from stylesheet-based fonts to explicit
QFontassignments is a good practice for test applications where pixel-precise rendering matters.Also applies to: 55-55, 61-61, 66-67, 76-77, 81-81, 86-86, 92-92, 96-96, 100-100, 104-104
107-114: LGTM! Explicit device pixel ratio ensures consistent image dimensions.Setting
setDevicePixelRatio(1.0)on pixmaps ensures images render at their actual pixel dimensions regardless of display scaling, which is essential for visual testing.
119-162: LGTM! Layout standardization supports predictable widget positioning.The explicit margins and spacing values ensure consistent layout across platforms, which is critical for the coordinate-based tests in
test_region_control.py.
264-269: LGTM!Moving
QApplicationcreation inside__main__is the standard pattern and avoids issues when the module is imported elsewhere.tests/qt6_image.py (3)
17-31: LGTM! DPI configuration consistent with qt6_application.py.The environment setup mirrors the changes in
qt6_application.py, ensuring both test applications render consistently across platforms.
41-55: LGTM! Layout changes explain the coordinate adjustments in test_region_expect.py.The zero margins, explicit device pixel ratio, and top-left alignment ensure images render at consistent, predictable positions—justifying the updated expected coordinates in the tests.
58-66: Thewidgetvariable is intentionally required—static analysis is a false positive.The
widgetvariable must remain in scope to prevent theImageWithLayoutinstance from being garbage collected beforeapp.exec()completes. This is standard Qt practice. You could optionally suppress the warning or make intent clearer:- widget = ImageWithLayout(sys.argv[1]) + widget = ImageWithLayout(sys.argv[1]) # noqa: F841 - must remain in scope
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #83 +/- ##
=======================================
Coverage 89.23% 89.23%
=======================================
Files 17 17
Lines 4643 4643
=======================================
Hits 4143 4143
Misses 500 500 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pevogam
force-pushed
the
macos-coverage
branch
from
70f5708 to
14b0aa7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_region_expect.py (1)
125-126: Coordinate adjustments are justified by Qt6 rendering updates.The test coordinate expectations reflect the changes made in
qt6_application.pyandqt6_image.py, which include:
- Explicit Arial font configuration across all UI elements
- Qt 6 Per-Monitor DPI Aware V2 by default with disabled high-DPI scaling (
QT_ENABLE_HIGHDPI_SCALING=0andsetHighDpiScaleFactorRoundingPolicy(PassThrough))- Layout margin adjustments via
setContentsMargins()calls- Arial's more rounded design with softer, fuller curves and more open counters compared to other fonts affects text metrics and visual positioning
The coordinate shifts at lines 125–126, 132–133, and 186–187 (expecting (20, 180)) and the varied shifts in lines 193, 209, and 223 align with these rendering changes. The
delta=5tolerance is appropriate for UI element positioning across different rendering contexts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/ci.yml(2 hunks)tests/qt6_application.py(5 hunks)tests/qt6_image.py(2 hunks)tests/test_region_control.py(7 hunks)tests/test_region_expect.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_region_control.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Linux pip with Python 3.13
- GitHub Check: Linux pip with Python 3.10.8
- GitHub Check: Linux pip with Python 3.11
- GitHub Check: Linux deb.ubuntu.noble with Python 3.12
- GitHub Check: Linux rpm.fedora.40 with Python 3.12
- GitHub Check: Linux pip with Python 3.12
- GitHub Check: Windows with any available coverage
- GitHub Check: MacOS with any available coverage
🔇 Additional comments (13)
tests/test_region_expect.py (1)
18-18: LGTM: Platform detection refactored for clarity.The change from
os.name == 'nt'toplatform.system() == 'Windows'is more explicit and aligns with the PR's broader platform detection unification.Also applies to: 80-80
.github/workflows/ci.yml (2)
131-131: LGTM: Documentation improvements for clarity.The job name and comment updates explicitly clarify the platform-specific limitations, which aligns well with the addition of the new macOS job.
Also applies to: 166-166
173-216: No compatibility concerns with wxPython 4.2.4 on macOS.The macOS job correctly uses wxPython 4.2.4, which has official binary wheels for macOS 11.0+ and x86-64 macOS 10.13+, supporting Python 3.12. The job structure and configuration are appropriate.
tests/qt6_application.py (6)
24-33: LGTM: DPI handling ensures consistent cross-platform rendering.The environment setup disables Qt's automatic DPI scaling before QApplication creation, which is the correct approach for reproducible test rendering across platforms.
42-43: LGTM: Arial provides better cross-platform font availability.Changing from Helvetica to Arial ensures consistent font rendering across Windows, macOS, and Linux, which is essential for reproducible test coordinates.
49-49: LGTM: Explicit font objects improve maintainability.Replacing inline stylesheet font declarations with explicit
QFontobjects is a best practice that ensures consistent font application and improves code maintainability.Also applies to: 55-55, 61-61, 66-66, 76-77, 81-82, 86-87, 92-93, 96-97, 100-101, 104-104
107-115: LGTM: Explicit device pixel ratio ensures consistent image rendering.Setting
devicePixelRatioto 1.0 for both pixmaps ensures consistent image rendering across different DPI screens, which is essential for reproducible test coordinates.
120-162: LGTM: Explicit layout margins ensure consistent rendering.Adding explicit margins and spacing to all layouts ensures consistent rendering across platforms, which is essential for reproducible test coordinates.
265-265: LGTM: QApplication initialization inside main guard is correct.Placing the QApplication initialization inside the main guard is appropriate for a test script.
tests/qt6_image.py (4)
17-17: LGTM: Consistent environment setup across Qt6 test files.The environment setup matches
qt6_application.py, ensuring consistent DPI handling and rendering behavior across all Qt6 test utilities.Also applies to: 22-31
36-36: LGTM: Adding default parent parameter follows Qt conventions.Adding the
parentparameter with a default value ofNonealigns with Qt widget initialization conventions and improves flexibility.
41-55: LGTM: Zero margins and explicit device pixel ratio ensure precise coordinate testing.The horizontal layout with zero margins, combined with device pixel ratio set to 1.0, ensures that images are positioned at precise coordinates for testing purposes.
59-66: LGTM: Argument validation improves script usability.Adding argument validation with a helpful usage message prevents errors and makes the script more user-friendly.
pevogam
changed the title
pevogam
force-pushed
the
macos-coverage
branch
from
14b0aa7 to
e0fd70e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.yml(2 hunks)tests/test_controller.py(1 hunks)tests/test_region_control.py(11 hunks)tests/test_region_expect.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_region_control.py
- tests/test_region_expect.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Analyze (python)
- GitHub Check: Linux pip with Python 3.11
- GitHub Check: Linux pip with Python 3.10.8
- GitHub Check: Linux deb.ubuntu.noble with Python 3.12
- GitHub Check: Linux pip with Python 3.12
- GitHub Check: Linux rpm.fedora.40 with Python 3.12
- GitHub Check: MacOS with any available coverage
- GitHub Check: Linux pip with Python 3.13
- GitHub Check: Windows with any available coverage
🔇 Additional comments (3)
.github/workflows/ci.yml (3)
131-131: LGTM!Adding a descriptive job name improves workflow readability and is consistent with the naming pattern used for other jobs.
166-166: LGTM!The comment clarification explicitly identifies AutoPy as the source of drag/drop issues on Windows, improving documentation.
173-216: LGTM!The new macOS CI job is well-structured and consistent with the Windows job pattern. Key strengths:
- Uses Python 3.12, matching the Windows job
- Follows the same workflow structure for consistency
- Caches ML models with the same key across all platforms
- Appropriately disables platform-incompatible features (xdotool, vncdotool)
- Documents known issues (AutoPy drag/drop) and future work (OCR, PyAutoGUI) with clear comments
pevogam
force-pushed
the
macos-coverage
branch
from
e0fd70e to
0c46b96
Compare
The new variant is a simple duplicate of the windows one.
Use environment variables for the fixed DPI settings that should make pixel counts independent of the environment screen. Fix zero margins and spacing for the Qt6 image window but adapt the locations once while taking the other possible approach for the Qt6 app - keep the prevous locations but fix the margins and spacing to match the previous ones for certain default displays. Zero spacing and magins was too ugly for the Qt6 application but not that relevant for the simpler Qt6 image. Also use fixed and more universal fonts in a more standard way (not stylesheet). Additionally sort the Qt imports better and only load the Qt app if running as a script.
This comprises mostly of tests not finding the green box in the CI (but finding it in a non-CI macOS environment as well as environment with identical resolution) which need more debugging as well as problems with AutoPy handling the SHIFT key on macOS, drag/drop problems, and mouse-up and middle-click problems. Also standardize the platform checks for Windows in comparison.
This is easy enough to provide without the feedback of additional interest compared to PyAutoGUI which has a lot more problems.
pevogam
force-pushed
the
macos-coverage
branch
from
0c46b96 to
beb244c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/qt6_application.py (1)
198-203: Consider removing the redundant close call.Both
self.parent().close()(line 199) andQtWidgets.QApplication.quit()(line 203) are called. The TODO comment suggests platform inconsistencies, but calling both is redundant sincequit()will terminate the application regardless.Consider keeping only one approach:
def dragEnterEvent(self, e: QtGui.QDragEnterEvent) -> None: - self.parent().close() - # TODO: this only works on some platforms and not others despite - # identical version but likely due to different dependency versions - # self.parent().close() QtWidgets.QApplication.quit()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/ci.yml(2 hunks)tests/qt6_application.py(5 hunks)tests/qt6_image.py(2 hunks)tests/test_calibrator.py(2 hunks)tests/test_controller.py(5 hunks)tests/test_region_control.py(12 hunks)tests/test_region_expect.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- tests/test_calibrator.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Linux deb.ubuntu.noble with Python 3.12
- GitHub Check: Linux pip with Python 3.13
- GitHub Check: Linux pip with Python 3.10.8
- GitHub Check: Linux rpm.fedora.40 with Python 3.12
- GitHub Check: Linux pip with Python 3.12
- GitHub Check: Linux pip with Python 3.11
- GitHub Check: MacOS with any available coverage
- GitHub Check: Windows with any available coverage
🔇 Additional comments (35)
tests/test_region_control.py (12)
17-18: LGTM!Adding
platformimport aligns with the standardized approach for platform detection across the test suite.
80-84: LGTM!Using
platform.system() == 'Windows'is more explicit and consistent with the broader refactoring in this PR. This is preferable toos.name == 'nt'as it returns human-readable platform names.
119-121: LGTM!The skip annotation with a descriptive reason for macOS-specific AutoPy behavior is appropriate.
151-158: LGTM!Skip annotation for macOS middle-click issues is appropriate with clear reasoning.
182-184: LGTM!Consistent Darwin skip pattern for green box detection issue.
196-198: LGTM!Consistent Darwin skip pattern.
228-231: LGTM!Skip annotation for macOS mouse up issues with clear reasoning.
319-319: LGTM!Darwin skip for SHIFT key handling issue is appropriate.
332-334: LGTM!Combined skip reason covers both SHIFT and green box detection issues on macOS.
343-345: LGTM!Consistent migration from
os.name == 'nt'toplatform.system() == 'Windows'.
352-354: LGTM!Consistent Windows platform check.
367-369: LGTM!Consistent Windows platform check.
tests/test_region_expect.py (6)
17-18: LGTM!Adding
platformimport aligns with the standardized platform detection approach.
78-83: LGTM!Consistent use of
platform.system() == 'Windows'for Python interpreter selection.
125-135: Verify the updated expected coordinates align with Qt6 layout changes.The expected coordinates changed from
(30, 190)to(20, 180). This appears to correspond to the layout margin and spacing adjustments made inqt6_image.py(zero margins and explicit DPI settings).
186-193: Updated coordinates reflect Qt6 DPI/layout fixes.The expected match coordinates have been consistently adjusted. The changes from
(25, 25)to(15, 15)etc. align with the zero-margin layout configuration in the Qt6 image viewer.
209-209: LGTM!Expected matches updated consistently with other coordinate changes.
223-223: LGTM!Expected matches updated consistently.
tests/test_controller.py (5)
17-18: LGTM!Adding
platformimport for standardized platform detection.
143-147: LGTM!Consistent use of
platform.system() == 'Windows'for Python interpreter selection.
264-267: LGTM!Runtime skip for macOS instant mouse move issues is appropriate. Using a
continuestatement within the loop is the correct approach here since@unittest.skipIfcan't selectively skip loop iterations.
325-328: LGTM!Runtime skip for macOS mouse up issues is appropriate within the loop context.
386-388: LGTM!Consistent migration from
os.name == 'nt'toplatform.system() == 'Windows'.tests/qt6_application.py (7)
17-18: LGTM!Import reordering to QtCore, QtGui, QtWidgets follows the conventional order from low-level to high-level modules.
24-33: LGTM!Setting environment variables and DPI scaling policy before QApplication creation is the correct approach for consistent cross-platform rendering. This should resolve DPI-related test flakiness.
42-44: LGTM!Arial is a more widely available cross-platform font than Helvetica, which may not be present on all systems.
46-67: LGTM!Replacing inline styles with explicit
setFont()calls is cleaner and more maintainable. The consistent font application across widgets ensures uniform appearance.
107-114: LGTM!Explicitly setting device pixel ratio to 1.0 on QPixmap objects ensures consistent image sizes across different display configurations, which directly addresses cross-platform test reliability.
119-162: LGTM!Explicit layout margins and spacing configuration provides predictable widget positioning, which is essential for coordinate-based testing. The small margins (2-10px) and consistent spacing align with the updated expected coordinates in the test files.
264-269: LGTM!Moving QApplication creation inside the
if __name__ == "__main__":guard is best practice. This prevents application instantiation when the module is imported, which aligns with the changes inqt6_image.py.tests/qt6_image.py (5)
17-19: LGTM!Added
osimport for environment variable access and reordered imports consistently withqt6_application.py.
22-31: LGTM!Environment setup and DPI scaling policy configuration mirrors
qt6_application.py, ensuring consistent behavior across both Qt6 test applications.
36-36: LGTM!Adding a default
parentparameter withNonefollows PyQt conventions and allows the widget to be used as a top-level window.
41-56: LGTM!The explicit QPixmap creation with
setDevicePixelRatio(1.0)and zero-margin layout configuration ensures consistent image positioning. This directly explains the coordinate adjustments intest_region_expect.py(e.g., from(30, 190)to(20, 180)).
58-67: LGTM!Moving QApplication creation inside the main guard with proper argument validation is best practice. The usage message provides clear feedback when the image path is missing. The past review comment about an unused
widgetglobal variable appears to be resolved—this variable is no longer present in the current code.
1 participant
Add minimal MacOS coverage and changes needed for more passing tests.
Summary by CodeRabbit
New Features
Tests
Style
Behavior
✏️ Tip: You can customize this high-level summary in your review settings.