Skip to content
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

migrate to qtpy #478

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

Conversation

nrbnlulu
Copy link

@nrbnlulu nrbnlulu commented Feb 1, 2023

closes #477

@nrbnlulu
Copy link
Author

nrbnlulu commented Feb 1, 2023

@nicoddemus Can you tell me which tests are redundant by now?

@nicoddemus
Copy link
Member

@nicoddemus Can you tell me which tests are redundant by now?

I think the only test that is checking the implementation details related to qt-compat is test_already_loaded_backend (so it can be dropped), however the others should be adapted because they test user-facing functionality.

Which Qt backend to use is part of the user configuration, so it should remain tested.

@@ -566,7 +566,7 @@ def _fake_import(name, *args):
def _fake_is_library_loaded(name, *args):
return False

monkeypatch.delenv("PYTEST_QT_API", raising=False)
Copy link
Member

Choose a reason for hiding this comment

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

Note that we need to keep supporting PYTEST_QT_API for backward compatibility, so we should not change these tests.

@@ -4,6 +4,7 @@ envlist = py{37,38,39,310}-{pyqt5,pyside2,pyside6,pyqt6}, linting
[testenv]
deps=
pytest
qtpy
Copy link
Member

Choose a reason for hiding this comment

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

This should not be needed because qtpy is already a dependency of pytest-qt itself (now that I noticed it, pytest above is not needed either).

Copy link
Author

Choose a reason for hiding this comment

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

IIRC tests were failing without it

Copy link
Member

Choose a reason for hiding this comment

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

Weird, can you double check? qtpy is already a dependency for the package, we should not have to duplicate it here... 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Ahh in case you were already running tox, try passing -r for it to create a fresh environment.

@nrbnlulu
Copy link
Author

nrbnlulu commented Feb 2, 2023

@nicoddemus Can you tell me which tests are redundant by now?

I think the only test that is checking the implementation details related to qt-compat is test_already_loaded_backend (so it can be dropped), however the others should be adapted because they test user-facing functionality.

Which Qt backend to use is part of the user configuration, so it should remain tested.

We can refer to qtpy docs instead no?

@nicoddemus
Copy link
Member

nicoddemus commented Feb 2, 2023

We can refer to qtpy docs instead no?

I would prefer to avoid breaking backward compatibility... probably it is a simple matter of:

if "PYTEST_QT_API" in os.environ:
    os.environ["QT_API"] = os.environ["PYTEST_QT_API"]

Before importing qtpy?

qt_api.set_qt_api(config.getini("qt_api"))


def pytest_report_header():
Copy link
Member

Choose a reason for hiding this comment

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

This should definitely stay - it's very useful information for test runs, and it missing also currently breaks a lot of self tests.

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.

Use qtpy instead of custom compatibility code
3 participants