pytest-dev / pytest-qt

pytest plugin for Qt (PyQt5/PyQt6 and PySide2/PySide6) application testing
https://pytest-qt.readthedocs.io
MIT License
409 stars 70 forks source link

Dynamically generated QTest methods not friendly to type checkers and IDEs #333

Closed TilmanK closed 3 years ago

TilmanK commented 3 years ago

When writing tests with QtBot (which is a blessing!) I also annotate my tests to easily find errors when writing them.

As an example a test definition looks like this: def test_filtering(qtbot: QtBot) -> None:

I've silenced mypy for pytest-qt anyway, but PyCharm keeps complaining about QtBot missing methods (that originate from the QTest api).

From my point of view (which might be limited and not complete) it would be sufficient to add definitions to QtBot to tell PyCharm (and other IDEs?) what to expect:

def mouseClick(self, button, mouse_button, stateKey=0, pos=None, delay=-1):...

I'd send a PR for this, if ok.

nicoddemus commented 3 years ago

Hi @TilmanK,

Indeed right now QTest methods are injected dynamically:

https://github.com/pytest-dev/pytest-qt/blob/f3ec8653357bb3de886c2b36ff5be243955f11db/src/pytestqt/qtbot.py#L600-L642

It would be great to define them explicitly to help type checkers and IDEs. 👍

(I've updated the description, hope you don't mind)

TilmanK commented 3 years ago

Great @nicoddemus!

Is the definition as mentioned above sufficient? Or do we need something else?

nicoddemus commented 3 years ago

Should be enough (of course for all methods plus calling the actual implementation). 👍

TilmanK commented 3 years ago

We could do that, but with the implementation as it is today, the method definitions are overwritten at runtime (as far as I understand) by the existing code. Is there any benefit in calling them from the definitions to come?

nicoddemus commented 3 years ago

I think the approach is to replace the current implementation, which creates proxy methods:

@classmethod
def _inject_qtest_methods(cls):
    # inject methods from QTest into QtBot 
    method_names = [ 
        "keyPress",
        "mouseClick", 
        ...
    ]
    if hasattr(qt_api.QtTest.QTest, "keySequence"): 
        # Added in Qt 5.10 
        method_names.append("keySequence") 

    for method_name in method_names: 
        method = create_qtest_proxy_method(method_name) 
        if method is not None: 
            setattr(cls, method_name, method)      

By explicit calls:

@staticmethod
def keyPress(widget, key, modifier=0, delay=-1) -> None:
    qt_api.QtTest.QTest.keyPress(widget, key, modifier, delay)

@staticmethod
def mouseClick(widget, mouse_button, stateKey=0, pos=None, delay=-1) -> None:
    qt_api.QtTest.QTest.mouseClick(widget, mouse_button, stateKey, pos, delay)    

...
TilmanK commented 3 years ago

Yeah, more tricky than I expected it to be. I can't pass i.e. a None for the pos. I have to pass a QPoint() which I can't define as default argument since the qtbot api isn't loaded when importing QtBot and its method definitions. I could do something like this which is ugly, but at least it works:

@staticmethod
def mouseClick(widget, button, modifier=0, pos=None, delay=-1):
    if modifier == 0:
        modifier = qt_api.Qt.NoModifier
    if pos is None:
        pos = qt_api.QtCore.QPoint()
    qt_api.QtTest.QTest.mouseClick(widget, button, modifier, pos, delay)

Any suggestions?

The-Compiler commented 3 years ago

Maybe use *args, **kwargs instead? I suppose it'll make autocompletion a bit worse, but at least fix the "missing method" issues. Also it'd make sure we actually keep the same signature as Qt for all of those.

altendky commented 3 years ago

One if doesn't seem so terrible. Though sure, other things will come up. Why the if modifier == 0: instead of just defaulting to the NoModifier? If you don't want to pass integers through then... I guess either type check or convert everything.

TilmanK commented 3 years ago

@The-Compiler, yes I've thought about that, too. It's more or less what we have but without the problem of the missing methods.

@altendky Well we'd have to use one or two of the if statements for each of the twelve methods. I can't use NoModifier as a default because that would create the necessity of an import which is impossible with the current architecture. When qtbot is imported, the qtapi hasn't yet created all of its attributes from which I'd have to do the import...

I'd go with @The-Compiler's suggestion and use *args, **kwargs instead, if no one has another idea. It's not like we're making things worse...

altendky commented 3 years ago

@TilmanK right... you said that the first time... :[ Thanks for setting me straight.

If the plan is to minimize the change to the runtime code then there could be inline stubs masked by if typing.TYPE_CHECKING:. Maybe stubtest (in Mypy) could help checking the hints vs. the runtime. I know it does something, I don't know the details though. But sure, adding parameter hinting could be a separate task from adding method-existence hinting.

TilmanK commented 3 years ago

@altendky Yeah, I guess type hinting is another task. But I've already thought about it. If we used *args, **kwargs together with type hints that would work, wouldn't it?

altendky commented 3 years ago

Here's one option, yeah.

https://mypy-play.net/?mypy=latest&python=3.9&gist=da7eef1a5cdec3355a7f27ab34d2d208

import typing

if typing.TYPE_CHECKING:
    def f(x: int, y: str = ...) -> bytes: ...
else:
    def f(*args, **kwargs):
        return None

reveal_type(f)
main.py:11: note: Revealed type is 'def (x: builtins.int, y: builtins.str =) -> builtins.bytes'

The hints could also be provided similarly while leaving the existing method-generation code in place.

https://mypy-play.net/?mypy=latest&python=3.9&gist=803f5add3cd3845ca52d4c25e6950fce

import typing

if typing.TYPE_CHECKING:
    def f(*args: object, **kwargs: object) -> object: ...

# method generation code here

reveal_type(f)
main.py:10: note: Revealed type is 'def (*args: builtins.object, **kwargs: builtins.object) -> builtins.object'

I don't know... it seems like there's enough wrapping that happens in Python that this ought to be a well explored and solved scenario, but it doesn't seem to be. :[ Anyways, whichever path is probably fine.

TilmanK commented 3 years ago

That sounds interesting. Nevertheless, how do we annotate the types if we don't know which Qt bindings to use? We'd need to get that dynamically right?

altendky commented 3 years ago

Do string hints not cut it?

if typing.TYPE_CHECKING:
    def f(x: "qt_api.Qt.Something") -> None: ...

I haven't looked at the specific scenario here.

TilmanK commented 3 years ago

qt_api is loaded in pytest_configure - which isn't run while type checking. Maybe we could load the qt_api when type checking though somewhere early while importing...

nicoddemus commented 3 years ago

I agree we can just forward the calls using *args and **kwargs, didn't realize qt_api indeed is not available during type-checking time. 👍