Opened 7 years ago

Closed 6 years ago

#3198 enhancement closed fixed (fixed)

add reactor-parameterized tests for system events

Reported by: exarkun Owned by:
Priority: normal Milestone:
Component: core Keywords:
Cc: therve, PenguinOfDoom Branch: branches/system-event-ordering-3198-2
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

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 6 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 7 years ago by exarkun

(In [23342]) Refer to another ticket

refs #3168
refs #3146
refs #3198

comment:2 Changed 7 years ago by exarkun

  • author set to exarkun
  • Branch changed from /branches/system-event-ordering-3146 to branches/system-event-ordering-3198-2

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

comment:3 Changed 7 years ago by exarkun

  • Owner changed from glyph to exarkun
  • Status changed from new to assigned

comment:4 Changed 6 years ago by exarkun

  • 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 6 years ago by therve

comment:5 Changed 6 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 6 years ago by therve

(In [23529]) Apply's patch

Refs #3198

comment:7 Changed 6 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted
  • Status changed from assigned to new

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 6 years ago by therve

  • Keywords review removed
  • Owner set to exarkun
  • 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 6 years ago by exarkun

  • Keywords review added
  • Owner changed from exarkun to therve
  • I'm not sure what you meant was missing from IReactorCore.running.
  • I added tests to test_core.py 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 6 years ago by therve

  • Keywords review removed
  • Owner changed from therve to exarkun

Please merge.

comment:11 Changed 6 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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 4 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.