Opened 15 years ago

Closed 15 years ago

#1996 task closed fixed (fixed)

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

Reported by: Jean-Paul Calderone Owned by:
Priority: highest Milestone:
Component: web Keywords:
Cc: therve Branch:


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.

Attachments (1)

web_1996.diff (8.5 KB) - added by therve 15 years ago.

Download all attachments as: .zip

Change History (9)

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

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

Index: twisted/web/
--- twisted/web/       (revision 17814)
+++ twisted/web/       (working copy)
@@ -399,10 +399,15 @@ = site
         self.uid = uid
         self.expireCallbacks = []
-        self.checkExpiredLoop = task.LoopingCall(self.checkExpired)
         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):

comment:2 Changed 15 years ago by acapnotic

Component: coreweb

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().

Changed 15 years ago by therve

Attachment: web_1996.diff added

comment:3 Changed 15 years ago by therve

Cc: therve added
Keywords: review added
Owner: acapnotic deleted
Priority: normalhighest

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.

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

Keywords: review removed
Owner: set to therve

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 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.

comment:5 Changed 15 years ago by therve

Keywords: review added
Owner: therve deleted

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.

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

Keywords: review removed
Owner: set to therve

Looks good. Please merge.

comment:7 Changed 15 years ago by therve

Resolution: fixed
Status: newclosed

(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.

comment:8 Changed 11 years ago by <automation>

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