Skip to content

Wait signal #14

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

Merged
merged 6 commits into from
Jul 2, 2014
Merged

Wait signal #14

merged 6 commits into from
Jul 2, 2014

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.

2 participants