Skip to content

Compute code coverage correctly on platform-specific files, no-cover some trivial things, remove caching of .venv. #144

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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

johnzhou721
Copy link
Contributor

@johnzhou721 johnzhou721 commented Jul 27, 2025

See title.

Caching of .venv is removed because we frequently get incompatible architechtures -- the arch is not included in the caching and it does not save much time anyway since PyQt is always reinstalled.

Comment on lines 80 to 103
QtWidgets = importlib.import_module(QtModuleName + ".QtWidgets", package=QtModuleName)
QApplication = QtWidgets.QApplication

if QtModuleName == "PyQt5":
from PyQt5 import QtWidgets
if QtModuleName == "PyQt5": # pragma: no cover
from PyQt5.QtCore import pyqtSlot as Slot

QApplication = QtWidgets.QApplication

elif QtModuleName == "PyQt6":
from PyQt6 import QtWidgets
elif QtModuleName == "PyQt6": # pragma: no cover
from PyQt6.QtCore import pyqtSlot as Slot

QApplication = QtWidgets.QApplication

elif QtModuleName == "PySide2":
from PySide2 import QtWidgets
elif QtModuleName == "PySide2": # pragma: no cover
from PySide2.QtCore import Slot

QApplication = QtWidgets.QApplication

elif QtModuleName == "PySide6":
from PySide6 import QtWidgets
elif QtModuleName == "PySide6": # pragma: no cover
from PySide6.QtCore import Slot

QApplication = QtWidgets.QApplication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW: I simplified the importing of QApplication in the process; importing Slot gets no-covered because there's really nothing we need to test here; adding PyQt5 etc conditionals would be a huge waste of time.

@johnzhou721
Copy link
Contributor Author

I also enabled printing missed branches so it's easier for us to know where to fill the gaps in.

@johnzhou721 johnzhou721 changed the title Compute code coverage correctly on platform-specific files, no-cover some trivial things. Compute code coverage correctly on platform-specific files, no-cover some trivial things, remove caching of .venv. Jul 27, 2025
@hosaka
Copy link
Collaborator

hosaka commented Jul 28, 2025

Slot import should be reduced to

Slot = getattr(QtCore, "pyqtSlot", None) or getattr(QtCore, "Slot")

ProcessEventFlags should be usable inline I think, without the need to if guard it. Although I recall it broke tests, so maybe using importlib is better.

        # run loop one last time to process all the events
        self.__app.eventDispatcher().processEvents(
            QtCore.QEventLoop.ProcessEventsFlags(0x00)
        )

QT_API env var does get used in the CI in all configurations, I see no reason to "no cover" it. I don't want to have comments explaining coverage and tests in the __init__.py these are just noise and are not relevant to the functionality, especially at the top of the file.

@johnzhou721 johnzhou721 marked this pull request as ready for review July 29, 2025 20:08
QtModuleName = env_to_mod_map[qtapi_env]
else:
except KeyError as e: # pragma: no cover
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW: I made this use try except for an extremely minor speed up — you don’t need to check for existness in the dict first when it exists — and from my experience it’s more pythonic to ask for forgiveness like this.

@johnzhou721 johnzhou721 requested a review from hosaka July 29, 2025 21:12
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.

2 participants