Support for capturing signals and args and improved SignalTimeoutError messages #153

MShekow · 2016-08-12T08:30:57Z

waitSignal(): added support for capturing arguments when callback was not satisfied (into blocker.all_args) and improved the message of SignalTimeoutError to include the name of the signal, non-matching signal parameters and name of the callback.

waitSignals(): Added capturing signals and their arguments (see all_signals_and_args). Cleaned up large parts of the code of MultiSignalBlocker for better readability. Improved the message of SignalTimeoutError. Updated documentation. Added tests. See #151

… (into blocker.all_args) and improved the message of SignalTimeoutError to include the name of the signal, non-matching signal parameters and name of the callback. Next step is to include similar functionality for MultiSignalBlocker.

…ee all_signals_and_args). Cleaned up large parts of the code of MultiSignalBlocker for better readability. Updated documentation. Added tests. #151

coveralls · 2016-08-12T08:33:23Z

Coverage decreased (-2.6%) to 97.354% when pulling 77b1cb0 on MShekow:master into 598c1c2 on pytest-dev:master.

The-Compiler · 2016-08-12T08:35:02Z

pytestqt/qtbot.py

        :param Signal signal:
-            A signal to wait for. Set to ``None`` to just use timeout.
+            A signal to wait for, or a tuple (signal, signal_name_as_str) to improve the error message that is part


The tuple should be monospaced as well for consistency

The-Compiler · 2016-08-12T08:49:30Z

Thanks for your great work on this, looking forward to it!

I added some style comments, and plan to take another closer look when I feel more awake 😆

Some other things to note:

The coverage should stay at 100% - can you investigate what's not covered yet?
The tests seem to fail with Python 2 and PyQt4/PySide - can you investigate?

MShekow · 2016-08-12T09:41:32Z

Hi. I'm having trouble fixing the bugs. The most problematic issue is unicode strings. Python 2 and 3 seem to work very different (I don't personally care for legacy software, so I never use Python 2). Python 2 breaks my tests because it converts str( ('somestring',) ) to "(u'somestring',)" (i.e. it adds the u which I don't want there.
What I can do is to have a if type(somestring) is unicode check when iterating over the tuple's elements (and convert them with str() in this case), but that only works in Python 2 (Python 3 doesn't recognize unicode). So, what do I do now? Do I really have to do Python version checking in my code to fix this?

The-Compiler · 2016-08-12T10:08:16Z

Why don't you want the u'...' there? If a parameter is an unicode string, I'd surely want to see that fact, just like I want to have ' around strings. I'd suggest just adjusting the test accordingly (with a version check probably).

MShekow · 2016-08-12T10:09:59Z

Well, I didn't want the u there due to the tests. I'll follow your suggestion and adapt the tests instead.

coveralls · 2016-08-12T13:21:13Z

Coverage decreased (-2.5%) to 97.531% when pulling 78ecf73 on MShekow:master into 598c1c2 on pytest-dev:master.

…nts (that we emit in test code as normal strings) are converted to "PyQt4.QtCore.QString(u'<stringcontent>')" by repr(), while as for all other Python 2 builds, this is just "u'<stringcontent>'".

coveralls · 2016-08-12T13:36:40Z

Coverage decreased (-0.7%) to 99.295% when pulling 061c11d on MShekow:master into 598c1c2 on pytest-dev:master.

MShekow · 2016-08-12T13:49:48Z

So, I've fixed all those things you mentioned and got the tests working again.
Regarding the coverage: I can add tests for those lines the tests do not currently cover. These are (see here for more details: https://coveralls.io/builds/7422331/source?filename=pytestqt%2Fwait_signal.py ):

waitSignals() code that raises TypeError or ValueError when user doesn't provide valid parameters
Line 120: no name for a provided callback could be determined. I'm not sure how to cover that. Maybe callable wrapped inside a functools.partial wrapped inside a functools partial?

MShekow · 2016-08-12T20:10:16Z

docs/signals.rst

+Getting emitted signals and arguments
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+.. versionadded:: 2.1


Is this guess for the next version actually correct?

…prove coverage.

coveralls · 2016-08-15T18:02:37Z

Coverage decreased (-0.09%) to 99.912% when pulling 6e18d47 on MShekow:master into 598c1c2 on pytest-dev:master.

…ged in 3.5

coveralls · 2016-08-15T18:23:22Z

Coverage decreased (-0.09%) to 99.912% when pulling 875f941 on MShekow:master into 598c1c2 on pytest-dev:master.

MShekow · 2016-08-15T18:25:38Z

I've done the necessary changes. I don't know how to further increase the coverage, tbh. Please note that coveralls seems to be buggy and the comments aren't always accurate. The actual reduction is by 0.09%. I understand I could get it back to 0% if I had the overriding get_timeout_error_message() functions call their empty super-implementation, but I don't see any good in doing so.

The-Compiler · 2016-08-15T18:54:44Z

pytestqt/wait_signal.py

        if self._timer is not None:
            _silent_disconnect(self._timer.timeout, self._quit_loop_by_timeout)
            self._timer.stop()
            self._timer = None

+    def get_timeout_error_message(self):
+        pass


Since this must be overridden in subclasses, you should do raise NotImplementedError here (docs) - that should also mean no coverage is required for it, and if coverage.py doesn't do so automatically, we can tell it to.

I've changed it in the code, but coveralls still complains about that line not being covered. So, should I use a pragma? I don't see any configuration files in place yet.

…hod.

coveralls · 2016-08-15T20:01:05Z

Coverage decreased (-0.09%) to 99.912% when pulling 2316261 on MShekow:master into 598c1c2 on pytest-dev:master.

nicoddemus · 2016-08-15T21:31:38Z

I've done the necessary changes. I don't know how to further increase the coverage, tbh.

For this case specifically I would say it is fair to add a # pragma: no cover on that line. 👍

nicoddemus · 2016-08-15T21:32:07Z

docs/signals.rst

+Getting all arguments of non-matching arguments
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+.. versionadded:: 2.0


This should be 2.1?

nicoddemus · 2016-08-15T21:58:32Z

Hey @MShekow, excellent documentation on the tests and code!

Other than adding the comment to skip the missing coverage, my suggestions are just that, suggestions, so feel free to not implement them if you don't really have the time as I'm guessing you have spent quite a few hours on this already.

All your work here is much appreciated!

The-Compiler · 2016-08-16T06:58:59Z

pytestqt/wait_signal.py

+            signal_name = signal_tuple[1]
+            if not isinstance(signal_name, str) or not signal_name:
+                raise TypeError("Invalid type for user-provided signal name, "
+                                "expected str but got {}".format(type(signal_name)))


If someone accidentally passed an empty string, they'd get "Invalid type for user-provided signal name, expected str but got str", which might be quite confusing.

You're right. I'll fix this, i.e. split it into two ifs (each one without any or).

…ma no cover.

coveralls · 2016-08-16T16:31:11Z

Coverage decreased (-1.7%) to 98.326% when pulling 56771f0 on MShekow:master into 598c1c2 on pytest-dev:master.

coveralls · 2016-08-16T16:31:11Z

Coverage decreased (-1.7%) to 98.326% when pulling 56771f0 on MShekow:master into 598c1c2 on pytest-dev:master.

coveralls · 2016-08-16T16:31:11Z

Coverage decreased (-1.7%) to 98.326% when pulling 56771f0 on MShekow:master into 598c1c2 on pytest-dev:master.

coveralls · 2016-08-16T16:31:11Z

Coverage decreased (-1.7%) to 98.326% when pulling 56771f0 on MShekow:master into 598c1c2 on pytest-dev:master.

coveralls · 2016-08-16T16:31:11Z

Coverage remained the same at 100.0% when pulling 56771f0 on MShekow:master into 598c1c2 on pytest-dev:master.

nicoddemus · 2016-09-16T00:07:34Z

Hey @MShekow,

Really sorry about the delay on this. Just now I found the time to review it and it looks great! Your entire PR is very well written and your test descriptions are really helpful! 👍

Long overdue, I'm merging this! I plan to have 2.1 out soon real soon. 😁

Also, thanks @The-Compiler for the through review as well. 😉

MShekow · 2016-09-16T04:47:53Z

My pleasure! Thanks for merging :)

NameZero912 added 3 commits August 4, 2016 19:44

Merge branch 'master' of https://github.com/pytest-dev/pytest-qt

23de0d1

Added capturing signals and their arguments for MultiSignalBlocker (s…

77b1cb0

…ee all_signals_and_args). Cleaned up large parts of the code of MultiSignalBlocker for better readability. Updated documentation. Added tests. #151

The-Compiler reviewed Aug 12, 2016
View reviewed changes

Fixed several small issues.

78ecf73

Try another fix. Apparently under Python 2.7 when using PyQt4, argume…

061c11d

…nts (that we emit in test code as normal strings) are converted to "PyQt4.QtCore.QString(u'<stringcontent>')" by repr(), while as for all other Python 2 builds, this is just "u'<stringcontent>'".

MShekow reviewed Aug 12, 2016
View reviewed changes

Add some more tests regarding invalid user-provided parameters, to im…

533e36b

…prove coverage.

NameZero912 added 3 commits August 15, 2016 19:46

Improve formatting

9d64b1e

Add 3 more tests to further improve coverage.

2f20e6b

Fixed an oversight.

6e18d47

Disable a test for Python 3.5 because functools.partial behavior chan…

875f941

…ged in 3.5

The-Compiler reviewed Aug 15, 2016
View reviewed changes

Added NotImplementedError to base get_timeout_error_message(self) met…

2316261

…hod.

nicoddemus reviewed Aug 15, 2016
View reviewed changes

The-Compiler reviewed Aug 16, 2016
View reviewed changes

Fixed minor issues. Attempt to get coverage back to 100% using # prag…

56771f0

…ma no cover.

nicoddemus merged commit dfbd99d into pytest-dev:master Sep 16, 2016

nicoddemus mentioned this pull request Sep 16, 2016

Improve waitSignal/waitSignals TimeoutError messages and catch arguments #151

Closed

nicoddemus added a commit to nicoddemus/pytest-qt that referenced this pull request Sep 16, 2016

Add CHANGELOG entry for pytest-dev#153

7289785

nicoddemus added a commit to nicoddemus/pytest-qt that referenced this pull request Oct 18, 2016

Add CHANGELOG entry for pytest-dev#153

bc211c2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for capturing signals and args and improved SignalTimeoutError messages #153

Support for capturing signals and args and improved SignalTimeoutError messages #153

MShekow commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

The-Compiler Aug 12, 2016

The-Compiler commented Aug 12, 2016

MShekow commented Aug 12, 2016

The-Compiler commented Aug 12, 2016

MShekow commented Aug 12, 2016

coveralls commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

MShekow commented Aug 12, 2016

MShekow Aug 12, 2016

nicoddemus Aug 15, 2016

coveralls commented Aug 15, 2016 •

edited

Loading

coveralls commented Aug 15, 2016 •

edited

Loading

MShekow commented Aug 15, 2016 •

edited

Loading

The-Compiler Aug 15, 2016

MShekow Aug 15, 2016

coveralls commented Aug 15, 2016 •

edited

Loading

nicoddemus commented Aug 15, 2016

nicoddemus Aug 15, 2016

nicoddemus commented Aug 15, 2016

The-Compiler Aug 16, 2016

MShekow Aug 16, 2016

coveralls commented Aug 16, 2016

coveralls commented Aug 16, 2016

coveralls commented Aug 16, 2016

coveralls commented Aug 16, 2016

coveralls commented Aug 16, 2016 •

edited

Loading

nicoddemus commented Sep 16, 2016

MShekow commented Sep 16, 2016

Support for capturing signals and args and improved SignalTimeoutError messages #153

Support for capturing signals and args and improved SignalTimeoutError messages #153

Conversation

MShekow commented Aug 12, 2016 • edited Loading

coveralls commented Aug 12, 2016 • edited Loading

Choose a reason for hiding this comment

The-Compiler commented Aug 12, 2016

MShekow commented Aug 12, 2016

The-Compiler commented Aug 12, 2016

MShekow commented Aug 12, 2016

coveralls commented Aug 12, 2016 • edited Loading

coveralls commented Aug 12, 2016 • edited Loading

MShekow commented Aug 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 15, 2016 • edited Loading

coveralls commented Aug 15, 2016 • edited Loading

MShekow commented Aug 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 15, 2016 • edited Loading

nicoddemus commented Aug 15, 2016

Choose a reason for hiding this comment

nicoddemus commented Aug 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 16, 2016

coveralls commented Aug 16, 2016

coveralls commented Aug 16, 2016

coveralls commented Aug 16, 2016

coveralls commented Aug 16, 2016 • edited Loading

nicoddemus commented Sep 16, 2016

MShekow commented Sep 16, 2016

MShekow commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 12, 2016 •

edited

Loading

coveralls commented Aug 15, 2016 •

edited

Loading

coveralls commented Aug 15, 2016 •

edited

Loading

MShekow commented Aug 15, 2016 •

edited

Loading

coveralls commented Aug 15, 2016 •

edited

Loading

coveralls commented Aug 16, 2016 •

edited

Loading