Fix test module imports and add macOS support #21
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes critical test failures caused by incorrect module import paths and adds cross-platform support for macOS. The changes address issue #17 which was causing 130+ test failures due to the module reorganization from backend.* to services.* and routers.*.
Key changes:
- Updated all test files to use correct module paths (
services.*androuters.*instead ofbackend.*) - Added platform-specific window detection for macOS using pyobjc-framework-Quartz
- Provided macOS setup documentation and helper scripts for Tkinter configuration
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_vector_service.py | Updated patch statements to reference services.vector_service instead of backend.vector_service |
| tests/unit/test_screenshot.py | Fixed all test patches to use services.screenshot module path |
| tests/unit/test_game_detection.py | Corrected imports to use services.screenshot.get_recent_screenshots |
| tests/unit/test_chatbot.py | Updated all mock patches to reference services.chatbot module |
| tests/unit/test_backend.py | Fixed API test patches to use correct router modules (routers.chat, routers.screenshot, etc.) |
| services/screenshot.py | Implemented platform-specific window detection with macOS support via Quartz framework |
| reinstall_python_tkinter.sh | Added helper script for macOS Tkinter installation |
| pyproject.toml | Added conditional macOS dependency for pyobjc-framework-Quartz |
| INSTALL.md | Added comprehensive macOS setup instructions and troubleshooting guide |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
reinstall_python_tkinter.sh
Outdated
|
|
||
| echo "" | ||
| echo "Installation complete! Testing Tkinter..." | ||
| python3 -m tkinter && echo "✓ Tkinter is working!" || echo "✗ Tkinter installation failed" |
There was a problem hiding this comment.
The script tests python3 but pyenv may not have set this as the default. The test should use the pyenv-installed Python explicitly, such as $(pyenv which python) -m tkinter to ensure it's testing the correct Python installation.
| python3 -m tkinter && echo "✓ Tkinter is working!" || echo "✗ Tkinter installation failed" | |
| $(pyenv which python) -m tkinter && echo "✓ Tkinter is working!" || echo "✗ Tkinter installation failed" |
|
I will be testing the code locally on my system once, before I accept the PR, meanwhile it appears that some test cases are unable to run, due to it referencing the wrong branch |
INSTALL.md
Outdated
|
|
||
| #### Prerequisites: Tkinter Support | ||
|
|
||
| Pixly requires Tkinter for the GUI overlay. If you're using `pyenv`, you need to install Python with Tkinter support: |
There was a problem hiding this comment.
Documentation is incorrect, it doesn't use Tkinter, it uses CustomTkinter both are two different libraries.
Please revert back this documenation and submit another pull request with the corrections made.
| chmod +x reinstall_python_tkinter.sh | ||
| ./reinstall_python_tkinter.sh | ||
| ``` | ||
|
|
There was a problem hiding this comment.
159-166 is not required
reinstall_python_tkinter.sh
Outdated
There was a problem hiding this comment.
this setup file isnt required
BrataBuilds
left a comment
BrataBuilds
left a comment
There was a problem hiding this comment.
Please go through the requested changes
Sure, I'm fixing this. |
…olete script - Update documentation to clarify that Pixly uses CustomTkinter (not just Tkinter) - Remove references to reinstall_python_tkinter.sh script which is no longer needed - Simplify macOS troubleshooting with direct brew install command - Clarify that Tkinter is the underlying library required by CustomTkinter
|
Test cases are still failing, testing it on locally i am finding more incorrect imports. |
2 participants
Description
Please include a summary of the change and which issue is fixed. List any dependencies that are required for this change.
Fixes # (issue) (only if applicable)
Fix test module imports and add macOS support
Resolves #17 - Fixes 130+ failing test cases due to incorrect module import paths.
Changes
🔧 Test Fixes
🍎 macOS Support
Testing
Tests now import correctly and can be executed. Module import errors resolved.
Type of Change
Please delete options that are not relevant.
Checklist:
Screenshots (if applicable)
Please include any relevant screenshots or images that help explain the changes made.