Skip to content

Conversation

@jdreaver
Copy link
Member

@jdreaver jdreaver commented Jul 2, 2014

I like the changes to SignalBlocker. That is a great, simple way to check if the signal was called.

One note about the context manager: the point of the context manager is to connect the signal(s) before some operation that fires signals begins. For example, if we call worker.run() before connecting the signals, there is a race condition; the signals might fire before the loop is executed, and we will have to use the timeout.

Also, the example doesn't look correct. The assert blocker.signal_triggered will fail because wait() is not called until the with statement exits. I would change the example in the docs to this:

def test_long_computation(qtbot):
    app = Application()

    # Watch for the app.worker.finished signal, then start the worker. 
    with qtbot.waitSignal(app.worker.finished, timeout=10000) as blocker:
        blocker.connect(app.worker.failed)  # Can add other signals to blocker
        app.worker.start()  

    # Test will wait here until either signal is emitted, or 10 seconds has elapsed

    assert blocker.signal_triggered  # Assuming the work took less than 10 seconds
    assert_application_results(app)

Other than changing the example, I think the explanation in the docs is great, and your refactoring of the tests and code looks awesome!

@jdreaver
Copy link
Member Author

jdreaver commented Jul 2, 2014

Oops, I didn't mean to open a new pull request. I thought I was hitting a "comment" button.

@nicoddemus
Copy link
Member

No worries! 😄

Updated the example, and will merge this in a few minutes!

@jdreaver
Copy link
Member Author

jdreaver commented Jul 2, 2014

Great! Nice work!

nicoddemus added a commit that referenced this pull request Jul 2, 2014
@nicoddemus nicoddemus merged commit 0feeb26 into master Jul 2, 2014
@nicoddemus nicoddemus deleted the wait-signal branch July 2, 2014 23:53
@nicoddemus
Copy link
Member

Many thanks again for the interest and full PR!

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.

3 participants