waitSignal and waitSignals() evaluate signal parameters #141

MShekow · 2016-07-14T11:04:42Z

waitSignal and waitSignals() have a callback/callbacks parameter that makes it possible to test that emitted signals have specific parameters set. waitSignals() also has a force_order parameter that allows to configure 3 different levels of strictness regarding the order in which the signals are expected. See #118

… makes it possible to test that emitted signals have specific parameters set. waitSignals() also has a force_order parameter that allows to configure 3 different levels of strictness regarding the order in which the signals are expected. #118

coveralls · 2016-07-14T11:06:53Z

Coverage remained the same at 100.0% when pulling b8507e3 on MShekow:master into f4a6bbb on pytest-dev:master.

nicoddemus · 2016-07-14T11:25:47Z

pytestqt/qtbot.py

@@ -234,7 +234,7 @@ def stopForInteraction(self):

    stop = stopForInteraction

-    def waitSignal(self, signal=None, timeout=1000, raising=None):
+    def waitSignal(self, signal=None, timeout=1000, callback=None, raising=None):


I think this parameter should appear at the end, so code doing waitSignal(signal, 1000, True) won't break.

Also I feel callback is to generic... perhaps check_params_callback would convey the intent a little better?

I don't mind this rename.

check_params_cb maybe? I think _cb is a common suffix, and check_params_callback seems so long 😉

nicoddemus · 2016-07-14T11:29:07Z

Hey @MShekow thanks a lot for the PR.

I have to leave to work so I can't review it more carefully now, I will try to make a proper review during lunch, or at the end the day the latest.

… makes it possible to test that emitted signals have specific parameters set. waitSignals() also has a force_order parameter that allows to configure 3 different levels of strictness regarding the order in which the signals are expected. #118 (this commit contains a bug fix for Python2.7 compatibility)

coveralls · 2016-07-14T11:56:46Z

Coverage remained the same at 100.0% when pulling a8cee4c on MShekow:master into f4a6bbb on pytest-dev:master.

The-Compiler · 2016-07-14T13:15:32Z

hmm, no idea why the windows builds are broken...

The-Compiler · 2016-07-14T13:16:14Z

pytestqt/qtbot.py

        if signal is not None:
            blocker.connect(signal)
        return blocker

    wait_signal = waitSignal  # pep-8 alias

-    def waitSignals(self, signals=None, timeout=1000, raising=None):
+    def waitSignals(self, signals=None, timeout=1000, force_order = "none", callbacks = None, raising=None):


nitpick: No spaces around default values

Also I think I'd rename force_order to order, and the parameters should be at the end as well.

I'm OK with renaming the parameter to order and placing it at the end. Spaces around the default value of the parameter can be removed.

MShekow · 2016-07-14T14:21:42Z

As I've never done PRs, I'd like to know whether the general procedure is this one?:
You suggest changes to the code, I add them to my fork, and once you're satisfied, you merge the PR?

The-Compiler · 2016-07-14T14:50:13Z

Right - if you push new changes to your fork, the PR will update automatically. Then we review it again, you update it if needed, etc., until it gets merged.

The-Compiler · 2016-07-14T21:56:43Z

FWIW I get the same AppVeyor issue for qutebrowser as well. I'll investigate tomorrow.

nicoddemus · 2016-07-15T02:50:10Z

pytestqt/qtbot.py

        :param int timeout:
            How many milliseconds to wait before resuming control flow.
+        :param str force_order:
+            "none": no order is enforced


Please format those as a list, like in here. The results are worth it IMO.

nicoddemus · 2016-07-15T03:25:34Z

@MShekow first let me say thank you for your extremely well written PR!

I reviewed it partially, but I'm afraid is very late and I must get some sleep now. I will continue reviewing it tomorrow.

The-Compiler · 2016-07-15T16:02:05Z

@nicoddemus can you rerun AppVeyor now that #142 is in? (Also, it'd be nice if this plugin was under the pytestbot account on AppVeyor too..)

nicoddemus · 2016-07-15T16:07:43Z

Don't we have to merge with master to get your changes into this pr anyway? This would cause a new build to be triggered anyway.

About the pytestbot user, I agree, will see how to do it.

The-Compiler · 2016-07-15T16:34:29Z

As far as I know, PRs on AppVeyor/Travis are always built against the latest master (which is also why they don't build when the PR is unmergeable)

nicoddemus · 2016-07-15T17:32:24Z

As far as I know, PRs on AppVeyor/Travis are always built against the latest master (which is also why they don't build when the PR is unmergeable)

Oh really didn't notice that, but it makes sense! 😉

Just triggered a build, let's see how it goes. I'll look later how to move ownership of the project to pytestbot.

nicoddemus · 2016-07-15T18:35:29Z

It broke only the docs environment now. 😁

MShekow · 2016-07-18T06:39:32Z

Hi. Are there any other remarks? I'll be going on vacation for the rest of July soon. Before that, I'll have time to fix all the rather "cosmetic" issues ;). There were 2 larger issues mentioned which I cannot complete before that:

Argument-capturing for SignalBlocker (all_args) and MultiSignalBlocker (recording the signals and their args in order)
More descriptive error message in SignalTimeoutError
The 2 points together are a couple of hours of work, given that I'd need to write tests for these new features, too.

* placed new parameters at the end * renamed "force_order" to "order" * renamed "callback" to "check_params_cb" and "callbacks" to "check_params_cbs" * improved various parts of the documentation * replaced assert with raising a ValueError

coveralls · 2016-07-18T09:07:50Z

Coverage decreased (-0.1%) to 99.888% when pulling 2e21419 on MShekow:master into f4a6bbb on pytest-dev:master.

nicoddemus · 2016-07-19T18:54:35Z

Hi @MShekow, sorry for the delay.

If you are going on vacation soon, I would rather we merge it sooner and work on improving the error message in 2.1. @The-Compiler what do you think?

nicoddemus · 2016-07-25T21:56:58Z

@MShekow if you could go ahead and fix the cosmetic issues it would be great.

The-Compiler · 2016-07-26T13:43:48Z

Sorry for the late answer - I was busy with Europython and a big mail-backlog after that, so I postponed everything which was a bit more work 😆

@nicoddemus I'm guessing @MShekow is in holidays by now - what cosmetic issues do you mean? I can see "docstring of pytestqt.qtbot.QtBot.waitSignal:36: WARNING: Inline emphasis start-string without end-string." on Appveyor, but it seems they fixed the other issues in 2e21419.

The PR looks good otherwise (though I don't really understand the order handling and tests 100% yet 😆).

I'm wondering if we should make order="strict" the default though, as we're already breaking stuff with v2.0 anyways.

nicoddemus · 2016-07-28T12:01:07Z

I will try to merge this tonight together with #130 and make the 2.0 release then.

The-Compiler · 2016-07-28T12:24:42Z

@nicoddemus What do you think about my comment regarding the default ordering above?

nicoddemus · 2016-07-28T12:25:54Z

I'm wondering if we should make order="strict" the default though, as we're already breaking stuff with v2.0 anyways.

Oh forgot to comment: I agree with using strict as default, but this is a new feature so we won't be breaking anything, right?

The-Compiler · 2016-07-28T12:27:38Z

Nope - MultiSignalBlocker already existed before and didn't care in which order the signals were emitted. If we turn on the order checking by default now, that is a breaking change.

nicoddemus · 2016-07-28T12:38:03Z

Oh right, sorry.

Hmm not sure TBH, it seems strict ordering only makes sense for signals coming from the same object. For multiple signals from different objects like the example code in the docs it doesn't make sense and is even error prone.

The-Compiler · 2016-07-28T14:11:48Z

Hmm, true. I reviewed my usage of waitSignals and it has many instances where the order is fixed but I don't actually care (e.g. a Question object in a prompt emitting answered, completed and answered_no), and some instances where I care (e.g. [reply.metaDataChanged, reply.readyRead, reply.finished]). Let's keep it.

edit: So what you said above, essentially 😆

nicoddemus · 2016-07-28T22:03:39Z

Merged. Thanks @MShekow again for the PR!

The-Compiler · 2016-07-29T05:22:14Z

So... I'm a bit late, but I just realised check_params_cb and order aren't mentioned in the signal docs at all, and order isn't even in the changelog.

nicoddemus · 2016-07-29T11:07:02Z

Thanks, completely missed that. Good thing the documentation is live, so it can be fixed on master. Created #147 for it. 😁

nicoddemus reviewed Jul 14, 2016
View reviewed changes

The-Compiler reviewed Jul 14, 2016
View reviewed changes

nicoddemus reviewed Jul 15, 2016
View reviewed changes

The-Compiler mentioned this pull request Jul 15, 2016

appveyor: Fix registry fixing #142

Merged

Fixed various smaller issues:

2e21419

* placed new parameters at the end * renamed "force_order" to "order" * renamed "callback" to "check_params_cb" and "callbacks" to "check_params_cbs" * improved various parts of the documentation * replaced assert with raising a ValueError

nicoddemus mentioned this pull request Jul 21, 2016

Configure Qt API using config file #130

Merged

nicoddemus merged commit bad3a3b into pytest-dev:master Jul 28, 2016

nicoddemus added a commit that referenced this pull request Jul 28, 2016

Add CHANGELOG for #141

f06cf7d

MShekow mentioned this pull request Aug 3, 2016

Improve waitSignal/waitSignals TimeoutError messages and catch arguments #151

Closed

MShekow mentioned this pull request Aug 12, 2016

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

Merged

waitSignal and waitSignals() evaluate signal parameters #141

waitSignal and waitSignals() evaluate signal parameters #141

Conversation

MShekow commented Jul 14, 2016

coveralls commented Jul 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicoddemus commented Jul 14, 2016

coveralls commented Jul 14, 2016 • edited Loading

The-Compiler commented Jul 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MShekow commented Jul 14, 2016

The-Compiler commented Jul 14, 2016

The-Compiler commented Jul 14, 2016

Choose a reason for hiding this comment

nicoddemus commented Jul 15, 2016

The-Compiler commented Jul 15, 2016

nicoddemus commented Jul 15, 2016

The-Compiler commented Jul 15, 2016

nicoddemus commented Jul 15, 2016

nicoddemus commented Jul 15, 2016

MShekow commented Jul 18, 2016

coveralls commented Jul 18, 2016 • edited Loading

nicoddemus commented Jul 19, 2016

nicoddemus commented Jul 25, 2016

The-Compiler commented Jul 26, 2016 • edited Loading

nicoddemus commented Jul 28, 2016

The-Compiler commented Jul 28, 2016

nicoddemus commented Jul 28, 2016

The-Compiler commented Jul 28, 2016

nicoddemus commented Jul 28, 2016

The-Compiler commented Jul 28, 2016 • edited Loading

nicoddemus commented Jul 28, 2016

The-Compiler commented Jul 29, 2016

nicoddemus commented Jul 29, 2016

coveralls commented Jul 14, 2016 •

edited

Loading

coveralls commented Jul 14, 2016 •

edited

Loading

coveralls commented Jul 18, 2016 •

edited

Loading

The-Compiler commented Jul 26, 2016 •

edited

Loading

The-Compiler commented Jul 28, 2016 •

edited

Loading