Add waitExposed and waitActive methods to QtBot #159

nicoddemus · 2016-09-16T01:33:55Z

Added waitForWindowExposed for PyQt5.

Fixes #158

coveralls · 2016-09-16T01:39:03Z

Coverage decreased (-1.8%) to 98.168% when pulling ace332e on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

coveralls · 2016-09-16T01:39:03Z

Coverage decreased (-1.8%) to 98.168% when pulling ace332e on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

coveralls · 2016-09-16T01:39:03Z

Coverage decreased (-1.8%) to 98.168% when pulling ace332e on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

coveralls · 2016-09-16T01:48:51Z

Coverage remained the same at 100.0% when pulling 7289785 on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

nicoddemus · 2016-09-16T01:53:44Z

I plan to release 2.1 after this is merged.

The-Compiler · 2016-09-16T08:48:07Z

I think we still have some things to consider here, ideally before releasing it as they're API changes - unsorted braindump as I don't have much time right now:

Probably should have qWaitForWindowActive too?
Why not make it raise on a timeout, with an optional raising=False? If we already have a Python wrapper with a (slightly) different name, let's make the API more pythonic and don't force the caller to check a return value, IMHO.
In the same vein, should it be a context manager? IMHO, this looks better:

with qtbot.waitForWindowExposed(window):
    window.show()
assert window.isVisible()

compared to:

window.show()
qtbot.waitForWindowExposed(window)
assert window.isVisible()

nicoddemus · 2016-09-16T11:03:10Z

Good point about waitForWindowActive... I should also review if there are other additions to QTest that should go in.

I agree having a higher level API is nice, but perhaps we should add them as separate methods rather than changing the original functions? I would rather keep the original QTest functions "as is" instead of adding additional behavior; it might be a little surprising to users and makes converting C++ examples to Python easier (for example as we did in the qmodeltester fixture).

Following your example, how about those context managers then:

def waitExposed(self, widget, timeout=1000, raising=True):
    pass
def waitActive(self, widget, timeout=1000, raising=True):
    pass
def waitFocus(self, widget, timeout=1000, raising=True):
    pass

The last one in particular is tricky to achieve in my experience, specially when executing under pytest-xdist.

Also, I think those high-level APIs are only worthwhile doing for PyQt5... let's move forward only. 😁

The-Compiler · 2016-09-26T21:00:57Z

FWIW we already change the function names by stripping the leading q 😉

I guess adding those would work. No big opinion on backends, I think nobody should use Qt4 nowadays anyways 😆

I also thought about having qtbot.waitForWindowActive(...) act as before, but the context manager form raising by default, but I guess that'd be kind of weird too.

nicoddemus · 2016-09-26T21:02:51Z

OK! Thanks for coming back to this. 😁

I will see if I can tackle this during the week.

nicoddemus · 2016-10-12T20:08:18Z

Hey @The-Compiler,

I implemented waitForWindowActive as we discussed.

After implementing the waitExposed and waitActive context managers as we discussed, I'm not sure they are any better than the actual functions.

Consider:

widget.show()
assert qtbot.waitForWindowExposed(widget, timeout)

vs:

with qtbot.waitExposed(widget, timeout):
    widget.show()

I feel we don't gain much and will bloat the API without compelling reason.

What do you think?

coveralls · 2016-10-12T20:13:21Z

Coverage decreased (-2.08%) to 97.92% when pulling c93a69c on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

coveralls · 2016-10-12T20:20:57Z

Coverage decreased (-2.08%) to 97.92% when pulling 193fc3f on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

coveralls · 2016-10-12T20:20:58Z

Coverage decreased (-2.08%) to 97.92% when pulling 193fc3f on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

The-Compiler · 2016-10-14T04:57:07Z

The desire for those two things is actually because I make those mistakes a lot. Accidentally using it as a context manager isn't that bad, accidentally thinking it'd be raising is 😉

The reason I make those mistakes over and over again is because the API is inconsistent - I can use other qtbot.wait* methods as a context manager and they'll raise when something is wrong, so I think it's a cognitive burden to have some wait* methods behave one way and some wait* methods behave differently.

FWIW, I think the cleanest design/API would be to have convenience wrappers as qtbot.wait*, and all the QTest methods as qtbot.qtest.* (or a separate qtest fixture even) - but I guess that's 3.0 material 😉

nicoddemus · 2016-10-14T11:30:19Z

The reason I make those mistakes over and over again is because the API is inconsistent - I can use other qtbot.wait* methods as a context manager and they'll raise when something is wrong, so I think it's a cognitive burden to have some wait* methods behave one way and some wait* methods behave differently.

Hmm I see, I agree that those are all valid points.

Well, since we are talking about adding two news methods, waitForWindowExposed and waitForWindowActive, we might decide to not expose them directly and expose a version we find more user-friendly:

def waitActive(self, widget, timeout=1000):
    """
    Raises a TimeoutError if the widget does not show in timeout miliseconds. 
    Calls the QTest.qWaitForWindowActive.
    PyQt5 only.
    """

Similarly for waitExposed.

We might think about adding a raising=True parameter, but I'm not sure it is a common need to ensure something is not being activated/exposed which is the only situation I can think of someone wanting to pass raising=False and observe the returned value, so I would rather avoid "bloating" the API. It is always simpler to add it later than having to deprecate/remove something.
Perhaps those should be context managers, for consistency with the other wait-signal methods?

FWIW, I think the cleanest design/API would be to have convenience wrappers as qtbot.wait_, and all the QTest methods as qtbot.qtest._ (or a separate qtest fixture even)

I agree it is cleaner, but I'm not sure it is better because it is not as convenient to use. Forcing to use qtbot.qtest.mouseClick instead of qtbot.mouseClick doesn't seem like a good trade-off in this case. Plus it would break all test suites and would be a little annoying to fix. Don't know, I would have to think about it some more.

Thanks as always for the thoughtful feedback! 😉

The-Compiler · 2016-10-16T15:29:38Z

I agree about all your points! I think they definitely should be context managers for consistency, and I agree we can just default to raising=True and not add raising until someone has a good reason why it's needed 😉

Fix pytest-dev#158

coveralls · 2016-10-18T22:49:59Z

Coverage decreased (-2.6%) to 97.442% when pulling a8e461c on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

coveralls · 2016-10-18T23:04:04Z

Coverage decreased (-2.6%) to 97.449% when pulling 36c261d on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

coveralls · 2016-10-18T23:17:41Z

Coverage decreased (-2.6%) to 97.368% when pulling 0dccfcc on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

coveralls · 2016-10-18T23:38:46Z

Coverage decreased (-2.6%) to 97.368% when pulling 7d48707 on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

coveralls · 2016-10-19T00:00:09Z

Coverage decreased (-2.6%) to 97.368% when pulling a3bc31f on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

coveralls · 2016-10-19T00:11:52Z

Coverage remained the same at 100.0% when pulling 5de1611 on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

coveralls · 2016-10-19T00:11:52Z

Coverage remained the same at 100.0% when pulling 5de1611 on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

Related to pytest-dev#160

coveralls · 2016-10-19T00:19:30Z

Coverage decreased (-0.08%) to 99.915% when pulling 502c290 on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

nicoddemus · 2016-10-21T19:42:21Z

@The-Compiler I know you are pretty buys, just a ping to let you know this is ready for reviewing. 😁

The-Compiler

Some minor stuff, and a SignalTimeout API change which is probably more important 😉

The-Compiler · 2016-10-24T04:38:29Z

pytestqt/qtbot.py


+    def waitActive(self, widget, timeout=1000):
+        """
+        Context manager that waits for timeout milliseconds or until the window is active.


Shouldn't the timeout be monospace?

The-Compiler · 2016-10-24T04:39:54Z

pytestqt/qtbot.py


+    def waitActive(self, widget, timeout=1000):
+        """
+        Context manager that waits for timeout milliseconds or until the window is active.


Shouldn't the timeout be monospace?

The-Compiler · 2016-10-24T04:40:05Z

pytestqt/qtbot.py

+                show_action()
+
+        :param QWidget widget:
+            Widget to wait on.


The-Compiler · 2016-10-24T04:40:15Z

pytestqt/qtbot.py

+            Widget to wait on.
+
+        :param int|None timeout:
+            How many miliseconds to wait on.


miliseconds -> milliseconds; on -> for?

The-Compiler · 2016-10-24T04:42:13Z

pytestqt/qtbot.py

+
+    wait_active = waitActive  # pep-8 alias
+
+    def waitExposed(self, widget, timeout=1000):


Same changes as above in this docstring 😉

The-Compiler · 2016-10-24T04:42:54Z

pytestqt/qtbot.py


-        .. note:: In Qt5, the actual method called is qWaitForWindowExposed,
-            but this name is kept for backward compatibility
+        .. note:: In ``PyQt5`` this function is considered deprecated in favor of :meth:`waitForWindowExposed`.


Should we mention waitExposed instead?

The-Compiler · 2016-10-24T04:44:47Z

pytestqt/qtbot.py



-# provide easy access to SignalTimeoutError to qtbot fixtures
+class TimeoutError(Exception):


What about inheriting from SignalTimeoutError (for backwards compatibility) and deprecating SignalTimeoutError, like discussed in #157 (comment) some while ago?

Done, well remembered! 😁

The-Compiler · 2016-10-24T04:49:20Z

pytestqt/qtbot.py

+                    msg = 'widget {} not {} in {} ms.'.format(self._widget, self._adjective_name, self._timeout)
+                    raise TimeoutError(msg)
+        finally:
+            self._widget = None


Is this to avoid leaking memory?

nicoddemus · 2016-10-24T20:39:28Z

I implemented all the changes you commented @The-Compiler!

coveralls · 2016-10-24T20:43:36Z

Coverage remained the same at 100.0% when pulling 24702b7 on nicoddemus:wait-window-params into dfbd99d on pytest-dev:master.

The-Compiler · 2016-10-25T08:51:07Z

Thanks! 👍

nicoddemus force-pushed the wait-window-params branch from ace332e to 7289785 Compare September 16, 2016 01:44

nicoddemus assigned The-Compiler Sep 16, 2016

nicoddemus added 3 commits October 18, 2016 20:36

Add CHANGELOG entry for pytest-dev#153

bc211c2

Move timer fixture to conftest.py and support generic callbacks

5d36883

Add waitActive and waitExposed methods

a8e461c

Fix pytest-dev#158

nicoddemus force-pushed the wait-window-params branch from 193fc3f to a8e461c Compare October 18, 2016 22:44

nicoddemus changed the title ~~Add waitForWindowExposed method~~ Add waitExposed and waitActive methods to QtBot Oct 18, 2016

Alias captureExceptions as capture_exceptions

3a7c377

pep-8 aliases for waitActive and waitExposed

b8f32a5

nicoddemus force-pushed the wait-window-params branch from 36c261d to 0dccfcc Compare October 18, 2016 23:12

nicoddemus force-pushed the wait-window-params branch from a3bc31f to 5de1611 Compare October 19, 2016 00:06

nicoddemus mentioned this pull request Oct 19, 2016

waitForWindowShown doesn't handle timeout #158

Closed

waitActive test expected to fail on Travis

502c290

Related to pytest-dev#160

nicoddemus force-pushed the wait-window-params branch from 5de1611 to 502c290 Compare October 19, 2016 00:14

The-Compiler requested changes Oct 24, 2016

View reviewed changes

nicoddemus added 2 commits October 24, 2016 18:20

Small adjustments as discussed in review

8f32b33

Change SignalTimeoutError to TimeoutError

24702b7

The-Compiler approved these changes Oct 25, 2016

View reviewed changes

The-Compiler merged commit 894a827 into pytest-dev:master Oct 25, 2016

The-Compiler mentioned this pull request May 31, 2021

API break suggestions for 4.0 #306

Closed

10 tasks


		wait_active = waitActive # pep-8 alias

		def waitExposed(self, widget, timeout=1000):



		# provide easy access to SignalTimeoutError to qtbot fixtures
		class TimeoutError(Exception):

Uh oh!

Add waitExposed and waitActive methods to QtBot #159

Add waitExposed and waitActive methods to QtBot #159

Uh oh!

Conversation

nicoddemus commented Sep 16, 2016

Uh oh!

coveralls commented Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Sep 16, 2016

Uh oh!

coveralls commented Sep 16, 2016

Uh oh!

coveralls commented Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoddemus commented Sep 16, 2016

Uh oh!

The-Compiler commented Sep 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoddemus commented Sep 16, 2016

Uh oh!

The-Compiler commented Sep 26, 2016

Uh oh!

nicoddemus commented Sep 26, 2016

Uh oh!

nicoddemus commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 12, 2016

Uh oh!

The-Compiler commented Oct 14, 2016

Uh oh!

nicoddemus commented Oct 14, 2016

Uh oh!

The-Compiler commented Oct 16, 2016

Uh oh!

coveralls commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 19, 2016

Uh oh!

coveralls commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicoddemus commented Oct 21, 2016

Uh oh!

The-Compiler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

coveralls commented Sep 16, 2016 •

edited

Loading

coveralls commented Sep 16, 2016 •

edited

Loading

The-Compiler commented Sep 16, 2016 •

edited

Loading

nicoddemus commented Oct 12, 2016 •

edited

Loading

coveralls commented Oct 12, 2016 •

edited

Loading

coveralls commented Oct 12, 2016 •

edited

Loading

coveralls commented Oct 18, 2016 •

edited

Loading

coveralls commented Oct 18, 2016 •

edited

Loading

coveralls commented Oct 18, 2016 •

edited

Loading

coveralls commented Oct 18, 2016 •

edited

Loading

coveralls commented Oct 19, 2016 •

edited

Loading

coveralls commented Oct 19, 2016 •

edited

Loading

coveralls commented Oct 19, 2016 •

edited

Loading

coveralls commented Oct 24, 2016 •

edited

Loading