Add support for qt5reactor

[altendky]

No description provided.

[vtitor]

@altendky Thanks for this. Could you give me an example of tests which use this feature, please? I guess we can do it in another way.

[altendky]

https://gist.github.com/42401c3ec3fa101a90b4b2faeb26e183

That's obviously contrived but I think it covers a starting point. It would be expected to run with pytest, pytest-qt, pytest-twisted, and qt5reactor installed. qt5reactor lets the PyQt5/Qt5 and Twisted event loops interoperate (IIRC the Qt event loop is primary).

The bigger picture is that I write a PyQt5/Twisted service tool for configuring and running some embedded hardware over a CANbus connection (EPyQ). I want to be able to write a pytest suite using my application to control and test the embedded device on a Hardware in the Loop test system (HiL).

Thanks for your consideration and any suggestions you have. This is my first foray into pytest plugins so I've certainly got a lot to learn.

[nicoddemus]

Hi guys, pytest-qt author here. 😁

IIUC, qt5reactor.install() must be called after pytest-qt's qapp fixture correct? Then I think the correct way is to inject a new fixture into the global namespace which depends on qapp at this point, something like this:

if config.getoption('qt5reactor'):
    
    class Qt5ReactorPlugin(object):
        
        @pytest.fixture(scope='session', autouse=True)
        def qt5reactor(qapp):
            import qt5reactor
            qt5reactor.install()

    config.pluginmanager.register(Qt5ReactorPlugin())   

This is the recommended way of creating new fixtures based on command-line options, and will ensure the QApplication object is created before qt5reactor.install is called.

But now that I think about it, perhaps pytest-twisted authors would also like to create a generic reactor fixture which is selected from a --reactor command-line option? Full disclosure, I've never used twisted and have only a vague idea of what a "reactor" is, so excuse me if that last thought didn't make much sense.

[altendky]

I guess my basic hangup was the different structures of the two plugins which I think at least in 'normal' usage stem from the different structures of Qt vs. Twisted. That being that in Qt you construct the singleton explicitly and can request the existing instance independently while with Twisted you just import both to create and to get access to it elsewhere (from the little Twisted I've seen).

https://github.com/twisted/twisted/blob/trunk/src/twisted/internet/reactor.py
https://github.com/twisted/twisted/blob/trunk/src/twisted/internet/default.py

Also what was pointed out about pytest-qt having a fixture create the singleton vs. pytest-twisted creating it very early in the plugin loading process. I'm not sure how much this part is up for discussion.

[altendky]

Not working either for my tests or the test suite but figured I'd share my attempt to add a fixture per your recommendation @nicoddemus. Somehow my check that there is an application instance finds one but then the QWidget construction complains that there isn't a QApplication...

https://gist.github.com/altendky/71264320527684193a9d62e0bb903fb8

[nicoddemus]

Ahh! I believe you need to add self to default_reactor and qt5_reactor because you are making them a method of ReactorPlugin:

def qt5_reactor(qapp):

The qapp parameter is being given self (which is an instance of ReactorPlugin);

[altendky]

@nicoddemus thanks... *sigh* I should have paid more attention when I was adding that _ parameter to the default reactor fixture. Anyways, the class is just a namespace so may as well staticmethod it instead. Still not working for me but fixed that weird QApplication issue at least. Time to dig some more.

[altendky]

So one issue with this I think, even if I get it working for my use case, is the test which tries to use the reactor in a pytest_configure hook.

pytest-twisted/testing/test_basic.py

Lines 108 to 128 in 28df3af

	def test_blocon_in_hook(testdir):
	testdir.makeconftest("""
	import pytest
	from twisted.internet import reactor, defer
	def pytest_configure(config):
	d = defer.Deferred()
	reactor.callLater(0.01, d.callback, 1)
	pytest.blockon(d)
	""")
	testdir.makepyfile("""
	from twisted.internet import reactor, defer
	def test_succeed():
	d = defer.Deferred()
	reactor.callLater(0.01, d.callback, 1)
	return d
	""")
	rr = testdir.run(sys.executable, "-m", "pytest", "-v")
	outcomes = rr.parseoutcomes()
	assert outcomes.get("passed") == 1

I guess with the master implementation this makes sure that other plugins can set up background stuff in the reactor at configure time. pytest-twisted would have to use @pytest.hookimpl(tryfirst=True) to be able to add a --qt5reactor option or such, but tryfirst doesn't feel great to me, though I'm not sure what particular scenario it would be a problem in. I tihnk this test does rule out the autouse fixture approach though. @vtitor do you know how important this test use case is?

[vtitor]

I'm back here :)
So, about pytest_addhooks, this was implemented in #12.
And I guess it's not critical (to use greenlet as early as possible - just call create_twisted_greenlet):

 def test_blocon_in_hook(testdir): 
     testdir.makeconftest(""" 
         import pytest 
         from pytest_twisted import create_twisted_greenlet
         from twisted.internet import reactor, defer 
  
         def pytest_configure(config): 
             create_twisted_greenlet()
             d = defer.Deferred() 
             reactor.callLater(0.01, d.callback, 1) 
             pytest.blockon(d) 
     """) 
     testdir.makepyfile(""" 
         from twisted.internet import reactor, defer 
  
         def test_succeed(): 
             d = defer.Deferred() 
             reactor.callLater(0.01, d.callback, 1) 
             return d 
     """) 
     rr = testdir.run(sys.executable, "-m", "pytest", "-v") 
     outcomes = rr.parseoutcomes() 
     assert outcomes.get("passed") == 1 

[altendky]

I thought I had switched back and just merged master in... :[ Time for some cleanup.

[altendky]

Just making sure to point out that this is presently skipped (yes, it fails...).

[altendky]

@vtitor It seems like it's not just a matter of calling something at that point but rather that this change to a fixture-created reactor means that any reactor created before the first fixture would get deleted/shutdown/... when the fixture eventually creates the configured reactor.

[vtitor]

@altendky IIUC, you are right in the case with qt5 reactor. But for default reactor the test should be passed.

[altendky]

@vtitor are you suggesting creating the default reactor regardless and then replacing it with the qt5 reactor if specified? And access in the hook will only work right if the qt5 reactor isn't specified?

To help me understand, what's the use case for needing the reactor there?

[vtitor]

are you suggesting creating the default reactor regardless and then replacing it with the qt5 reactor if specified? And access in the hook will only work right if the qt5 reactor isn't specified?

@altendky Nope. I have created this altendky#1 pr to clarify what I mean.

The main idea is having the ability to use pytest twisted stuff not only in tests. For example, the blockon function in pytest hooks is really often necessary when you write integration tests for a huge twisted project.
So, in this case, you can just call init_reactor in the place where you need (for example in some custom plugin than prepare the specific server state depending on cmd option):

import pytest_twisted

def pytest_configure(config):
    pytest_twisted.init_reactor()
    if config.getoption('customoption'):
        # do smth using blockon 

and maybe smth like this for qt (not familiar with qt):

import pytest_twisted

def pytest_configure(config):
    pytest_twisted.init_qt5_reactor(QApplication([])) 

[altendky]

@vtitor Thanks. So my summary would be that the auto-use fixtures remain but users can call them early if they want. Do I understand correctly? I would probably want to tweak the fixtures to fail if there already is a reactor of the wrong type rather than attempting the del sys.modules['twisted.internet.reactor'] I have in the qt reactor fixture. Does that seem good? Otherwise early-created reactors might unexpectedly get killed.

I also have been thinking that to make this a bit more general perhaps --qt5reactor should instead be --reactor=qt5reactor. Just seems like a more explicit way to do exclusive choices for when someone wants qtreactor (qt4) or whatever other reactor option. I assume there are more.

[vtitor]

So my summary would be that the auto-use fixtures remain but users can call them early if they want. Do I understand correctly?

@altendky Yes. And I think both your ideas are great.

[altendky]

This seems super hacky but I wasn't sure how else to get the type that the default reactor would be without constructing it (or changing this part of twisted, which I think would be good).

[altendky]

Perhaps I'll come up with something more but for now my only remaining concern is the hackiness mentioned above. Otherwise, I think this is ready.

Here's a bit of a summary for future reference. Probably belongs somewhere else, but copy/paste isn't that hard.

What it takes to add a new reactor:

• In pytest_twisted.py
  • Write an init_foo_reactor() function
  • Add 'foo': init_foo_reactor, to _reactor_fixtures where the key will be the string to be passed such as --reactor=foo.
• In testing/test_basic.py
  • Add test_blockon_in_hook_with_foo() decorated by @skip_if_reactor_not('foo')
  • Add test_wrong_reactor_with_foo() decorated by @skip_if_reactor_not('foo')
• In tox.ini
  • Adjust envlist to include the fooreactor factor for the appropriate versions of Python
  • Add conditional deps for the new reactor such as foo: foobar to the appropriate test environments
  • Add the conditional assignment foo: reactor_option=foo to setenv in the appropriate test environments
• In .travis.yml
  • Consider any extra system packages which may be required

It would be nice to maybe parameterize the tests somehow to avoid some repetition. At least for the non-default reactors. It would also be nice to adjust the Tox test environment setup to have a single entry with complex factor conditions, but my first attempts failed.

[vtitor]

@altendky Excellent! Thanks for taking the time to make this pull request. Could you also add some documentation to the README for the new option?

[altendky]

I don't like this (which works) or the previous setenv approach (which almost worked except for on AppVeyor with TOXENV). Haven't found something nicer though since I could

reactor_option=
    defaultreactor: default
    qt5reactor: qt5reactor
commands=py.test --reactor={reactor_option} []

but I don't know how to use that in a substitution in commands= and just get

tox.ConfigError: ConfigError: substitution key 'reactor_option' not found

[altendky]

I think I'm good with this finally... Any last concerns?

[vtitor]

@altendky Everything is fine. Thanks for the great job.