Opened 11 years ago

Closed 11 years ago

#3198 enhancement closed fixed (fixed)

add reactor-parameterized tests for system events

Reported by: Jean-Paul Calderone Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: therve, PenguinOfDoom Branch: branches/system-event-ordering-3198-2
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun


There should be tests for the reactor event system which are run against all reactors.

Attachments (1)

3198-iocp.diff (1.7 KB) - added by therve 11 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 11 years ago by Jean-Paul Calderone

(In [23342]) Refer to another ticket

refs #3168 refs #3146 refs #3198

comment:2 Changed 11 years ago by Jean-Paul Calderone

author: exarkun
Branch: /branches/system-event-ordering-3146branches/system-event-ordering-3198-2

(In [23492]) Branching to 'system-event-ordering-3198-2'

comment:3 Changed 11 years ago by Jean-Paul Calderone

Owner: changed from Glyph to Jean-Paul Calderone
Status: newassigned

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

Cc: therve PenguinOfDoom added

The branch may be in more or less reasonable shape. However, iocpreactor fails at least one of the new tests such that the Windows slaves become unusable. It'd be nice if someone with a Windows development environment could look into this.

Changed 11 years ago by therve

Attachment: 3198-iocp.diff added

comment:5 Changed 11 years ago by therve

Attached patch fix run under windows with select reactor (when running the test of the iocp reactor). iocp wasn't passing the reactor argument properly, so the tests was using the global select reactor. I wonder if that can't happen somewhere without notice. Maybe we should delete twisted.internet.reactor during the tests?

comment:6 Changed 11 years ago by therve

(In [23529]) Apply's patch

Refs #3198

comment:7 Changed 11 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

Okay, thanks to therve's iocp patch and a few more minor tweaks, I guess this is ready for review.

I didn't try to do anything as drastic as eliminate twisted.internet.reactor for the duration of the tests, since that would be hard. It's certainly the case that there may be some shared state still hiding somewhere (in fact, it's not even really hiding - the process module is hell of shared - it'd be great to do something about that, perhaps even in this branch... or perhaps in another one).

comment:8 Changed 11 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone
  • the docstring of IReactorCore.running seems to miss a part
  • if I remove the addSystemEventTrigger in crash, no test in twisted.internet.test breaks. It seems vaguely useful but very indirectly
  • same thing for the _justStopped flag in stop, and in runUntilConcurrent
  • I don't see the use of the _installSignalHandlers flag: why not keep a system event? Is there a real reason

I suppose you realize the _started and _justStopped flags suck - just as the _stopped flag did. I feel it's a bit slightly tested, but well, it's not really a blocking point, because this branch already adds the ability to create good tests for reactors. So you can merge once the doc of the running flag is fixed.

comment:9 Changed 11 years ago by Jean-Paul Calderone

Keywords: review added
Owner: changed from Jean-Paul Calderone to therve
  • I'm not sure what you meant was missing from IReactorCore.running.
  • I added tests to covering the addSystemEventTrigger in `crash
  • I deleted the fireSystemEvent('shutdown') in crash, since that was the point of _justStopped/runUntilCurrent. Not sure how that call got there, I think I messed up the initial merge or something. The manipulation of the _justStopped flag is now necessary for test_core to pass.
  • signal handlers could be installed with a system event, but then there'd be a second system event that needs to be installed in two places. it could probably be done without too much mess, but I don't think it'll be much of a win.

_started and friends definitely suck. I added a bit to the docstrings for these in hopes that we won't forget that we're unhappy with them. :)

comment:10 Changed 11 years ago by therve

Keywords: review removed
Owner: changed from therve to Jean-Paul Calderone

Please merge.

comment:11 Changed 11 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [23634]) Merge system-event-ordering-3198-2

Author: exarkun Reviewer: therve Fixes: #3198

Add unit tests which are parameterized on the reactor implementation and are can be run against any (or all) available reactor, regardless of the reactor selected at trial invocation time.

Some tests are added for IReactorCore, IReactorTime, IReactorTCP, and IReactorProcess. None of these interfaces is completely covered by the new tests; some old tests have been removed in favor of these new ones. Eventually most of the old tests should be converted to this new style so that all reactors can be tested more easily.

Some small changes have been made to allow new reactors to be created and used independently of the global reactor. Since most reactor tests are still written in the old style, this doesn't mean that multiple reactors are now supported, only that they work well enough to pass the small number of new tests which require this.

The "running" attribute is now documented (as most people treated it as a public attribute already) and more thoroughly tested.

comment:12 Changed 8 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.