Opened 4 years ago

Closed 4 years ago

#6596 enhancement closed fixed (fixed)

twisted.application.reactors.installReactor should return the installed reactor.

Reported by: Tom Prince Owned by: Tom Prince
Priority: normal Milestone:
Component: core Keywords:
Cc: Lukasz Dobrzanski Branch: branches/install-reactor-6596
branch-diff, diff-cov, branch-cov, buildbot
Author: tomprince

Description

In general, it is preferred to pass a reactor around explicitly, rather than just importing it from twisted.internet. However, when testing code that explicitly creates a reactor, the only way for that code to get a hold of the just installed reactor is to import it.

This means that that to test code that explicitly installs the reactor and then uses it, you need to stub out the code that installs the reactor *and* the global reactor.

If instead, the reactor that is installed is returned, then only the former needs to be stubbed out.

Change History (10)

comment:1 Changed 4 years ago by Tom Prince

Author: tomprince
Branch: branches/install-reactor-6596

(In [38916]) Branching to install-reactor-6596.

comment:2 Changed 4 years ago by Tom Prince

(In [38917]) Fix existing test, to match its docstring.

test_reactorInstallation claims to test that it test Reactor.install, but in fact only tests a stubbed out version.

This changes the test to actually test it, by modifying sys.modules.

Refs: #6596

comment:3 Changed 4 years ago by Tom Prince

Keywords: review added
Owner: Tom Prince deleted

I'd be inclined to change twisted.application.reactors.Reactor to return the reactor, but that wouldn't be compatible with existing plugins that don't use that to implement IReactorInstaller.

comment:4 Changed 4 years ago by Tom Prince

(In [38922]) A bit more coverage.

Refs: #6596

comment:5 Changed 4 years ago by Lukasz Dobrzanski

Cc: Lukasz Dobrzanski added
  • IReactorInstaller.install could need some extra docstring to reflect from twisted.internet import reactor; return reactor (it is tested using patch)
I am twisted newcomer not sure if letting IReactorInstaller.install return None
reactorObject would not make more sense then basing/modifying namespace

comment:6 Changed 4 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Tom Prince

Thanks for your work on this tom.prince and lukmdo.

In addition to the above:

  1. test_installReactorReturnsReactor uses assertEqual. I think this is an appropriate place to use assertIdentical. The API should return exactly the expected object, by identity, not merely something semantically equivalent to that object. I guess? Not that there are a lot of things that are semantically equivalent to any particular reactor.
  2. In the news fragment, "just-installed" is perhaps slightly more correct than "just installed".

I agree with your compatibility concerns regarding Reactor.install. If it were changed, then user code could only take advantage of that if they also handled all other implementations that don't return the reactor. So leaving it alone seems like the right thing to do for now.

Thanks again. Please merge after addressing these points to your satisfaction.

comment:7 in reply to:  5 Changed 4 years ago by Tom Prince

Replying to lukmdo:

I am twisted newcomer not sure if letting IReactorInstaller.install return None
reactorObject would not make more sense then basing/modifying namespace

In an ideal world, if the api was being designed now, IReactorInstaller.install would always return the reactor is just installed. But there exist[1] external implementations of IReactorInstaller.install that don't do that. Thus we can't depend on that behavior, so we would have to handle that case[2] in installReactor anyway.

[1] Even if there were no publicly availble external implementations, our CompatibilityPolicy requires we behave as if there were.

[2] In fact, we can't even assume that the return value will be a reactor or None. Since IReactorInstaller.install doesn't document a return value, it can potentially return anything.

IReactorInstaller.install could need some extra docstring to reflect from twisted.internet import reactor; return reactor`

I just did some digging, and I couldn't find any documentation that explains what "installing a reactor" means. I filed some tickets related to this (#6623, #6624, #6625).

comment:8 Changed 4 years ago by Tom Prince

(In [39158]) Use assertIdentical instead of assertEqual to compare reactors.

Refs: #6596

comment:9 Changed 4 years ago by Tom Prince

(In [39159]) topfile punctuation

Refs: #6596

comment:10 Changed 4 years ago by Tom Prince

Resolution: fixed
Status: newclosed

(In [39160]) Merge install-reactor-6596: Make twisted.application.reactors.installReactor return the installed reactor.

Author: tom.prince Reviewers: lukmdo, exarkun Fixes: #6596

This allows code that installs a reactor by name to be tested by just stubbing out installReactor, rather than both it *and* the global reactor.

Note: See TracTickets for help on using tickets.