Opened 8 years ago

Closed 8 years ago

#1996 task closed fixed (fixed)

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

Reported by: exarkun Owned by:
Priority: highest Milestone:
Component: web Keywords:
Cc: therve Branch:
Author: Launchpad Bug:

Description

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

Download all attachments as: .zip

Change History (9)

comment:1 Changed 8 years ago by 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):

comment:2 Changed 8 years ago by acapnotic

  • Component changed from core to web

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

comment:3 Changed 8 years ago by therve

  • Cc therve added
  • Keywords review added
  • Owner acapnotic deleted
  • Priority changed from normal to highest

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 8 years ago by exarkun

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

comment:5 Changed 8 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 8 years ago by exarkun

  • Keywords review removed
  • Owner set to therve

Looks good. Please merge.

comment:7 Changed 8 years ago by therve

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

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

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