Remove repr() and getDelayedCalls() usage from twisted.web.test.test_web.SessionTest.test_delayedCallCleanup

[exarkun]

Comparing repr()s of things is pretty terrible. We shouldn't do it. Structured objects FTW.

getDelayedCalls should never have been added to IReactorTime. We should avoid using it wherever possible, remove all existing usages of it from the codebase, and then remove it. But that's a separate ticket. For this ticket, just remove it from test_delayedCallCleanup.

Session creation should be parameterized such that the clock used for scheduling is accessible to the test suite and a fake one which does not involve the reactor or real wallclock time can be specified for the test.

[exarkun]

I started to do this but the branch was merged before I finished:

Index: twisted/web/server.py
===================================================================
--- twisted/web/server.py       (revision 17814)
+++ twisted/web/server.py       (working copy)
@@ -399,10 +399,15 @@
         self.site = site
         self.uid = uid
         self.expireCallbacks = []
-        self.checkExpiredLoop = task.LoopingCall(self.checkExpired)
         self.touch()
         self.sessionNamespaces = {}
 
+
+    def startCheckingExpiration(self, lifetime):
+        self.checkExpiredLoop = task.LoopingCall(self.checkExpired)
+        self.checkExpiredLoop.start(lifetime)
+
+
     def notifyOnExpire(self, callback):
         """Call this callback when the session expires or logs out.
         """
@@ -443,6 +448,7 @@
 
     counter = 0
     requestFactory = Request
+    sessionFactory = Session
     displayTracebacks = True
     
     def __init__(self, resource, logPath=None, timeout=60*60*12):
@@ -472,8 +478,8 @@
         """Generate a new Session instance, and store it for future reference.
         """
         uid = self._mkuid()
-        session = self.sessions[uid] = Session(self, uid)
-        session.checkExpiredLoop.start(1800)
+        session = self.sessions[uid] = self.sessionFactory(self, uid)
+        session.startCheckingExpiration(1800)
         return session
 
     def getSession(self, uid):

[acapnotic]

I won't say that getDelayedCalls is not a bad thing to use, but I fear what you're suggesting for test_delayedCallCleanup changes the meaning of the test. That particular test is to ensure that the implementation of "startCheckingExpiration" that people use does not leave crap in the scheduler's reactor after the Session goes away. If you use a different implementation of startCheckingExpiration in the test, then you aren't testing that.

I also haven't figured out a way to test that without the use of getDelayedCalls().

[therve]

I went on and wrote something. acapnotic is right that we can't really test if there is no remain DelayedCall somewhere, but I considered that checking loop is enough. I also extracted some constants as class variables.

FWIW, I consider having a LoopingCall by session bad design. We should have a looping call for the Site checking all the sessions, unless I miss something.

[exarkun]

Changes look good, for the most part. A couple very minor points:

• Instead of if self.checkExpiredLoop:, consider if self.checkExpiredLoop is not None:. The case being tested for is really that a LoopingCall has been assigned to the attribute, not anything about the boolean value of the object it is bound to. Checking against None explicitly more directly expresses the intent here, in addition to avoiding all of the __nonzero__ stuff Python does for the former spelling.
• @ivar doc markup for the new attributes on Session and Site would be good.
• There are a few pyflakes warnings for test_web.py. As long as the file is being modified, it'd be good to clean these up.

The parameterization of the session timeout which is done here looks like it should also resolve #1090, as I suspect you have already noticed. :) If you agree, resolve that ticket as well when you merge this branch.

Also, thanks for the docstring reformatting.

[therve]

Putting it back for review(in session-cleanup-1996), for my far from perfect english, as I added a few doctrings. I also cleanups test_web and removed a useless string import.

[exarkun]

Looks good. Please merge.

[therve]

(In [19411]) Merge session-cleanup-1996

Author: therve Reviewer: exarkun Fixes #1996

getDelayedCalls() was used in twisted.web.test.test_web.SessionTest, whereas it's an internal method that should be removed. The test has been rewritten, and, in the process, web.Session and web.Site have been refactored to be tested easier. #1090 is also fixed by the changes.