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

Difference between `pytestqt.exceptions.TimeoutError` and Python's `TimeoutError` #459

Closed bersbersbers closed 2 years ago

bersbersbers commented 2 years ago

Python has a TimeoutError already: https://docs.python.org/3/library/exceptions.html#TimeoutError

What is the purpose of pytestqt.exceptions.TimeoutError, which is not derived from TimeoutError? It is very easy write a

try:
  ...
except  TimeoutError:
  pass

that does not work as intended because one is missing from pytestqt.exceptions import TimeoutError (which by the way gives:

Redefining built-in 'TimeoutError'

in pylint).

I think pytestqt.exceptions.TimeoutError could be removed or, if it should be kept for compatibility, it should derive from TimeoutError.

The-Compiler commented 2 years ago

Python's TimeoutError is a subclass of OSError, and its description is:

Raised when a system function timed out at the system level. Corresponds to errno ETIMEDOUT.

What's timing out in pytest-qt is not related to system functions, OS calls or errno in any way. pytest-qt raising an OSError for something not related to the OS at all seems like unnecessarily confusing to me, so I'm -1 on this.

nicoddemus commented 2 years ago

I agree with @The-Compiler.

However we could rename it to something else to avoid confusion, say QtTimeoutError, or something like that.

The-Compiler commented 2 years ago

It does seem like a bit of a footgun, given that having from pytestqt.exceptions import TimeoutError would also break other code trying to catch the OSError one.

However, I feel like there are some points against that too:

bersbersbers commented 2 years ago

Agree with breaking other TimeoutErrors - that was not the best example. The main issue, however, is in code that does not use this import, which is what made me report this.

I see two more ways of avoiding confusion:

  1. Move TimeoutError from pytestqt.exceptions to just pytestqt, so one can catch pytestqt.TimeoutError (one would not want to catch pytestqt.exceptions.TimeoutError, I guess; nor from pytestqt import exceptions)
  2. Use pytestqt.TimeoutError (or QtBot.TimeoutError, just noticed that) explicitly in all docstrings.
The-Compiler commented 2 years ago

Given that it's rather uncommon to import things from pytest plugins (other than for type annotations nowadays, I suppose), I think the best way forward is to make it clear that qtbot.TimeoutError is the preferred way of using the exception. I can see we use TimeoutError without qtbot. right in the first code snippet in the docs, that could probably be improved. Other than that, we seem to be pretty consistently use qtbot.TimeoutError already.

One last question, however: What's your usecase for actually catching it? Off-hand, I can't really think of any situation where that would make sense. I'm wondering if we could have some kind of better API in the first place for whatever it is that you're doing?

nicoddemus commented 2 years ago

:+1: to just advertise using qtbot.TimeoutError.

bersbersbers commented 2 years ago

I can see we use TimeoutError without qtbot. right in the first code snippet in the docs

Yes, this is the one I read as well :) I am perfectly fine with qtbot.TimeoutError. Maybe add a "Tip" box to advertise it?

One last question, however: What's your usecase for actually catching it? Off-hand, I can't really think of any situation where that would make sense. I'm wondering if we could have some kind of better API in the first place for whatever it is that you're doing?

I was implementing waits after sending a key event (using pyautogui) to a QWindow. Based on some fixed_wait parameter, I need to wait for either a signal to be emitted or some fixed amount of time to pass. I thought I might combine both approaches into a single waitSignal function call, and experimented with except TimeoutError until I discovered raising=False. If that did not exist, this would be the answer to your question :)