Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added signal blocker, as discussed in #12 #13

Merged
merged 1 commit into from
Jul 2, 2014
Merged

Added signal blocker, as discussed in #12 #13

merged 1 commit into from
Jul 2, 2014

Conversation

jdreaver
Copy link
Member

@jdreaver jdreaver commented Jul 1, 2014

Here it is! Tell me if the API is wonky, or anything else you see that needs improvement.

The only reason I yield the event loop is to facilitate the test.

Also, should we add Python 3.3 and 3.4 to Travis?

@nicoddemus
Copy link
Member

Hi John,

Many thanks, this looks great! 😄

I was just wondering if perhaps waitSignal or waitSignals (or even waitForSignal/s) would be a better name? What you think?

About 3.3 and 3.4 in Travis, the problem is that we install PyQt and PySide using apt-get which only have packages for 2.7 and 3.2 at the moment, otherwise the Travis job takes too long because it tries to compile everything and times out.

I will merge this tomorrow and make a new release, because today unfortunately I'm kinda busy.

I really appreciate your effort, thanks again! 😄

Cheers,

@jdreaver
Copy link
Member Author

jdreaver commented Jul 1, 2014

Thanks!

I agree with your suggestion to use waitSignal. I just don't know which method to name what. How about this:

  • Call the context manager version waitSignalSetup, since the event loop doesn't start until the code in the context manager is executed. I kind of don't like this name, but I can't think of anything else at the moment.
with qtbot.waitSignalSetup(signal, timeout=1000):
    long_operation_that_calls_signal()
  • Call the function version waitSignal.
long_operation_that_calls_signal()
qtbot.waitSignal(signal, timeout=1000)

I think I will also write an example for the docs today or tomorrow.

@nicoddemus
Copy link
Member

Hmm, how about we take the approach that pytest.raises and UnitTest.assertRaises use, which can work as a function or context manager depending on the parameters passed? Following this approach, the following code blocks are equivalent:

with qtbot.waitSignal(signal, timeout):
    long_operation_that_calls_signal(*args)

Or

qtbot.waitSignal(signal, timeout, long_operation_that_calls_signal, *args)

The only downside I see is that you must specify a timeout using the second form... what do you think?

About updating the docs, I was planning on doing that tomorrow, but if you want to contribute with that as well, fantastic! 😄

Cheers.

@jdreaver
Copy link
Member Author

jdreaver commented Jul 1, 2014

Attaching behavior to the API limits the use of the function version. Sometimes, signals are not generated as a side effect of a single, long-running function. For example, I have signals that are periodically emitted as new data is generated. They are not emitted as part of a single function call, so the second API you suggested would keep me from catching the signal.

I can create an object that can act as both a context manager and a return value (in object form). The first way would work like we have discussed:

with waitSignal(signal, timeout=1000):
    some()
    setup()
    functions()
    assert some_assertion()

The second way would be this:

blocker = waitSignal(signal, timeout=1000)
some()
setup()
functions()
assert some_assertion()
blocker.wait()

Or, if you just want to block without any setup, you can call it two ways:

blocker = waitSignal(signal, timeout=1000)
blocker.wait()
waitSignal(signal, timeout=1000).wait()

Implementation

Here is how it can be implemented.

class SignalBlocker:

    def __init__(self, timeout=None):
        self.loop = QtCore.QEventLoop()
        self.timeout = timeout

    def wait(self):
        if self.timeout is not None:
            QtCore.QTimer.singleShot(self.timeout, self.loop.quit)
        self.loop.exec_()

    def connect(self, signal):
        signal.connect(self.loop.quit)

    def __enter__(self):
        return self.loop  # In case user wants to bind event loop

    def __exit__(self, type, value, traceback):
        # If used in a with statement, call wait().
        self.wait()

def waitSignal(signals, timeout=10000):
    blocker = SignalBlocker(timeout=timeout)
    if not isinstance(signals, list):
        signals = [signals]
    for signal in signals:
        blocker.connect(signal)
    return blocker

In fact, if we exposed SignalBlocker as part of the public API, then we can use it like this:

blocker = SignalBlocker(timeout=1000)
blocker.connect(signal1)
blocker.connect(signal2)
blocker.timeout = 2000  # Can change timeout before wait() called
blocker.wait()

Maybe we can make a constructor method in qtbot, so we can call blocker = qtbot.signalBlocker(). I personally like this approach best, as it gives the user maximum flexibility.

@jdreaver
Copy link
Member Author

jdreaver commented Jul 2, 2014

Actually, my suggestion of using qtbot.signalBlocker() is redundant. We can just call qtbot.waitSignal with no arguments to get the same behavior.

The following three calls are equivalent:

with qtbot.waitSignal(signal, timeout=1000):
    some_function()
    other_function()
blocker = qtbot.waitSignal(signal, timeout=1000)
some_function()
other_function()
blocker.wait()
blocker = qtbot.waitSignal()
blocker.connect(signal)
blocker.timeout = 1000
some_function()
other_function()
blocker.wait()

This seems to be the cleanest option API-wise, to me at least.

@jdreaver
Copy link
Member Author

jdreaver commented Jul 2, 2014

I've updated the pull request. It's a lot simpler now. qtbot.wait() takes just one signal, as other signals can be added. I added two examples in the docstring. Of course, it would be nice to have examples/rationale in the main docs.

@nicoddemus
Copy link
Member

Hi John,

Excellent work, thanks!

After reviewing the code, I realized it might be interesting to know if a waitSignal finished because of an actual signal or time-out, so I added a signal_triggered attribute to SignalBlocker. Also, I updated the documentation as you suggested and tweaked the tests a bit.

I've pushed the changes to a new branch:
https://github.com/nicoddemus/pytest-qt/compare/wait-signal?expand=1

Let me know what you think!

Cheers,

@nicoddemus nicoddemus merged commit b63f7f0 into pytest-dev:master Jul 2, 2014
nicoddemus added a commit that referenced this pull request Jul 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants