Skip to content

Disconnect SignalBlocker after exiting loop. #71

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 5 commits into from
Aug 7, 2015
Merged

Disconnect SignalBlocker after exiting loop. #71

merged 5 commits into from
Aug 7, 2015

Conversation

The-Compiler
Copy link
Member

Otherwise, a second signal emission will still call the functions of the
already garbage-collected SignalBlocker with PyQt5 < 5.3.

The signals could already be disconnected, so the TypeError (older PyQt
versions) or RuntimeError (newer PyQt versions) which is raised in that case is
ignored.

Fixes #69.

Otherwise, a second signal emission will still call the functions of the
already garbage-collected SignalBlocker with PyQt5 < 5.3.

The signals could already be disconnected, so the TypeError (older PyQt
versions) or RuntimeError (newer PyQt versions) which is raised in that case is
ignored.

Fixes #69.
self._loop.exec_()
if not self.signal_triggered and self.raising:
raise SignalTimeoutError("Didn't get signal after %sms." %
self.timeout)

def _quit_loop_by_timeout(self):
self._loop.quit()
self._cleanup()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't cleanup always be called? If so, I think you should call it as soon after self._loop.exec_() finishes (in line 377).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True - I first only had it in _quit_loop_by_signal and then noticed it's also needed on timeouts, that's why I didn't think of putting it there.

@nicoddemus
Copy link
Member

cool

@The-Compiler, could you also add a new CHANGELOG entry, under 1.5.2.dev? (I just started using it instead of the GitHub release page because it makes it easier to add entries together with PRs).

@The-Compiler
Copy link
Member Author

Gah... the newest commit (a4ca81a) breaks it again:

=================================================== ERRORS ===================================================
_____________________________ ERROR at teardown of test_wait_twice[False-False] ______________________________

self = <_pytest.runner.SetupState object at 0x7f2f7dc3ee10>
colitem = <Function 'test_wait_twice[False-False]'>

    def _callfinalizers(self, colitem):
        finalizers = self._finalizers.pop(colitem, None)
        exc = None
        while finalizers:
            fin = finalizers.pop()
            try:
>               fin()

.venv/lib/python3.4/site-packages/_pytest/runner.py:356: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.venv/lib/python3.4/site-packages/_pytest/python.py:1868: in finish
    func()
.venv/lib/python3.4/site-packages/_pytest/python.py:1826: in teardown
    next()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

qapp = <PyQt5.QtWidgets.QApplication object at 0x7f2f73384168>
request = <SubRequest 'qtbot' for <Function 'test_wait_twice[False-False]'>>

    @pytest.yield_fixture
    def qtbot(qapp, request):
        """
        Fixture used to create a QtBot instance for using during testing.

        Make sure to call addWidget for each top-level widget you create to ensure
        that they are properly closed after the test ends.
        """
        result = QtBot()
        no_capture = _exception_capture_disabled(request.node)
        if no_capture:
            yield result  # pragma: no cover
        else:
            with capture_exceptions() as exceptions:
                yield result
            if exceptions:
>               pytest.fail(format_captured_exceptions(exceptions))
E               Failed: Qt exceptions in virtual methods:
E               ________________________________________________________________________________
E                 File "/home/buildbotx/pytest-qt/pytestqt/plugin.py", line 446, in _quit_loop_by_signal
E                   self.signal_triggered = True
E               
E               AttributeError: 'NoneType' object has no attribute 'signal_triggered'
E               ________________________________________________________________________________

pytestqt/plugin.py:582: Failed
-------------------------------------------- Captured stderr call -------------------------------------------$
Traceback (most recent call last):
  File "/home/buildbotx/pytest-qt/pytestqt/plugin.py", line 446, in _quit_loop_by_signal
    self.signal_triggered = True
AttributeError: 'NoneType' object has no attribute 'signal_triggered'
===================================== 51 passed, 1 error in 6.03 seconds ====================================$

I'll investigate what's happening later.

@The-Compiler
Copy link
Member Author

Okay... I have no idea what difference the commit would make... Do you?

@nicoddemus
Copy link
Member

Hmmm if I have to guess as I can't debug this right now, I think during _cleanup it should disconnect also all signals that were connected (in the _signals attribute).

It's really a disappointment that signal disconnection is not handled automatically... I believe it was handled automatically in other PyQt versions (at least PyQt4).

Btw sorry if I don't answer all issues/PRs Today because I'm kinda busy, but I will probably be able to work on them tomorrow. 😄

@The-Compiler
Copy link
Member Author

Hmmm if I have to guess as I can't debug this right now, I think during _cleanup it should disconnect also all signals that were connected (in the _signals attribute).

The issue does (for a reason I don't know) not happen with MultiSignalBlocker in the first place, so I didn't want to needlessly complicate that. However, the SingleSignalBlocker test fails after changing where _cleanup is called... Are you okay with just reverting a4ca81a?

It's really a disappointment that signal disconnection is not handled automatically... I believe it was handled automatically in other PyQt versions (at least PyQt4).

Yeah, as said it's also handled properly in PyQt5 5.3 and newer, this is really only an issue with ancient PyQt versions (but Ubuntu Trusty, which is still supported until 2019, comes with 5.2).

Btw sorry if I don't answer all issues/PRs Today because I'm kinda busy, but I will probably be able to work on them tomorrow.

No worries - take your time! I just did a brain-dump of everything still in my mind 😉

@nicoddemus
Copy link
Member

Are you okay with just reverting a4ca81a?

Yes, we can improve that later.

I just did a brain-dump of everything still in my mind 😉

More than welcome to do that! 😄

This reverts commit a4ca81a.

For some reason this makes the test fail on Trusty again, and nobody seems to
understand why - so let's just keep the old way.
@The-Compiler
Copy link
Member Author

I reverted the commit and made sure the test passes on Trusty (after struggling with the VM a bit). This is good to merge from my POV now.

nicoddemus added a commit that referenced this pull request Aug 7, 2015
Disconnect SignalBlocker after exiting loop.
@nicoddemus nicoddemus merged commit e9246a9 into pytest-dev:master Aug 7, 2015
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