fix assertion in tests, add new test for blockon usage in fixture

[ybilopolov]

There was a bug in assert_outcomes which made tests never fail.

[ybilopolov]

OK, this showed up few bigger issues. First of all, --reactor option was not passed to testdir.run calls in tests, so tests were always installing the default reactor. I've tried to fix this with 3263af3. But now we have broken tests for qt5 mode. This happens because of tests having reactor imports on their module level and test modules imports happen before fixture calls.

Frankly speaking, I'm not a huge fan of this new --reactor option, as I think it would be better to provide something like custom plugin hook (e.g. pytest_twisted_install_reactor) and let users to install there whatever reactor they want.

But still, I've tried to make current version compatible with direct test-level reactor imports by moving plugin reactor initialization to pytest_configure - 842ad41. What do you guys think?

[vtitor]

@ybilopolov Thanks for this. Custom plugin hook sounds good to me.

[ybilopolov]

I've read #16 more carefully and see that there was a requirement to call qt5reactor.install after pytest-qt's qapp fixture which I've broken with the last commit. I am not very familiar with Qt, can someone please explain why do we have this requirement? It doesn't look like an easy thing to do properly with the common approach in Twisted applications of getting currently installed reactor by simply importing it from twisted.internet (which installs the default one, if no other was already installed).

[vtitor]

@ybilopolov I am not familiar with qt too, but I guess that this is necessary to create the QApplication instance before qt5reactor.install call.

[vtitor]

Another strange behavior

@altendky Seems like appveyor doesn't work as expected

[altendky]

@ybilopolov Qt calls it's thing an event loop rather than a reactor but it's roughly the same thing as Twisted at that level. While there is QEventLoop, QApplication is generally how you get the primary event loop created (and whatever else QApplication does). So, to have a qt5reactor you need to have a QApplication.

This fixture approach was recommended by @nicoddemus afair so perhaps they can weigh in on the plugin alternative. Even in the original implementation we expected to be able to create a reactor at the time of plugin configuration which is also when the reactor cli option can be parsed so the developer has to manually pick the reactor to match whatever was set on the cli. Point being that even that isn't nice. I'm not particularly familiar with Pluggy so perhaps you could explain the custom plugin hook? When would this run? Would it be another module on PyPI for each (non-default) reactor you may choose to use?

My impression is that the Twisted approach of importing the reactor seems problematic exactly because it causes us to have these global get/create attempts that are racing to be first. I haven't discussed it in any detail but it just seems like an the obvious issue with singletons. Sure, you only want one event loop (in the 'normal' case) but you could still have reactor = twisted.internet.get_reactor() or such. For reference, Qt handles this with QApplication.instance(). Maybe just a side effect of being C++ based rather than Python but it at least doesn't encourage global imports of the singleton.

On the more detailed side, where does the qapp/QApplication get created now? It seems to be completely missing yet tests are passing. :[

@vtitor AppVeyor issue noted in #27

[nicoddemus]

This fixture approach was recommended by @nicoddemus afair so perhaps they can weigh in on the plugin alternative.

I proposed that because usually pytest-qt creates the QApplication object inside the qapp fixture:

https://github.com/pytest-dev/pytest-qt/blob/1b8d2d8362ca49f4e5e143363bcdf26e3ead4c04/pytestqt/plugin.py#L38-L53

If you don't want to rely on the fixture being present (makes sense because the user might not be using pytest-qt at all) notice that qapp doesn't try to create the QApplication again if it has been created already, so it is possible for someone else to create QApplication and install the reactor.

The pytest_twisted_install_reactor hook idea seems nice, but we still would have problems if twisted installs reactors just by importing some module; then users and pytest-twisted own test suite will need to take extra care to not import things at the module level if they want to use different reactors.