twisted.internet.test.reactormixins makes some mistakes about which reactors it tries to test

[exarkun]

The idea behind ReactorBuilder._reactors is that it's just a list of all the reactors we've got, and if the reactor can be imported and installed, it's supported on the current platform and should be tested.

This is a little too simple when it comes to the glib2 and gtk2 reactors though. Glib2Reactor and Gtk2Reactor can both be used on Windows, but actually PortableGtk2Reactor is what you get in real life if you ask for gtk2 on Windows. And Glib2Reactor really just shouldn't be used, I guess (although maybe that's a question to settle on another day).

A less important problem is that trying the full list all the time generates a mountain of skips at the end of every test run, because every platform has some reactors it is just never going to be able to run.

This list should be computed with a little more thought to avoid these problems.

This came up as I was trying to get some new tests working in the #4854 branch and noticed that Gtk2Reactor was being used on Windows.

[exarkun]

(In [31415]) Branching to 'reactormixins-4987'

[exarkun]

Please review.

The main change is the way the list of reactors to generate test cases for is constructed. Before it was static, now it is dynamic based on some platform checks. I fixed a bug and a half in PortableGtkReactor as well:

1. crash inside a during startup event wouldn't actually crash the reactor; an existing test_core.py test hit this.
2. SIGCHLD on POSIX wasn't being handled properly, since pygtk was stealing the handler. This is the half bug, since PortableGtkReactor on POSIX isn't really useful or supported.

 Latest build results

[spiv]

As Jerub says on IRC, the winxp32-py2.6 buildslave failure is concerning.

Also there's no news file snippet in this branch.

Otherwise this looks fine to me. Thanks for the clear description of what this patch is for. I see other folks on IRC are taking a look so I'll leave the review tag on and let someone else claim the glory…

[jesstess]

spiv: there is a twisted/topfiles/4987.misc, it's just empty. :)

Thanks for working on this, exarkun. I'm a little unclear on how the changes in test_tls.py fit together with #3371:

I'm reading

if platform.isWindows() and tls is None

as skip the TLS tests if you are on Windows and you have a pyOpenSSL older than 0.10. #3371 says there are some issues with OpenSSL on Windows so let's skip the TLS tests. Is having a new enough pyOpenSSL on Windows all it takes to not have the issues in #3371?

Other than that, I think this is good to merge.

[exarkun]

Thanks for the review spiv, jesstess. jesstess, excellent question about #3371.

I think I jumped the gun with the test_tls.py change. When #4854 is resolved, having a new enough version of pyOpenSSL should get rid of any special SSL/Gtk2 interactions on Windows by making an SSL connection look the same as a TCP connection to the reactor. I hope that means Glib2Reactor and Gtk2Reactor will be able to pass the currently skipped tests on Windows. However, I'm not sure.

I managed to convince myself that #4854 was already resolved (which of course it is not) and that the tests were now passing on Windows, but they are actually still skipping on Windows because none of the slaves have both pygtk and a new enough version of pyOpenSSL to avoid the skip. If they did, I expect we would have seen some failures.

So, I'll back out all the test_tls.py changes and try them again after #4854 is merged (and at least one Windows slave has the necessary versions of everything to actually try the test).

Thanks again for the review!

[exarkun]

(In [31476]) Back out changes which depend on #4854

refs #4987 refs #4854 refs #3371

[exarkun]

(In [31477]) Merge reactormixins-4987

Author: exarkun Reviewer: spiv, jesstess Fixes: #4987

Previously the ReactorBuilder-based tests kept a single list of reactors to generate unit tests for. Change this so that the current platform is considered when defining the list, and omit reactors which certainly cannot be tested on the platform in use.

Fix a couple small problems revealed by this, both with PortableGtkReactor on POSIX (where it isn't really supported, but the fixes are trivial).