Update of callbacks dict in RequestDispatcher.dispatch makes it difficult to write test case

[Ryan J Ollos]

For example I've written some test case like ​browser:trachacksplugin/trunk/trachacks/tests/web_ui.py@15127:109#L104, but when I try to use MockRequest the tests don't pass because the callbacks are overwritten: tags/trac-1.2/trac/web/main.py@:195-206#L184​.

I can make the tests work with the following change:

trac/web/main.py

@@ -192 +192 @@
         chrome = Chrome(self.env)
 
         # Setup request callbacks for lazily-evaluated properties
-        req.callbacks.update({
+        default_callbacks = {
             'authname': self.authenticate,
             'chrome': chrome.prepare_request,
             'form_token': self._get_form_token,
@@ -203 +203 @@
             'tz': self._get_timezone,
             'use_xsendfile': self._get_use_xsendfile,
             'xsendfile_header': self._get_xsendfile_header,
-        })
+        }
+        for key, value in default_callbacks.iteritems():
+            req.callbacks.setdefault(key, value)
 
         try:
             # Select the component that should handle the request

Does anyone foresee any negative side effects?

[Ryan J Ollos]

Changes in log:rjollos.git:t12713_req_callbacks. However, changes to test cases are just workarounds to get test to pass. If we adopt the change I think we should not use MockRequest in trac.web.tests.main, rather use _make_environ and _make_req as in trac.web.tests.api.

[Ryan J Ollos]

Possible alternate solution:

trac/web/main.py

@@ -191 +191 @@
         self.log.debug('Dispatching %r', req)
         chrome = Chrome(self.env)
 
-        # Setup request callbacks for lazily-evaluated properties
-        req.callbacks.update({
-            'authname': self.authenticate,
-            'chrome': chrome.prepare_request,
-            'form_token': self._get_form_token,
-            'lc_time': self._get_lc_time,
-            'locale': self._get_locale,
-            'perm': self._get_perm,
-            'session': self._get_session,
-            'tz': self._get_timezone,
-            'use_xsendfile': self._get_use_xsendfile,
-            'xsendfile_header': self._get_xsendfile_header,
-        })
-
         try:
             # Select the component that should handle the request
             chosen_handler = None
@@ -317 +303 @@
 
     # Internal methods
 
+    def set_default_callbacks(self, req):
+        """Setup request callbacks for lazily-evaluated properties.
+        """
+        req.callbacks.update({
+            'authname': self.authenticate,
+            'chrome': Chrome(self.env).prepare_request,
+            'form_token': self._get_form_token,
+            'lc_time': self._get_lc_time,
+            'locale': self._get_locale,
+            'perm': self._get_perm,
+            'session': self._get_session,
+            'tz': self._get_timezone,
+            'use_xsendfile': self._get_use_xsendfile,
+            'xsendfile_header': self._get_xsendfile_header,
+        })
+
     @lazy
     def _request_handlers(self):
         return dict((handler.__class__.__name__, handler)
@@ -615 +617 @@
     try:
         if not env and env_error:
             raise HTTPInternalError(env_error)
+        dispatcher = RequestDispatcher(env)
         try:
-            dispatcher = RequestDispatcher(env)
+            dispatcher.set_default_callbacks(req)
             dispatcher.dispatch(req)
         except RequestDone as req_done:
             resp = req_done.iterable

[Jun Omae]

The changes in comment:2 sound good. It might be able to change the callbacks by inheriting the dispatcher for other purposes.

[Ryan J Ollos]

Thanks, I also prefer the comment:2 change.

[Ryan J Ollos]

Committed to 1.2-stable in [15632:15633], merged to trunk in [15634]