Skip to content

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

Closed
johnzhou721 wants to merge 12 commits into
CabbageDevelopment:masterfrom
johnzhou721:cover
Closed

Compute code coverage correctly on platform-specific files, no-cover some trivial things, remove caching of .venv.#144
johnzhou721 wants to merge 12 commits into
CabbageDevelopment:masterfrom
johnzhou721:cover

Conversation

@johnzhou721

@johnzhou721 johnzhou721 commented Jul 27, 2025

Copy link
Copy Markdown
Contributor

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 thread qasync/__init__.py
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
Copy Markdown
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.

Comment thread tests/conftest.py
Comment thread qasync/__init__.py Outdated
@johnzhou721

Copy link
Copy Markdown
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

hosaka commented Jul 28, 2025

Copy link
Copy Markdown
Collaborator

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
Comment thread qasync/__init__.py
try:
QtModuleName = env_to_mod_map[qtapi_env]
else:
except KeyError as e: # pragma: no cover

Copy link
Copy Markdown
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
@johnzhou721

Copy link
Copy Markdown
Contributor Author

Too many conflicts, closings.

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